uasc: return an error for invalid uri/mode combinations with None

The previous code forced security mode None if the URI was None changing the incoming Config object. While it makes the code more robust it also has a side effect and we were already checking for other invalid combinations. This patch puts all checks in one place and returns an error if the combination is invalid.
gopcua · Jun 14, 2023 · 3219bac · 3219bac
1 parent 1130b79
commit 3219bac
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 14 deletions.
diff --git a/go.mod b/go.mod
@@ -5,11 +5,17 @@ go 1.20
 require (
 	github.com/pascaldekloe/goe v0.1.1
 	github.com/pkg/errors v0.9.1
+	github.com/stretchr/testify v1.8.4
 	golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1
 	golang.org/x/term v0.8.0
 )
 
-require golang.org/x/sys v0.8.0 // indirect
+require (
+	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
+	golang.org/x/sys v0.8.0 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
+)
 
 retract (
 	v0.2.5 // https://github.com/gopcua/opcua/issues/538

diff --git a/go.sum b/go.sum
@@ -1,10 +1,20 @@
+github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/pascaldekloe/goe v0.1.1 h1:Ah6WQ56rZONR3RW3qWa2NCZ6JAVvSpUcoLBaOmYFt9Q=
 github.com/pascaldekloe/goe v0.1.1/go.mod h1:KSyfaxQOh0HZPjDP1FL/kFtbqYqrALJTaMafFUIccqU=
 github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
 github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 h1:k/i9J1pBpvlfR+9QsetwPyERsqu1GIbi967PQMq3Ivc=
 golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1/go.mod h1:V1LtkGg67GoY2N1AnLN78QLrzxkLyJw7RJb1gzOOz9w=
 golang.org/x/sys v0.8.0 h1:EBmGv8NaZBZTWvrbjNoL6HVt+IVy3QDQpJs7VRIw3tU=
 golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.8.0 h1:n5xxQn2i3PC0yLAbjTpNT85q/Kgzcr2gIoX9OrJUols=
 golang.org/x/term v0.8.0/go.mod h1:xPskH00ivmX89bAKVGSKKtLOWNx2+17Eiy94tnKShWo=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
diff --git a/uasc/secure_channel.go b/uasc/secure_channel.go
@@ -129,19 +129,17 @@ func NewSecureChannel(endpoint string, c *uacp.Conn, cfg *Config, errCh chan<- e
 		return nil, errors.Errorf("no secure channel config")
 	}
 
-	if cfg.SecurityPolicyURI != ua.SecurityPolicyURINone {
-		if cfg.SecurityMode == ua.MessageSecurityModeNone {
-			return nil, errors.Errorf("invalid channel config: Security policy '%s' cannot be used with '%s'", cfg.SecurityPolicyURI, cfg.SecurityMode)
-		}
-		if cfg.LocalKey == nil {
-			return nil, errors.Errorf("invalid channel config: Security policy '%s' requires a private key", cfg.SecurityPolicyURI)
-		}
+	if errCh == nil {
+		return nil, errors.Errorf("no error channel")
 	}
 
-	// Force the security mode to None if the policy is also None
-	// TODO: I don't like that a SecureChannel changes the incoming config
-	if cfg.SecurityPolicyURI == ua.SecurityPolicyURINone {
-		cfg.SecurityMode = ua.MessageSecurityModeNone
+	switch {
+	case cfg.SecurityPolicyURI == ua.SecurityPolicyURINone && cfg.SecurityMode != ua.MessageSecurityModeNone:
+		return nil, errors.Errorf("invalid channel config: Security policy '%s' cannot be used with '%s'", cfg.SecurityPolicyURI, cfg.SecurityMode)
+	case cfg.SecurityPolicyURI != ua.SecurityPolicyURINone && cfg.SecurityMode == ua.MessageSecurityModeNone:
+		return nil, errors.Errorf("invalid channel config: Security policy '%s' cannot be used with '%s'", cfg.SecurityPolicyURI, cfg.SecurityMode)
+	case cfg.SecurityPolicyURI != ua.SecurityPolicyURINone && cfg.LocalKey == nil:
+		return nil, errors.Errorf("invalid channel config: Security policy '%s' requires a private key", cfg.SecurityPolicyURI)
 	}
 
 	s := &SecureChannel{

diff --git a/uasc/secure_channel_test.go b/uasc/secure_channel_test.go
@@ -10,12 +10,14 @@ import (
 	"testing"
 	"time"
 
+	"github.com/pascaldekloe/goe/verify"
+	"github.com/stretchr/testify/require"
+
 	"github.com/gopcua/opcua/id"
 	"github.com/gopcua/opcua/ua"
+	"github.com/gopcua/opcua/uacp"
 	"github.com/gopcua/opcua/uapolicy"
 	"github.com/gopcua/opcua/uatest"
-
-	"github.com/pascaldekloe/goe/verify"
 )
 
 func TestNewRequestMessage(t *testing.T) {
@@ -306,3 +308,33 @@ func TestSignAndEncryptVerifyAndDecrypt(t *testing.T) {
 		})
 	}
 }
+
+func TestNewSecureChannel(t *testing.T) {
+	t.Run("no connection", func(t *testing.T) {
+		_, err := NewSecureChannel("", nil, nil, nil)
+		require.ErrorContains(t, err, "no connection")
+	})
+	t.Run("no error channel", func(t *testing.T) {
+		_, err := NewSecureChannel("", &uacp.Conn{}, nil, nil)
+		require.ErrorContains(t, err, "no secure channel config")
+	})
+	t.Run("no config", func(t *testing.T) {
+		_, err := NewSecureChannel("", &uacp.Conn{}, nil, make(chan error))
+		require.ErrorContains(t, err, "no secure channel config")
+	})
+	t.Run("uri none, mode not none", func(t *testing.T) {
+		cfg := &Config{SecurityPolicyURI: ua.SecurityPolicyURINone, SecurityMode: ua.MessageSecurityModeSign}
+		_, err := NewSecureChannel("", &uacp.Conn{}, cfg, make(chan error))
+		require.ErrorContains(t, err, "invalid channel config: Security policy 'http://opcfoundation.org/UA/SecurityPolicy#None' cannot be used with 'MessageSecurityModeSign'")
+	})
+	t.Run("uri not none, mode none", func(t *testing.T) {
+		cfg := &Config{SecurityPolicyURI: ua.SecurityPolicyURIBasic256, SecurityMode: ua.MessageSecurityModeNone}
+		_, err := NewSecureChannel("", &uacp.Conn{}, cfg, make(chan error))
+		require.ErrorContains(t, err, "Security policy 'http://opcfoundation.org/UA/SecurityPolicy#Basic256' cannot be used with 'MessageSecurityModeNone'")
+	})
+	t.Run("uri not none, local key missing", func(t *testing.T) {
+		cfg := &Config{SecurityPolicyURI: ua.SecurityPolicyURIBasic256, SecurityMode: ua.MessageSecurityModeSign}
+		_, err := NewSecureChannel("", &uacp.Conn{}, cfg, make(chan error))
+		require.ErrorContains(t, err, "invalid channel config: Security policy 'http://opcfoundation.org/UA/SecurityPolicy#Basic256' requires a private key")
+	})
+}