Repository: gopcua/opcua
Pull request #188 (2019-05-08 14:24:48)

fix: return error status from Node.Attribute()
return error status from Node.Attribute() to prevent nil dereference
on cast in BrowseName() and DisplayName() when e.g. node not found


Patch:
diff --git a/client.go b/client.go
index ed5232c..3fad684 100644
--- a/client.go
+++ b/client.go
@@ -64,6 +64,13 @@ func NewClient(endpoint string, opts ...Option) *Client {
 		opt(c.cfg, c.sessionCfg)
 	}
 
+	// UserIdentityToken was removed from DefaultSessionConfig() so ensure a default still is set
+	if c.sessionCfg.UserIdentityToken == nil {
+		opt := AuthAnonymous()
+		opt(c.cfg, c.sessionCfg)
+		opt = AuthPolicyID("Anonymous")
+		opt(c.cfg, c.sessionCfg)
+	}
 	return c
 }
 
@@ -180,16 +187,6 @@ func (c *Client) CreateSession(cfg *uasc.SessionConfig) (*Session, error) {
 			return nil
 		}
 
-		// Ensure we have a valid identity token that the server will accept before trying to activate a session
-		if c.sessionCfg.UserIdentityToken == nil {
-			opt := AuthAnonymous()
-			opt(c.cfg, c.sessionCfg)
-
-			p := anonymousPolicyID(resp.ServerEndpoints)
-			opt = AuthPolicyID(p)
-			opt(c.cfg, c.sessionCfg)
-		}
-
 		s = &Session{
 			cfg:               cfg,
 			resp:              resp,
@@ -202,24 +199,6 @@ func (c *Client) CreateSession(cfg *uasc.SessionConfig) (*Session, error) {
 	return s, err
 }
 
-const defaultAnonymousPolicyID = "Anonymous"
-
-func anonymousPolicyID(endpoints []*ua.EndpointDescription) string {
-	for _, e := range endpoints {
-		if e.SecurityMode != ua.MessageSecurityModeNone || e.SecurityPolicyURI != ua.SecurityPolicyURINone {
-			continue
-		}
-
-		for _, t := range e.UserIdentityTokens {
-			if t.TokenType == ua.UserTokenTypeAnonymous {
-				return t.PolicyID
-			}
-		}
-	}
-
-	return defaultAnonymousPolicyID
-}
-
 // ActivateSession activates the session and associates it with the client. If
 // the client already has a session it will be closed. To retain the current
 // session call DetachSession.
diff --git a/config.go b/config.go
index 722f085..7f107c9 100644
--- a/config.go
+++ b/config.go
@@ -21,7 +21,7 @@ import (
 // to establish a secure channel and a session.
 func DefaultClientConfig() *uasc.Config {
 	return &uasc.Config{
-		SecurityPolicyURI: ua.SecurityPolicyURINone,
+		SecurityPolicyURI: uapolicy.SecurityPolicyNone,
 		SecurityMode:      ua.MessageSecurityModeNone,
 		Lifetime:          uint32(time.Hour / time.Millisecond),
 	}
@@ -165,8 +165,8 @@ func SecurityFromEndpoint(ep *ua.EndpointDescription, authType ua.UserTokenType)
 		}
 
 		if sc.UserIdentityToken == nil {
-			sc.UserIdentityToken = &ua.AnonymousIdentityToken{PolicyID: defaultAnonymousPolicyID}
-			sc.AuthPolicyURI = ua.SecurityPolicyURINone
+			sc.UserIdentityToken = &ua.AnonymousIdentityToken{PolicyID: "Anonymous"}
+			sc.AuthPolicyURI = uapolicy.SecurityPolicyNone
 		}
 
 	}
diff --git a/examples/crypto/crypto.go b/examples/crypto/crypto.go
index 86abf6e..cf90777 100644
--- a/examples/crypto/crypto.go
+++ b/examples/crypto/crypto.go
@@ -18,6 +18,7 @@ import (
 	"github.com/gopcua/opcua"
 	"github.com/gopcua/opcua/debug"
 	"github.com/gopcua/opcua/ua"
+	"github.com/gopcua/opcua/uapolicy"
 
 	"golang.org/x/crypto/ssh/terminal"
 )
@@ -135,11 +136,11 @@ func clientOptsFromFlags(endpoints []*ua.EndpointDescription) []opcua.Option {
 	switch {
 	case *policy == "auto":
 		// set it later
-	case strings.HasPrefix(*policy, ua.SecurityPolicyURIPrefix):
+	case strings.HasPrefix(*policy, uapolicy.SecurityPolicyURL):
 		secPolicy = *policy
 		*policy = ""
 	case *policy == "Basic128Rsa15" || *policy == "Basic256" || *policy == "Basic256Sha256":
-		secPolicy = ua.SecurityPolicyURIPrefix + *policy
+		secPolicy = uapolicy.SecurityPolicyURL + *policy
 		*policy = ""
 	default:
 		log.Fatalf("Invalid security policy: %s", *policy)
@@ -166,9 +167,9 @@ func clientOptsFromFlags(endpoints []*ua.EndpointDescription) []opcua.Option {
 	}
 
 	// Allow input of only one of sec-mode,sec-policy when choosing 'None'
-	if secMode == ua.MessageSecurityModeNone || secPolicy == ua.SecurityPolicyURINone {
+	if secMode == ua.MessageSecurityModeNone || secPolicy == uapolicy.SecurityPolicyNone {
 		secMode = ua.MessageSecurityModeNone
-		secPolicy = ua.SecurityPolicyURINone
+		secPolicy = uapolicy.SecurityPolicyNone
 	}
 
 	// Find the best endpoint based on our input and server recommendation (highest SecurityMode+SecurityLevel)
diff --git a/node.go b/node.go
index b04b1c1..929be80 100644
--- a/node.go
+++ b/node.go
@@ -65,9 +65,15 @@ func (n *Node) Attribute(attrID ua.AttributeID) (*ua.Variant, error) {
 		return nil, err
 	}
 	if len(res.Results) == 0 {
-		return nil, nil
+		// #188: we return StatusBadUnexpectedError because it is unclear, under what
+		// circumstances the server would return no error and no results in the response
+		return nil, ua.StatusBadUnexpectedError
 	}
-	return res.Results[0].Value, nil
+	value := res.Results[0].Value
+	if res.Results[0].Status != ua.StatusOK {
+		return value, res.Results[0].Status
+	}
+	return value, nil
 }
 
 // References retrns all references for the node.
diff --git a/ua/enums.go b/ua/enums.go
index ba9f0f5..c406c8e 100644
--- a/ua/enums.go
+++ b/ua/enums.go
@@ -82,16 +82,3 @@ const (
 	TypeIDVariant         TypeID = 24
 	TypeIDDiagnosticInfo  TypeID = 25
 )
-
-// SecurityPolicyURI is a listing of UA security policy URIs
-// Specification: Part 7, 6.6.161-166
-
-const (
-	SecurityPolicyURIPrefix              = "http://opcfoundation.org/UA/SecurityPolicy#"
-	SecurityPolicyURINone                = "http://opcfoundation.org/UA/SecurityPolicy#None"
-	SecurityPolicyURIBasic128Rsa15       = "http://opcfoundation.org/UA/SecurityPolicy#Basic128Rsa15"
-	SecurityPolicyURIBasic256            = "http://opcfoundation.org/UA/SecurityPolicy#Basic256"
-	SecurityPolicyURIBasic256Sha256      = "http://opcfoundation.org/UA/SecurityPolicy#Basic256Sha256"
-	SecurityPolicyURIAes128Sha256RsaOaep = "http://opcfoundation.org/UA/SecurityPolicy#Aes128_Sha256_RsaOaep"
-	SecurityPolicyURIAes256Sha256RsaPss  = "http://opcfoundation.org/UA/SecurityPolicy#Aes256_Sha256_RsaPss"
-)
diff --git a/uapolicy/securitypolicy.go b/uapolicy/securitypolicy.go
index 52d32ee..edae8aa 100644
--- a/uapolicy/securitypolicy.go
+++ b/uapolicy/securitypolicy.go
@@ -12,8 +12,16 @@ import (
 	"errors"
 	"fmt"
 	"sort"
+)
 
-	"github.com/gopcua/opcua/ua"
+const (
+	SecurityPolicyURL                 = "http://opcfoundation.org/UA/SecurityPolicy#"
+	SecurityPolicyNone                = "http://opcfoundation.org/UA/SecurityPolicy#None"
+	SecurityPolicyBasic128Rsa15       = "http://opcfoundation.org/UA/SecurityPolicy#Basic128Rsa15"
+	SecurityPolicyBasic256            = "http://opcfoundation.org/UA/SecurityPolicy#Basic256"
+	SecurityPolicyBasic256Sha256      = "http://opcfoundation.org/UA/SecurityPolicy#Basic256Sha256"
+	SecurityPolicyAes128Sha256RsaOaep = "http://opcfoundation.org/UA/SecurityPolicy#Aes128_Sha256_RsaOaep"
+	SecurityPolicyAes256Sha256RsaPss  = "http://opcfoundation.org/UA/SecurityPolicy#Aes256_Sha256_RsaPss"
 )
 
 // SupportedPolicies returns all supported Security Policies
@@ -34,21 +42,21 @@ func Asymmetric(uri string, localKey *rsa.PrivateKey, remoteKey *rsa.PublicKey)
 		return nil, fmt.Errorf("unsupported security policy %s", uri)
 	}
 
-	if uri != ua.SecurityPolicyURINone && (localKey == nil || remoteKey == nil) {
+	if uri != SecurityPolicyNone && (localKey == nil || remoteKey == nil) {
 		return nil, errors.New("invalid asymmetric security policy config: both keys required")
 	}
 
 	return p.asymmetric(localKey, remoteKey)
 }
 
-// Symmetric returns the symmetric encryption algorithm for the given security policy.
+// Symmetrics returns the symmetric encryption algorithm for the given security policy.
 func Symmetric(uri string, localNonce, remoteNonce []byte) (*EncryptionAlgorithm, error) {
 	p, ok := policies[uri]
 	if !ok {
 		return nil, fmt.Errorf("unsupported security policy %s", uri)
 	}
 
-	if uri != ua.SecurityPolicyURINone && (localNonce == nil || remoteNonce == nil) {
+	if uri != SecurityPolicyNone && (localNonce == nil || remoteNonce == nil) {
 		return nil, errors.New("invalid symmetric security policy config: both nonces required")
 	}
 
@@ -157,12 +165,12 @@ func (e *EncryptionAlgorithm) SignatureURI() string {
 }
 
 var policies = map[string]policy{
-	ua.SecurityPolicyURINone:                {newNoneAsymmetric, newNoneSymmetric},
-	ua.SecurityPolicyURIBasic128Rsa15:       {newBasic128Rsa15Asymmetric, newBasic128Rsa15Symmetric},
-	ua.SecurityPolicyURIBasic256:            {newBasic256Asymmetric, newBasic256Symmetric},
-	ua.SecurityPolicyURIBasic256Sha256:      {newBasic256Rsa256Asymmetric, newBasic256Rsa256Symmetric},
-	ua.SecurityPolicyURIAes128Sha256RsaOaep: {newAes128Sha256RsaOaepAsymmetric, newAes128Sha256RsaOaepSymmetric},
-	ua.SecurityPolicyURIAes256Sha256RsaPss:  {newAes256Sha256RsaPssAsymmetric, newAes256Sha256RsaPssSymmetric},
+	SecurityPolicyNone:                {newNoneAsymmetric, newNoneSymmetric},
+	SecurityPolicyBasic128Rsa15:       {newBasic128Rsa15Asymmetric, newBasic128Rsa15Symmetric},
+	SecurityPolicyBasic256:            {newBasic256Asymmetric, newBasic256Symmetric},
+	SecurityPolicyBasic256Sha256:      {newBasic256Rsa256Asymmetric, newBasic256Rsa256Symmetric},
+	SecurityPolicyAes128Sha256RsaOaep: {newAes128Sha256RsaOaepAsymmetric, newAes128Sha256RsaOaepSymmetric},
+	SecurityPolicyAes256Sha256RsaPss:  {newAes256Sha256RsaPssAsymmetric, newAes256Sha256RsaPssSymmetric},
 }
 
 type policy struct {
diff --git a/uasc/secure_channel.go b/uasc/secure_channel.go
index 760b802..5ac31b2 100644
--- a/uasc/secure_channel.go
+++ b/uasc/secure_channel.go
@@ -69,7 +69,7 @@ func NewSecureChannel(endpoint string, c *uacp.Conn, cfg *Config) (*SecureChanne
 		return nil, fmt.Errorf("no secure channel config")
 	}
 
-	if cfg.SecurityPolicyURI != ua.SecurityPolicyURINone {
+	if cfg.SecurityPolicyURI != uapolicy.SecurityPolicyNone {
 		if cfg.SecurityMode == ua.MessageSecurityModeNone {
 			return nil, fmt.Errorf("invalid channel config: Security policy '%s' cannot be used with '%s'", cfg.SecurityPolicyURI, cfg.SecurityMode)
 		}
@@ -79,7 +79,7 @@ func NewSecureChannel(endpoint string, c *uacp.Conn, cfg *Config) (*SecureChanne
 	}
 
 	// Force the security mode to None if the policy is also None
-	if cfg.SecurityPolicyURI == ua.SecurityPolicyURINone {
+	if cfg.SecurityPolicyURI == uapolicy.SecurityPolicyNone {
 		cfg.SecurityMode = ua.MessageSecurityModeNone
 	}
 
@@ -228,10 +228,6 @@ func (s *SecureChannel) SendAsync(svc interface{}, authToken *ua.NodeID) (resp c
 	if authToken == nil {
 		authToken = ua.NewTwoByteNodeID(0)
 	}
-
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
 	// the request header is always the first field
 	val := reflect.ValueOf(svc)
 	val.Elem().Field(0).Set(reflect.ValueOf(s.reqhdr))
@@ -267,9 +263,9 @@ func (s *SecureChannel) SendAsync(svc interface{}, authToken *ua.NodeID) (resp c
 
 	// register the handler
 	resp = make(chan Response)
-	// s.mu.Lock()
+	s.mu.Lock()
 	s.handler[reqid] = resp
-	//s.mu.Unlock()
+	s.mu.Unlock()
 	return resp, nil
 }
 
diff --git a/uasc/secure_channel_crypto.go b/uasc/secure_channel_crypto.go
index d97654c..8e6493c 100644
--- a/uasc/secure_channel_crypto.go
+++ b/uasc/secure_channel_crypto.go
@@ -167,8 +167,8 @@ func (s *SecureChannel) VerifySessionSignature(cert, nonce, signature []byte) er
 // EncryptUserPassword issues a new signature for the client to send in ActivateSessionRequest
 func (s *SecureChannel) EncryptUserPassword(policyURI, password string, cert, nonce []byte) ([]byte, string, error) {
 
-	if policyURI == ua.SecurityPolicyURINone {
-		return []byte(password), "", nil
+	if policyURI == uapolicy.SecurityPolicyNone {
+		return []byte(password), uapolicy.SecurityPolicyNone, nil
 	}
 
 	// If the User ID Token's policy was null, then default to the secure channel's policy
@@ -204,7 +204,7 @@ func (s *SecureChannel) EncryptUserPassword(policyURI, password string, cert, no
 // NewUserTokenSignature issues a new signature for the client to send in ActivateSessionRequest
 func (s *SecureChannel) NewUserTokenSignature(policyURI string, cert, nonce []byte) ([]byte, string, error) {
 
-	if policyURI == ua.SecurityPolicyURINone {
+	if policyURI == uapolicy.SecurityPolicyNone {
 		return nil, "", nil
 	}
 


Test patch:
diff --git a/uapolicy/securitypolicy_test.go b/uapolicy/securitypolicy_test.go
index be26549..983a6b1 100644
--- a/uapolicy/securitypolicy_test.go
+++ b/uapolicy/securitypolicy_test.go
@@ -13,8 +13,6 @@ import (
 	"testing"
 
 	"github.com/pascaldekloe/goe/verify"
-
-	"github.com/gopcua/opcua/ua"
 )
 
 func TestSupportedPolicies(t *testing.T) {
@@ -142,7 +140,7 @@ func TestEncryptionAlgorithms(t *testing.T) {
 
 			nonceLength := localAsymmetric.NonceLength()
 			localNonce, remoteNonce := makeNonce(nonceLength), makeNonce(nonceLength)
-			if nonceLength == 0 && uri != ua.SecurityPolicyURINone {
+			if nonceLength == 0 && uri != SecurityPolicyNone {
 				t.Fatalf("client nonce length zero")
 			}