Skip to content

Commit

Permalink
Don't panic on a nil credential value
Browse files Browse the repository at this point in the history
Just skip the authorization step.
  • Loading branch information
jhendrixMSFT committed Oct 25, 2023
1 parent 4d58405 commit 9f0a513
Show file tree
Hide file tree
Showing 7 changed files with 69 additions and 5 deletions.
1 change: 1 addition & 0 deletions sdk/azcore/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
7 changes: 7 additions & 0 deletions sdk/azcore/runtime/policy_bearer_token.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
}
Expand Down
12 changes: 12 additions & 0 deletions sdk/azcore/runtime/policy_bearer_token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
13 changes: 9 additions & 4 deletions sdk/azcore/runtime/policy_key_credential.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
17 changes: 17 additions & 0 deletions sdk/azcore/runtime/policy_key_credential_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
7 changes: 6 additions & 1 deletion sdk/azcore/runtime/policy_sas_credential.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
17 changes: 17 additions & 0 deletions sdk/azcore/runtime/polilcy_sas_credential_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

0 comments on commit 9f0a513

Please sign in to comment.