Repository: gopcua/opcua
Pull request #664 (2023-06-14 06:51:08)

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.

magiconair: I've added https://github.com/stretchr/testify as a dependency for testing. I'm open for using something else like https://github.com/gotestyourself/gotest.tools if there are strong opinions. What do others think?

Patch:
diff --git a/uasc/secure_channel.go b/uasc/secure_channel.go
index 918e548..f6aa425 100644
--- 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{


Test patch:
diff --git a/uasc/secure_channel_test.go b/uasc/secure_channel_test.go
index 1149d8e..c31f1d9 100644
--- a/uasc/secure_channel_test.go
+++ b/uasc/secure_channel_test.go
@@ -7,14 +7,15 @@ import (
 	"encoding/pem"
 	"fmt"
 	"math"
+	"strings"
 	"testing"
 	"time"
 
 	"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"
 )
 
@@ -146,7 +147,7 @@ func TestNewRequestMessage(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			m, err := tt.sechan.activeInstance.newRequestMessage(tt.req, tt.sechan.nextRequestID(), tt.authToken, tt.timeout)
 			if err != nil {
-				t.Fatalf("got err %v want nil", err)
+				t.Fatal(err)
 			}
 			verify.Values(t, "", m, tt.m)
 		})
@@ -158,16 +159,22 @@ func TestSignAndEncryptVerifyAndDecrypt(t *testing.T) {
 		t.Helper()
 
 		certPEM, keyPEM, err := uatest.GenerateCert("localhost", bits, 24*time.Hour)
+		if err != nil {
+			t.Fatal(err)
+		}
+
 		block, _ := pem.Decode(keyPEM)
 		pk, err := x509.ParsePKCS1PrivateKey(block.Bytes)
 		if err != nil {
 			t.Fatal(err)
 		}
+
 		certblock, _ := pem.Decode(certPEM)
 		remoteX509Cert, err := x509.ParseCertificate(certblock.Bytes)
 		if err != nil {
 			t.Fatal(err)
 		}
+
 		remoteKey := remoteX509Cert.PublicKey.(*rsa.PublicKey)
 		alg, _ := uapolicy.Asymmetric(uri, pk, remoteKey)
 		return alg
@@ -306,3 +313,43 @@ func TestSignAndEncryptVerifyAndDecrypt(t *testing.T) {
 		})
 	}
 }
+
+func TestNewSecureChannel(t *testing.T) {
+	t.Run("no connection", func(t *testing.T) {
+		_, err := NewSecureChannel("", nil, nil, nil)
+		errorContains(t, err, "no connection")
+	})
+	t.Run("no error channel", func(t *testing.T) {
+		_, err := NewSecureChannel("", &uacp.Conn{}, nil, nil)
+		errorContains(t, err, "no secure channel config")
+	})
+	t.Run("no config", func(t *testing.T) {
+		_, err := NewSecureChannel("", &uacp.Conn{}, nil, make(chan error))
+		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))
+		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))
+		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))
+		errorContains(t, err, "invalid channel config: Security policy 'http://opcfoundation.org/UA/SecurityPolicy#Basic256' requires a private key")
+	})
+}
+
+func errorContains(t *testing.T, err error, msg string) {
+	t.Helper()
+	if err == nil {
+		t.Fatal("expected an error but got nil")
+	}
+	if !strings.Contains(err.Error(), msg) {
+		t.Fatalf("error '%s' does not contain '%s'", err, msg)
+	}
+}