From c269413989ac1d368536f5a6c19c33e351014b16 Mon Sep 17 00:00:00 2001 From: Andrew Wilkins Date: Mon, 29 Jul 2024 12:47:40 +0800 Subject: [PATCH 1/2] [extension/bearertokenauth] minor improvements - clarify error message in case of missing header - don't use implementation code to verify expectations in tests - format header value ahead of time, rather than on every use, to avoid allocations - consistently synchronise access to header value for both client and server authenticators (now using sync/atomic.Value rather than RWMutex) --- .../bearertokenauth.go | 65 ++++++++++--------- .../bearertokenauth_test.go | 2 +- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/extension/bearertokenauthextension/bearertokenauth.go b/extension/bearertokenauthextension/bearertokenauth.go index b41521925142..e7a8ad3e4212 100644 --- a/extension/bearertokenauthextension/bearertokenauth.go +++ b/extension/bearertokenauthextension/bearertokenauth.go @@ -5,11 +5,12 @@ package bearertokenauthextension // import "github.com/open-telemetry/openteleme import ( "context" + "crypto/subtle" "errors" "fmt" "net/http" "os" - "sync" + "sync/atomic" "github.com/fsnotify/fsnotify" "go.opentelemetry.io/collector/component" @@ -42,9 +43,8 @@ var ( // BearerTokenAuth is an implementation of auth.Client. It embeds a static authorization "bearer" token in every rpc call. type BearerTokenAuth struct { - muTokenString sync.RWMutex - scheme string - tokenString string + scheme string + authorizationValueAtomic atomic.Value shutdownCH chan struct{} @@ -58,12 +58,13 @@ func newBearerTokenAuth(cfg *Config, logger *zap.Logger) *BearerTokenAuth { if cfg.Filename != "" && cfg.BearerToken != "" { logger.Warn("a filename is specified. Configured token is ignored!") } - return &BearerTokenAuth{ - scheme: cfg.Scheme, - tokenString: string(cfg.BearerToken), - filename: cfg.Filename, - logger: logger, + a := &BearerTokenAuth{ + scheme: cfg.Scheme, + filename: cfg.Filename, + logger: logger, } + a.setAuthorizationValue(string(cfg.BearerToken)) + return a } // Start of BearerTokenAuth does nothing and returns nil if no filename @@ -135,9 +136,21 @@ func (b *BearerTokenAuth) refreshToken() { b.logger.Error(err.Error()) return } - b.muTokenString.Lock() - b.tokenString = string(token) - b.muTokenString.Unlock() + b.setAuthorizationValue(string(token)) +} + +func (b *BearerTokenAuth) setAuthorizationValue(token string) { + value := token + if b.scheme != "" { + value = b.scheme + " " + value + } + b.authorizationValueAtomic.Store(value) +} + +// authorizationValue returns the Authorization header/metadata value +// to set for client auth, and expected value for server auth. +func (b *BearerTokenAuth) authorizationValue() string { + return b.authorizationValueAtomic.Load().(string) } // Shutdown of BearerTokenAuth does nothing and returns nil @@ -158,22 +171,15 @@ func (b *BearerTokenAuth) Shutdown(_ context.Context) error { // PerRPCCredentials returns PerRPCAuth an implementation of credentials.PerRPCCredentials that func (b *BearerTokenAuth) PerRPCCredentials() (credentials.PerRPCCredentials, error) { return &PerRPCAuth{ - metadata: map[string]string{"authorization": b.bearerToken()}, + metadata: map[string]string{"authorization": b.authorizationValue()}, }, nil } -func (b *BearerTokenAuth) bearerToken() string { - b.muTokenString.RLock() - token := fmt.Sprintf("%s %s", b.scheme, b.tokenString) - b.muTokenString.RUnlock() - return token -} - // RoundTripper is not implemented by BearerTokenAuth func (b *BearerTokenAuth) RoundTripper(base http.RoundTripper) (http.RoundTripper, error) { return &BearerAuthRoundTripper{ - baseTransport: base, - bearerTokenFunc: b.bearerToken, + baseTransport: base, + auth: b, }, nil } @@ -184,14 +190,11 @@ func (b *BearerTokenAuth) Authenticate(ctx context.Context, headers map[string][ auth, ok = headers["Authorization"] } if !ok || len(auth) == 0 { - return ctx, errors.New("authentication didn't succeed") + return ctx, errors.New("missing or empty authorization header") } token := auth[0] - expect := b.tokenString - if len(b.scheme) != 0 { - expect = fmt.Sprintf("%s %s", b.scheme, expect) - } - if expect != token { + expect := b.authorizationValue() + if subtle.ConstantTimeCompare([]byte(expect), []byte(token)) == 0 { return ctx, fmt.Errorf("scheme or token does not match: %s", token) } return ctx, nil @@ -199,8 +202,8 @@ func (b *BearerTokenAuth) Authenticate(ctx context.Context, headers map[string][ // BearerAuthRoundTripper intercepts and adds Bearer token Authorization headers to each http request. type BearerAuthRoundTripper struct { - baseTransport http.RoundTripper - bearerTokenFunc func() string + baseTransport http.RoundTripper + auth *BearerTokenAuth } // RoundTrip modifies the original request and adds Bearer token Authorization headers. @@ -209,6 +212,6 @@ func (interceptor *BearerAuthRoundTripper) RoundTrip(req *http.Request) (*http.R if req2.Header == nil { req2.Header = make(http.Header) } - req2.Header.Set("Authorization", interceptor.bearerTokenFunc()) + req2.Header.Set("Authorization", interceptor.auth.authorizationValue()) return interceptor.baseTransport.RoundTrip(req2) } diff --git a/extension/bearertokenauthextension/bearertokenauth_test.go b/extension/bearertokenauthextension/bearertokenauth_test.go index 990a6f7020d0..b09291327c9c 100644 --- a/extension/bearertokenauthextension/bearertokenauth_test.go +++ b/extension/bearertokenauthextension/bearertokenauth_test.go @@ -92,7 +92,7 @@ func TestBearerAuthenticator(t *testing.T) { } expectedHeaders := http.Header{ "Foo": {"bar"}, - "Authorization": {bauth.bearerToken()}, + "Authorization": {"Bearer " + string(cfg.BearerToken)}, } resp, err := roundTripper.RoundTrip(&http.Request{Header: orgHeaders}) From 2d6f02b9e2a56111610f8598b5f1c4683ffa5690 Mon Sep 17 00:00:00 2001 From: Alex Boten <223565+codeboten@users.noreply.github.com> Date: Thu, 8 Aug 2024 08:10:39 -0700 Subject: [PATCH 2/2] changelog Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com> --- .chloggen/codeboten_bearerauth-patch.yaml | 27 +++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 .chloggen/codeboten_bearerauth-patch.yaml diff --git a/.chloggen/codeboten_bearerauth-patch.yaml b/.chloggen/codeboten_bearerauth-patch.yaml new file mode 100644 index 000000000000..b69be898107b --- /dev/null +++ b/.chloggen/codeboten_bearerauth-patch.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: bearertokenauthextension + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: use constant time comparison + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [34516] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: []