Merge pull request #664 from gopcua/secure-channel-do-not-change-config

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

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{

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)
+	}
+}