From 5a155594a69dc78ee664d28607a89f4e3b97ceed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frank=20Schr=C3=B6der?= Date: Thu, 25 May 2023 13:54:39 +0200 Subject: [PATCH 1/2] Fix races to c.Session() and c.SecureChannel() In these cases the method calls c.SecureChannel and/or c.Session multiple times and during a re-connect the returned value can change. This patch addresses this by storing the returned value in a local variable. Fixes #640 --- client.go | 37 +++++++++++++++++++------------------ client_sub.go | 11 +++++++++-- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/client.go b/client.go index e106a245..7c2d20b9 100644 --- a/client.go +++ b/client.go @@ -199,6 +199,9 @@ func (c *Client) Connect(ctx context.Context) error { return c.cfgerr } + // todo(fs): the secure channel is 'nil' during a re-connect + // todo(fs): but we expect this method to be called once during startup + // todo(fs): so this is probably safe if c.SecureChannel() != nil { return errors.Errorf("already connected") } @@ -605,8 +608,8 @@ func (c *Client) CloseWithContext(ctx context.Context) error { if c.mcancel != nil { c.mcancel() } - if c.SecureChannel() != nil { - c.SecureChannel().Close() + if sc := c.SecureChannel(); sc != nil { + sc.Close() c.setSecureChannel(nil) } @@ -676,11 +679,6 @@ func (c *Client) setSession(s *Session) { stats.Client().Add("Session", 1) } -// sessionClosed returns true when there is no session. -func (c *Client) sessionClosed() bool { - return c.Session() == nil -} - // Session is a OPC/UA session as described in Part 4, 5.6. type Session struct { cfg *uasc.SessionConfig @@ -727,7 +725,8 @@ func (c *Client) CreateSession(cfg *uasc.SessionConfig) (*Session, error) { // Note: Starting with v0.5 this method is superseded by the non 'WithContext' method. func (c *Client) CreateSessionWithContext(ctx context.Context, cfg *uasc.SessionConfig) (*Session, error) { - if c.SecureChannel() == nil { + sc := c.SecureChannel() + if sc == nil { return nil, ua.StatusBadServerNotConnected } @@ -752,14 +751,14 @@ func (c *Client) CreateSessionWithContext(ctx context.Context, cfg *uasc.Session var s *Session // for the CreateSessionRequest the authToken is always nil. - // use c.SecureChannel().SendRequest() to enforce this. - err := c.SecureChannel().SendRequestWithContext(ctx, req, nil, func(v interface{}) error { + // use sc.SendRequest() to enforce this. + err := sc.SendRequestWithContext(ctx, req, nil, func(v interface{}) error { var res *ua.CreateSessionResponse if err := safeAssign(v, &res); err != nil { return err } - err := c.SecureChannel().VerifySessionSignature(res.ServerCertificate, nonce, res.ServerSignature.Signature) + err := sc.VerifySessionSignature(res.ServerCertificate, nonce, res.ServerSignature.Signature) if err != nil { log.Printf("error verifying session signature: %s", err) return nil @@ -820,11 +819,12 @@ func (c *Client) ActivateSession(s *Session) error { // Note: Starting with v0.5 this method is superseded by the non 'WithContext' method. func (c *Client) ActivateSessionWithContext(ctx context.Context, s *Session) error { - if c.SecureChannel() == nil { + sc := c.SecureChannel() + if sc == nil { return ua.StatusBadServerNotConnected } stats.Client().Add("ActivateSession", 1) - sig, sigAlg, err := c.SecureChannel().NewSessionSignature(s.serverCertificate, s.serverNonce) + sig, sigAlg, err := sc.NewSessionSignature(s.serverCertificate, s.serverNonce) if err != nil { log.Printf("error creating session signature: %s", err) return nil @@ -835,7 +835,7 @@ func (c *Client) ActivateSessionWithContext(ctx context.Context, s *Session) err // nothing to do case *ua.UserNameIdentityToken: - pass, passAlg, err := c.SecureChannel().EncryptUserPassword(s.cfg.AuthPolicyURI, s.cfg.AuthPassword, s.serverCertificate, s.serverNonce) + pass, passAlg, err := sc.EncryptUserPassword(s.cfg.AuthPolicyURI, s.cfg.AuthPassword, s.serverCertificate, s.serverNonce) if err != nil { log.Printf("error encrypting user password: %s", err) return err @@ -844,7 +844,7 @@ func (c *Client) ActivateSessionWithContext(ctx context.Context, s *Session) err tok.EncryptionAlgorithm = passAlg case *ua.X509IdentityToken: - tokSig, tokSigAlg, err := c.SecureChannel().NewUserTokenSignature(s.cfg.AuthPolicyURI, s.serverCertificate, s.serverNonce) + tokSig, tokSigAlg, err := sc.NewUserTokenSignature(s.cfg.AuthPolicyURI, s.serverCertificate, s.serverNonce) if err != nil { log.Printf("error creating session signature: %s", err) return err @@ -868,7 +868,7 @@ func (c *Client) ActivateSessionWithContext(ctx context.Context, s *Session) err UserIdentityToken: ua.NewExtensionObject(s.cfg.UserIdentityToken), UserTokenSignature: s.cfg.UserTokenSignature, } - return c.SecureChannel().SendRequestWithContext(ctx, req, s.resp.AuthenticationToken, func(v interface{}) error { + return sc.SendRequestWithContext(ctx, req, s.resp.AuthenticationToken, func(v interface{}) error { var res *ua.ActivateSessionResponse if err := safeAssign(v, &res); err != nil { return err @@ -965,14 +965,15 @@ func (c *Client) SendWithContext(ctx context.Context, req ua.Request, h func(int // the response. If the client has an active session it injects the // authentication token. func (c *Client) sendWithTimeout(ctx context.Context, req ua.Request, timeout time.Duration, h func(interface{}) error) error { - if c.SecureChannel() == nil { + sc := c.SecureChannel() + if sc == nil { return ua.StatusBadServerNotConnected } var authToken *ua.NodeID if s := c.Session(); s != nil { authToken = s.resp.AuthenticationToken } - return c.SecureChannel().SendRequestWithTimeoutWithContext(ctx, req, authToken, timeout, h) + return sc.SendRequestWithTimeoutWithContext(ctx, req, authToken, timeout, h) } // Node returns a node object which accesses its attributes diff --git a/client_sub.go b/client_sub.go index 6778f76c..e8b70d8b 100644 --- a/client_sub.go +++ b/client_sub.go @@ -171,14 +171,21 @@ func (c *Client) sendRepublishRequests(ctx context.Context, sub *Subscription, a req.RetransmitSequenceNumber, ) - if c.sessionClosed() { + s := c.Session() + if s == nil { debug.Printf("Republishing subscription %d aborted", req.SubscriptionID) return ua.StatusBadSessionClosed } + sc := c.SecureChannel() + if sc == nil { + debug.Printf("Republishing subscription %d aborted", req.SubscriptionID) + return ua.StatusBadNotConnected + } + debug.Printf("RepublishRequest: req=%s", debug.ToJSON(req)) var res *ua.RepublishResponse - err := c.SecureChannel().SendRequestWithContext(ctx, req, c.Session().resp.AuthenticationToken, func(v interface{}) error { + err := sc.SendRequestWithContext(ctx, req, s.resp.AuthenticationToken, func(v interface{}) error { return safeAssign(v, &res) }) debug.Printf("RepublishResponse: res=%s err=%v", debug.ToJSON(res), err) From 611b2d7c020a71eb2f9003a157340516cae66c57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frank=20Schr=C3=B6der?= Date: Thu, 25 May 2023 14:02:46 +0200 Subject: [PATCH 2/2] update method docs --- client.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/client.go b/client.go index 7c2d20b9..fc5f3e23 100644 --- a/client.go +++ b/client.go @@ -660,6 +660,8 @@ func (c *Client) setPublishTimeout(d time.Duration) { } // SecureChannel returns the active secure channel. +// During reconnect this value can change. +// Make sure to capture the value in a method before using it. func (c *Client) SecureChannel() *uasc.SecureChannel { return c.atomicSechan.Load().(*uasc.SecureChannel) } @@ -670,6 +672,8 @@ func (c *Client) setSecureChannel(sc *uasc.SecureChannel) { } // Session returns the active session. +// During reconnect this value can change. +// Make sure to capture the value in a method before using it. func (c *Client) Session() *Session { return c.atomicSession.Load().(*Session) }