From 9f0a5138ed87ef85b7543d0d6dae96c2cffc616b Mon Sep 17 00:00:00 2001 From: Joel Hendrix Date: Wed, 25 Oct 2023 12:43:16 -0700 Subject: [PATCH] Don't panic on a nil credential value Just skip the authorization step. --- sdk/azcore/CHANGELOG.md | 1 + sdk/azcore/runtime/policy_bearer_token.go | 7 +++++++ sdk/azcore/runtime/policy_bearer_token_test.go | 12 ++++++++++++ sdk/azcore/runtime/policy_key_credential.go | 13 +++++++++---- .../runtime/policy_key_credential_test.go | 17 +++++++++++++++++ sdk/azcore/runtime/policy_sas_credential.go | 7 ++++++- .../runtime/polilcy_sas_credential_test.go | 17 +++++++++++++++++ 7 files changed, 69 insertions(+), 5 deletions(-) diff --git a/sdk/azcore/CHANGELOG.md b/sdk/azcore/CHANGELOG.md index 1bfb7d46ef30..6a10d3934eec 100644 --- a/sdk/azcore/CHANGELOG.md +++ b/sdk/azcore/CHANGELOG.md @@ -13,6 +13,7 @@ * Fixed an issue that could cause some allowed HTTP header values to not show up in logs. * Include error text instead of error type in traces when the transport returns an error. * Fixed an issue that could cause an HTTP/2 request to hang when the TCP connection becomes unresponsive. +* Passing a `nil` credential value will no longer cause a panic. Instead, the authentication is skipped. ### Other Changes diff --git a/sdk/azcore/runtime/policy_bearer_token.go b/sdk/azcore/runtime/policy_bearer_token.go index f2a0ba9cea93..c7036efc8a3c 100644 --- a/sdk/azcore/runtime/policy_bearer_token.go +++ b/sdk/azcore/runtime/policy_bearer_token.go @@ -72,6 +72,13 @@ func (b *BearerTokenPolicy) authenticateAndAuthorize(req *policy.Request) func(p // Do authorizes a request with a bearer token func (b *BearerTokenPolicy) Do(req *policy.Request) (*http.Response, error) { + // skip adding the authorization header if no TokenCredential was provided. + // this prevents a panic that might be hard to diagnose and allows testing + // against http endpoints that don't require authentication. + if b.cred == nil { + return req.Next() + } + if strings.ToLower(req.Raw().URL.Scheme) != "https" { return nil, shared.NonRetriableError(errors.New("bearer token authentication is not permitted for non TLS protected (https) endpoints")) } diff --git a/sdk/azcore/runtime/policy_bearer_token_test.go b/sdk/azcore/runtime/policy_bearer_token_test.go index 630c42cb9915..f36717b77b8d 100644 --- a/sdk/azcore/runtime/policy_bearer_token_test.go +++ b/sdk/azcore/runtime/policy_bearer_token_test.go @@ -237,3 +237,15 @@ func TestBearerTokenPolicy_RequiresHTTPS(t *testing.T) { var nre errorinfo.NonRetriable require.ErrorAs(t, err, &nre) } + +func TestBearerTokenPolicy_NilCredential(t *testing.T) { + policy := NewBearerTokenPolicy(nil, nil, nil) + pl := exported.NewPipeline(shared.TransportFunc(func(req *http.Request) (*http.Response, error) { + require.Zero(t, req.Header.Get(shared.HeaderAuthorization)) + return &http.Response{}, nil + }), policy) + req, err := NewRequest(context.Background(), "GET", "http://contoso.com") + require.NoError(t, err) + _, err = pl.Do(req) + require.NoError(t, err) +} diff --git a/sdk/azcore/runtime/policy_key_credential.go b/sdk/azcore/runtime/policy_key_credential.go index 2e47a5bad065..9c6d0b844d6e 100644 --- a/sdk/azcore/runtime/policy_key_credential.go +++ b/sdk/azcore/runtime/policy_key_credential.go @@ -40,10 +40,15 @@ func NewKeyCredentialPolicy(cred *exported.KeyCredential, header string, options // Do implementes the Do method on the [policy.Polilcy] interface. func (k *KeyCredentialPolicy) Do(req *policy.Request) (*http.Response, error) { - val := exported.KeyCredentialGet(k.cred) - if k.prefix != "" { - val = k.prefix + val + // skip adding the authorization header if no KeyCredential was provided. + // this prevents a panic that might be hard to diagnose and allows testing + // against http endpoints that don't require authentication. + if k.cred != nil { + val := exported.KeyCredentialGet(k.cred) + if k.prefix != "" { + val = k.prefix + val + } + req.Raw().Header.Add(k.header, val) } - req.Raw().Header.Add(k.header, val) return req.Next() } diff --git a/sdk/azcore/runtime/policy_key_credential_test.go b/sdk/azcore/runtime/policy_key_credential_test.go index e26e009ff602..8092dc4bf3de 100644 --- a/sdk/azcore/runtime/policy_key_credential_test.go +++ b/sdk/azcore/runtime/policy_key_credential_test.go @@ -48,3 +48,20 @@ func TestKeyCredentialPolicy(t *testing.T) { _, err = pl.Do(req) require.NoError(t, err) } + +func TestKeyCredentialPolicy_NilCredential(t *testing.T) { + const headerName = "fake-auth" + policy := NewKeyCredentialPolicy(nil, headerName, nil) + require.NotNil(t, policy) + + pl := exported.NewPipeline(shared.TransportFunc(func(req *http.Request) (*http.Response, error) { + require.Zero(t, req.Header.Get(headerName)) + return &http.Response{}, nil + }), policy) + + req, err := NewRequest(context.Background(), http.MethodGet, "http://contoso.com") + require.NoError(t, err) + + _, err = pl.Do(req) + require.NoError(t, err) +} diff --git a/sdk/azcore/runtime/policy_sas_credential.go b/sdk/azcore/runtime/policy_sas_credential.go index 25266030ba21..ed3f97d85ea0 100644 --- a/sdk/azcore/runtime/policy_sas_credential.go +++ b/sdk/azcore/runtime/policy_sas_credential.go @@ -34,6 +34,11 @@ func NewSASCredentialPolicy(cred *exported.SASCredential, header string, options // Do implementes the Do method on the [policy.Polilcy] interface. func (k *SASCredentialPolicy) Do(req *policy.Request) (*http.Response, error) { - req.Raw().Header.Add(k.header, exported.SASCredentialGet(k.cred)) + // skip adding the authorization header if no SASCredential was provided. + // this prevents a panic that might be hard to diagnose and allows testing + // against http endpoints that don't require authentication. + if k.cred != nil { + req.Raw().Header.Add(k.header, exported.SASCredentialGet(k.cred)) + } return req.Next() } diff --git a/sdk/azcore/runtime/polilcy_sas_credential_test.go b/sdk/azcore/runtime/polilcy_sas_credential_test.go index 5853eed253d7..0df2828ca8e3 100644 --- a/sdk/azcore/runtime/polilcy_sas_credential_test.go +++ b/sdk/azcore/runtime/polilcy_sas_credential_test.go @@ -32,3 +32,20 @@ func TestSASCredentialPolicy(t *testing.T) { _, err = pl.Do(req) require.NoError(t, err) } + +func TestSASCredentialPolicy_NilCredential(t *testing.T) { + const headerName = "fake-auth" + policy := NewSASCredentialPolicy(nil, headerName, nil) + require.NotNil(t, policy) + + pl := exported.NewPipeline(shared.TransportFunc(func(req *http.Request) (*http.Response, error) { + require.Zero(t, req.Header.Get(headerName)) + return &http.Response{}, nil + }), policy) + + req, err := NewRequest(context.Background(), http.MethodGet, "http://contoso.com") + require.NoError(t, err) + + _, err = pl.Do(req) + require.NoError(t, err) +}