Require number, CIDR or auth for SIP inbound. (#890)

.changeset/violet-sheep-bathe.md

@@ -0,0 +1,5 @@
+---
+"github.com/livekit/protocol": patch
+---
+
+Require number, CIDR or auth for SIP inbound.

livekit/sip.go

@@ -122,6 +122,9 @@ func (p *CreateSIPOutboundTrunkRequest) Validate() error {
 	if p.Trunk == nil {
 		return errors.New("missing trunk")
 	}
+	if p.Trunk.SipTrunkId != "" {
+		return errors.New("trunk id must not be set")
+	}
 	if err := p.Trunk.Validate(); err != nil {
 		return err
 	}
@@ -132,15 +135,21 @@ func (p *CreateSIPInboundTrunkRequest) Validate() error {
 	if p.Trunk == nil {
 		return errors.New("missing trunk")
 	}
+	if p.Trunk.SipTrunkId != "" {
+		return errors.New("trunk id must not be set")
+	}
 	if err := p.Trunk.Validate(); err != nil {
 		return err
 	}
 	return nil
 }
 
 func (p *SIPInboundTrunkInfo) Validate() error {
-	if len(p.Numbers) == 0 {
-		return errors.New("no trunk numbers specified")
+	hasAuth := p.AuthUsername != "" || p.AuthPassword != ""
+	hasCIDR := len(p.AllowedAddresses) != 0
+	hasNumbers := len(p.Numbers) != 0 // TODO: remove this condition, it doesn't really help with security
+	if !hasAuth && !hasCIDR && !hasNumbers {
+		return errors.New("for security, one of the fields must be set: AuthUsername+AuthPassword, AllowedAddresses or Numbers")
 	}
 	if err := validateHeaders(p.Headers); err != nil {
 		return err
@@ -157,7 +166,9 @@ func (p *SIPOutboundTrunkInfo) Validate() error {
 	}
 	if p.Address == "" {
 		return errors.New("no outbound address specified")
-	} else if strings.Contains(p.Address, "@") {
+	} else if strings.Contains(p.Address, "transport=") {
+		return errors.New("trunk transport should be set as a field, not a URI parameter")
+	} else if strings.ContainsAny(p.Address, "@;") || strings.HasPrefix(p.Address, "sip:") || strings.HasPrefix(p.Address, "sips:") {
 		return errors.New("trunk address should be a hostname or IP, not SIP URI")
 	}
 	if err := validateHeaders(p.Headers); err != nil {

livekit/sip_test.go

@@ -1,8 +1,9 @@
 package livekit
 
 import (
-	"github.com/stretchr/testify/require"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestSIPTrunkAs(t *testing.T) {
@@ -35,3 +36,141 @@ func TestSIPTrunkAs(t *testing.T) {
 		require.Equal(t, out, got)
 	})
 }
+
+func TestSIPValidate(t *testing.T) {
+	cases := []struct {
+		name string
+		req  interface {
+			Validate() error
+		}
+		exp bool
+	}{
+		{
+			name: "inbound empty",
+			req:  &SIPInboundTrunkInfo{},
+			exp:  false,
+		},
+		{
+			name: "inbound numbers",
+			req: &SIPInboundTrunkInfo{
+				Numbers: []string{"+1111"},
+			},
+			exp: true,
+		},
+		{
+			name: "inbound ips",
+			req: &SIPInboundTrunkInfo{
+				AllowedAddresses: []string{"1.1.1.1"},
+			},
+			exp: true,
+		},
+		{
+			name: "inbound auth",
+			req: &SIPInboundTrunkInfo{
+				AuthUsername: "user",
+				AuthPassword: "pass",
+			},
+			exp: true,
+		},
+		{
+			name: "inbound x-header",
+			req: &SIPInboundTrunkInfo{
+				Numbers: []string{"+1111"},
+				HeadersToAttributes: map[string]string{
+					"X-Test": "test",
+				},
+			},
+			exp: true,
+		},
+		{
+			name: "inbound other header",
+			req: &SIPInboundTrunkInfo{
+				Numbers: []string{"+1111"},
+				HeadersToAttributes: map[string]string{
+					"From": "from",
+				},
+			},
+			exp: false,
+		},
+		{
+			name: "outbound empty",
+			req:  &SIPOutboundTrunkInfo{},
+			exp:  false,
+		},
+		{
+			name: "outbound no numbers",
+			req: &SIPOutboundTrunkInfo{
+				Address: "sip.example.com",
+			},
+			exp: false,
+		},
+		{
+			name: "outbound with numbers",
+			req: &SIPOutboundTrunkInfo{
+				Address: "sip.example.com",
+				Numbers: []string{"+2222"},
+			},
+			exp: true,
+		},
+		{
+			name: "outbound with user",
+			req: &SIPOutboundTrunkInfo{
+				Address: "[email protected]",
+				Numbers: []string{"+2222"},
+			},
+			exp: false,
+		},
+		{
+			name: "outbound with transport",
+			req: &SIPOutboundTrunkInfo{
+				Address: "sip.example.com;transport=tcp",
+				Numbers: []string{"+2222"},
+			},
+			exp: false,
+		},
+		{
+			name: "outbound with schema",
+			req: &SIPOutboundTrunkInfo{
+				Address: "sip:example.com",
+				Numbers: []string{"+2222"},
+			},
+			exp: false,
+		},
+		{
+			name: "outbound with schema (tls)",
+			req: &SIPOutboundTrunkInfo{
+				Address: "sips:example.com",
+				Numbers: []string{"+2222"},
+			},
+			exp: false,
+		},
+		{
+			name: "outbound x-header",
+			req: &SIPOutboundTrunkInfo{
+				Address: "sip.example.com",
+				Numbers: []string{"+2222"},
+				HeadersToAttributes: map[string]string{
+					"X-Test": "test",
+				},
+			},
+			exp: true,
+		},
+		{
+			name: "outbound other header",
+			req: &SIPOutboundTrunkInfo{
+				Address: "sip.example.com",
+				Numbers: []string{"+2222"},
+				HeadersToAttributes: map[string]string{
+					"From": "from",
+				},
+			},
+			exp: false,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			err := c.req.Validate()
+			require.Equal(t, c.exp, err == nil, "error: %v", err)
+		})
+	}
+}