From 4e88d2b2ded58dc4a93ad9c1352175959640399f Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Fri, 7 Jan 2022 14:08:10 -0800 Subject: [PATCH 1/6] Make authentication errors structs --- sdk/azidentity/CHANGELOG.md | 3 +- sdk/azidentity/chained_token_credential.go | 2 +- .../client_certificate_credential_test.go | 4 +- .../client_secret_credential_test.go | 4 +- sdk/azidentity/environment_credential_test.go | 8 +- sdk/azidentity/errors.go | 86 ++++++++++--------- .../username_password_credential_test.go | 4 +- 7 files changed, 59 insertions(+), 52 deletions(-) diff --git a/sdk/azidentity/CHANGELOG.md b/sdk/azidentity/CHANGELOG.md index ce5be9f15710..7e46697f7fee 100644 --- a/sdk/azidentity/CHANGELOG.md +++ b/sdk/azidentity/CHANGELOG.md @@ -1,10 +1,11 @@ # Release History -## 0.12.1 (Unreleased) +## 0.13.0 (Unreleased) ### Features Added ### Breaking Changes +* Replaced `AuthenticationFailedError.RawResponse()` with a field having the same name ### Bugs Fixed diff --git a/sdk/azidentity/chained_token_credential.go b/sdk/azidentity/chained_token_credential.go index 7e3c8aefbaaf..fa86e6ae20da 100644 --- a/sdk/azidentity/chained_token_credential.go +++ b/sdk/azidentity/chained_token_credential.go @@ -53,7 +53,7 @@ func (c *ChainedTokenCredential) GetToken(ctx context.Context, opts policy.Token var authFailed AuthenticationFailedError if errors.As(err, &authFailed) { err = fmt.Errorf("Authentication failed:\n%s\n%s"+createChainedErrorMessage(errList), err) - authErr := newAuthenticationFailedError(err, authFailed.RawResponse()) + authErr := newAuthenticationFailedError(err, authFailed.RawResponse) return nil, authErr } return nil, err diff --git a/sdk/azidentity/client_certificate_credential_test.go b/sdk/azidentity/client_certificate_credential_test.go index 177813d7d190..a7d2c4be5e6c 100644 --- a/sdk/azidentity/client_certificate_credential_test.go +++ b/sdk/azidentity/client_certificate_credential_test.go @@ -177,7 +177,7 @@ func TestClientCertificateCredential_InvalidCertLive(t *testing.T) { if !errors.As(err, &e) { t.Fatal("expected AuthenticationFailedError") } - if e.RawResponse() == nil { - t.Fatal("expected RawResponse() to return a non-nil *http.Response") + if e.RawResponse == nil { + t.Fatal("expected a non-nil RawResponse") } } diff --git a/sdk/azidentity/client_secret_credential_test.go b/sdk/azidentity/client_secret_credential_test.go index f929b39066fe..00e93b9fb83e 100644 --- a/sdk/azidentity/client_secret_credential_test.go +++ b/sdk/azidentity/client_secret_credential_test.go @@ -62,7 +62,7 @@ func TestClientSecretCredential_InvalidSecretLive(t *testing.T) { if !errors.As(err, &e) { t.Fatal("expected AuthenticationFailedError") } - if e.RawResponse() == nil { - t.Fatal("expected RawResponse() to return a non-nil *http.Response") + if e.RawResponse == nil { + t.Fatal("expected a non-nil RawResponse") } } diff --git a/sdk/azidentity/environment_credential_test.go b/sdk/azidentity/environment_credential_test.go index 951f173a85e8..16aef6f31592 100644 --- a/sdk/azidentity/environment_credential_test.go +++ b/sdk/azidentity/environment_credential_test.go @@ -210,8 +210,8 @@ func TestEnvironmentCredential_InvalidClientSecretLive(t *testing.T) { if !errors.As(err, &e) { t.Fatal("expected AuthenticationFailedError") } - if e.RawResponse() == nil { - t.Fatal("expected RawResponse() to return a non-nil *http.Response") + if e.RawResponse == nil { + t.Fatal("expected a non-nil RawResponse") } } @@ -254,7 +254,7 @@ func TestEnvironmentCredential_InvalidPasswordLive(t *testing.T) { if !errors.As(err, &e) { t.Fatal("expected AuthenticationFailedError") } - if e.RawResponse() == nil { - t.Fatal("expected RawResponse() to return a non-nil *http.Response") + if e.RawResponse == nil { + t.Fatal("expected a non-nil RawResponse") } } diff --git a/sdk/azidentity/errors.go b/sdk/azidentity/errors.go index 777d0d663b8a..082917b853c6 100644 --- a/sdk/azidentity/errors.go +++ b/sdk/azidentity/errors.go @@ -4,7 +4,11 @@ package azidentity import ( + "bytes" + "encoding/json" "errors" + "fmt" + "io" "net/http" "github.com/Azure/azure-sdk-for-go/sdk/internal/errorinfo" @@ -12,74 +16,76 @@ import ( ) // AuthenticationFailedError indicates an authentication request has failed. -type AuthenticationFailedError interface { - errorinfo.NonRetriable - RawResponse() *http.Response - authenticationFailed() -} +type AuthenticationFailedError struct { + err error -type authenticationFailedError struct { - error - resp *http.Response + // RawResponse is the HTTP response motivating the error, if available. + RawResponse *http.Response } func newAuthenticationFailedError(err error, resp *http.Response) AuthenticationFailedError { if resp == nil { var e msal.CallErr if errors.As(err, &e) { - return authenticationFailedError{err, e.Resp} + return AuthenticationFailedError{err: e, RawResponse: e.Resp} } } - return authenticationFailedError{err, resp} + return AuthenticationFailedError{err: err, RawResponse: resp} } -// NonRetriable indicates that this error should not be retried. -func (authenticationFailedError) NonRetriable() { - // marker method +// Error implements the error interface for type ResponseError. +// Note that the message contents are not contractual and can change over time. +func (e AuthenticationFailedError) Error() string { + if e.RawResponse == nil { + return e.err.Error() + } + msg := &bytes.Buffer{} + fmt.Fprintf(msg, "%s %s://%s%s\n", e.RawResponse.Request.Method, e.RawResponse.Request.URL.Scheme, e.RawResponse.Request.URL.Host, e.RawResponse.Request.URL.Path) + fmt.Fprintln(msg, "--------------------------------------------------------------------------------") + fmt.Fprintf(msg, "RESPONSE %s\n", e.RawResponse.Status) + fmt.Fprintln(msg, "--------------------------------------------------------------------------------") + body, err := io.ReadAll(e.RawResponse.Body) + e.RawResponse.Body.Close() + if err != nil { + fmt.Fprintf(msg, "Error reading response body: %v", err) + } else if len(body) > 0 { + e.RawResponse.Body = io.NopCloser(bytes.NewReader(body)) + if err := json.Indent(msg, body, "", " "); err != nil { + // failed to pretty-print so just dump it verbatim + fmt.Fprint(msg, string(body)) + } + } else { + fmt.Fprint(msg, "Response contained no body") + } + fmt.Fprintln(msg, "\n--------------------------------------------------------------------------------") + return msg.String() } -// AuthenticationFailed indicates that an authentication attempt failed -func (authenticationFailedError) authenticationFailed() { +// NonRetriable indicates that this error should not be retried. +func (AuthenticationFailedError) NonRetriable() { // marker method } -// RawResponse returns the HTTP response motivating the error, if available. -func (e authenticationFailedError) RawResponse() *http.Response { - return e.resp -} - -var _ AuthenticationFailedError = (*authenticationFailedError)(nil) -var _ errorinfo.NonRetriable = (*authenticationFailedError)(nil) - -// CredentialUnavailableError indicates a credential can't attempt authenticate -// because it lacks required data or state. -type CredentialUnavailableError interface { - errorinfo.NonRetriable - credentialUnavailable() -} +var _ errorinfo.NonRetriable = (*AuthenticationFailedError)(nil) -type credentialUnavailableError struct { +// CredentialUnavailableError indicates a credential can't attempt +// authentication because it lacks required data or state. +type CredentialUnavailableError struct { credType string message string } func newCredentialUnavailableError(credType, message string) CredentialUnavailableError { - return credentialUnavailableError{credType: credType, message: message} + return CredentialUnavailableError{credType: credType, message: message} } -func (e credentialUnavailableError) Error() string { +func (e CredentialUnavailableError) Error() string { return e.credType + ": " + e.message } // NonRetriable indicates that this error should not be retried. -func (e credentialUnavailableError) NonRetriable() { - // marker method -} - -// CredentialUnavailable indicates that the credential didn't attempt to authenticate -func (e credentialUnavailableError) credentialUnavailable() { +func (e CredentialUnavailableError) NonRetriable() { // marker method } -var _ CredentialUnavailableError = (*credentialUnavailableError)(nil) -var _ errorinfo.NonRetriable = (*credentialUnavailableError)(nil) +var _ errorinfo.NonRetriable = (*CredentialUnavailableError)(nil) diff --git a/sdk/azidentity/username_password_credential_test.go b/sdk/azidentity/username_password_credential_test.go index b0e9f24a2650..7a88a8d58661 100644 --- a/sdk/azidentity/username_password_credential_test.go +++ b/sdk/azidentity/username_password_credential_test.go @@ -60,7 +60,7 @@ func TestUsernamePasswordCredential_InvalidPasswordLive(t *testing.T) { if !errors.As(err, &e) { t.Fatal("expected AuthenticationFailedError") } - if e.RawResponse() == nil { - t.Fatal("expected RawResponse() to return a non-nil *http.Response") + if e.RawResponse == nil { + t.Fatal("expected a non-nil RawResponse") } } From 09b5771dba5be56bed156177b3f4677324c208ff Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Fri, 7 Jan 2022 14:53:22 -0800 Subject: [PATCH 2/6] update version --- sdk/azidentity/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/azidentity/version.go b/sdk/azidentity/version.go index 83e7d514ab08..61c7d1404671 100644 --- a/sdk/azidentity/version.go +++ b/sdk/azidentity/version.go @@ -11,5 +11,5 @@ const ( component = "azidentity" // Version is the semantic version (see http://semver.org) of this module. - version = "v0.12.1" + version = "v0.13.0" ) From 29de73370d0c93f1f02fb6e9e42c4b207c3b19a8 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Tue, 11 Jan 2022 09:06:40 -0800 Subject: [PATCH 3/6] unexport CredentialUnavailableError --- sdk/azidentity/CHANGELOG.md | 3 ++- sdk/azidentity/chained_token_credential.go | 6 +++--- sdk/azidentity/errors.go | 14 +++++++------- sdk/azidentity/managed_identity_credential_test.go | 2 +- 4 files changed, 13 insertions(+), 12 deletions(-) diff --git a/sdk/azidentity/CHANGELOG.md b/sdk/azidentity/CHANGELOG.md index 7e46697f7fee..96bf67bec92f 100644 --- a/sdk/azidentity/CHANGELOG.md +++ b/sdk/azidentity/CHANGELOG.md @@ -1,11 +1,12 @@ # Release History -## 0.13.0 (Unreleased) +## 0.13.0 (2022-01-11) ### Features Added ### Breaking Changes * Replaced `AuthenticationFailedError.RawResponse()` with a field having the same name +* Unexported `CredentialUnavailableError` ### Bugs Fixed diff --git a/sdk/azidentity/chained_token_credential.go b/sdk/azidentity/chained_token_credential.go index fa86e6ae20da..e7b813c89bdc 100644 --- a/sdk/azidentity/chained_token_credential.go +++ b/sdk/azidentity/chained_token_credential.go @@ -43,10 +43,10 @@ func NewChainedTokenCredential(sources []azcore.TokenCredential, options *Chaine // ctx: Context controlling the request lifetime. // opts: Options for the token request, in particular the desired scope of the access token. func (c *ChainedTokenCredential) GetToken(ctx context.Context, opts policy.TokenRequestOptions) (token *azcore.AccessToken, err error) { - var errList []CredentialUnavailableError + var errList []credentialUnavailableError for _, cred := range c.sources { token, err = cred.GetToken(ctx, opts) - var credErr CredentialUnavailableError + var credErr credentialUnavailableError if errors.As(err, &credErr) { errList = append(errList, credErr) } else if err != nil { @@ -69,7 +69,7 @@ func (c *ChainedTokenCredential) GetToken(ctx context.Context, opts policy.Token return nil, credErr } -func createChainedErrorMessage(errList []CredentialUnavailableError) string { +func createChainedErrorMessage(errList []credentialUnavailableError) string { msg := "" for _, err := range errList { msg += err.Error() diff --git a/sdk/azidentity/errors.go b/sdk/azidentity/errors.go index 082917b853c6..138ac629cbd9 100644 --- a/sdk/azidentity/errors.go +++ b/sdk/azidentity/errors.go @@ -68,24 +68,24 @@ func (AuthenticationFailedError) NonRetriable() { var _ errorinfo.NonRetriable = (*AuthenticationFailedError)(nil) -// CredentialUnavailableError indicates a credential can't attempt +// credentialUnavailableError indicates a credential can't attempt // authentication because it lacks required data or state. -type CredentialUnavailableError struct { +type credentialUnavailableError struct { credType string message string } -func newCredentialUnavailableError(credType, message string) CredentialUnavailableError { - return CredentialUnavailableError{credType: credType, message: message} +func newCredentialUnavailableError(credType, message string) credentialUnavailableError { + return credentialUnavailableError{credType: credType, message: message} } -func (e CredentialUnavailableError) Error() string { +func (e credentialUnavailableError) Error() string { return e.credType + ": " + e.message } // NonRetriable indicates that this error should not be retried. -func (e CredentialUnavailableError) NonRetriable() { +func (e credentialUnavailableError) NonRetriable() { // marker method } -var _ errorinfo.NonRetriable = (*CredentialUnavailableError)(nil) +var _ errorinfo.NonRetriable = (*credentialUnavailableError)(nil) diff --git a/sdk/azidentity/managed_identity_credential_test.go b/sdk/azidentity/managed_identity_credential_test.go index 6d0a85d2c33a..6c90f4a01cc5 100644 --- a/sdk/azidentity/managed_identity_credential_test.go +++ b/sdk/azidentity/managed_identity_credential_test.go @@ -389,7 +389,7 @@ func TestManagedIdentityCredential_GetTokenIMDS400(t *testing.T) { } // cred should return CredentialUnavailableError when IMDS responds 400 to a token request. // Also, it shouldn't send another token request (mockIMDS will appropriately panic if it does). - var expected CredentialUnavailableError + var expected credentialUnavailableError for i := 0; i < 3; i++ { _, err = cred.GetToken(context.Background(), policy.TokenRequestOptions{Scopes: []string{msiScope}}) if !errors.As(err, &expected) { From 1c51b46c2759e822375bd63245f583e8076b3245 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Tue, 11 Jan 2022 10:09:48 -0800 Subject: [PATCH 4/6] update go.mod --- sdk/azidentity/go.mod | 6 ++---- sdk/azidentity/go.sum | 7 ++++--- sdk/azidentity/live_test.go | 4 ++-- sdk/azidentity/managed_identity_credential_test.go | 6 +++--- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/sdk/azidentity/go.mod b/sdk/azidentity/go.mod index b1a21a0f0f05..9829e90382f5 100644 --- a/sdk/azidentity/go.mod +++ b/sdk/azidentity/go.mod @@ -2,11 +2,9 @@ module github.com/Azure/azure-sdk-for-go/sdk/azidentity go 1.16 -replace github.com/Azure/azure-sdk-for-go/sdk/azcore => ../azcore - require ( - github.com/Azure/azure-sdk-for-go/sdk/azcore v0.20.0 - github.com/Azure/azure-sdk-for-go/sdk/internal v0.8.2 + github.com/Azure/azure-sdk-for-go/sdk/azcore v0.21.0 + github.com/Azure/azure-sdk-for-go/sdk/internal v0.8.3 github.com/AzureAD/microsoft-authentication-library-for-go v0.4.0 github.com/davecgh/go-spew v1.1.1 // indirect golang.org/x/crypto v0.0.0-20201016220609-9e8e0b390897 diff --git a/sdk/azidentity/go.sum b/sdk/azidentity/go.sum index ace9e01be592..9a943f908cb2 100644 --- a/sdk/azidentity/go.sum +++ b/sdk/azidentity/go.sum @@ -1,6 +1,7 @@ -github.com/Azure/azure-sdk-for-go/sdk/internal v0.8.1/go.mod h1:KLF4gFr6DcKFZwSuH8w8yEK6DpFl3LP5rhdvAb7Yz5I= -github.com/Azure/azure-sdk-for-go/sdk/internal v0.8.2 h1:rImM7Yjz9yUgpdxp3A4cZLm1JZuo4XbtIp2LrUZnwYw= -github.com/Azure/azure-sdk-for-go/sdk/internal v0.8.2/go.mod h1:KLF4gFr6DcKFZwSuH8w8yEK6DpFl3LP5rhdvAb7Yz5I= +github.com/Azure/azure-sdk-for-go/sdk/azcore v0.21.0 h1:8wVJL0HUP5yDFXvotdewORTw7Yu88JbreWN/mobSvsQ= +github.com/Azure/azure-sdk-for-go/sdk/azcore v0.21.0/go.mod h1:fBF9PQNqB8scdgpZ3ufzaLntG0AG7C1WjPMsiFOmfHM= +github.com/Azure/azure-sdk-for-go/sdk/internal v0.8.3 h1:E+m3SkZCN0Bf5q7YdTs5lSm2CYY3CK4spn5OmUIiQtk= +github.com/Azure/azure-sdk-for-go/sdk/internal v0.8.3/go.mod h1:KLF4gFr6DcKFZwSuH8w8yEK6DpFl3LP5rhdvAb7Yz5I= github.com/AzureAD/microsoft-authentication-library-for-go v0.4.0 h1:WVsrXCnHlDDX8ls+tootqRE87/hL9S/g4ewig9RsD/c= github.com/AzureAD/microsoft-authentication-library-for-go v0.4.0/go.mod h1:Vt9sXTKwMyGcOxSmLDMnGPgqsUg7m8pe215qMLrDXw4= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/sdk/azidentity/live_test.go b/sdk/azidentity/live_test.go index 1077fdb6b723..fb81d63e6c64 100644 --- a/sdk/azidentity/live_test.go +++ b/sdk/azidentity/live_test.go @@ -106,7 +106,7 @@ func TestMain(m *testing.M) { // TODO: reset matcher case recording.RecordingMode: // remove default sanitizers such as the OAuth response sanitizer - err := recording.ResetSanitizers(nil) + err := recording.ResetProxy(nil) if err != nil { panic(err) } @@ -151,7 +151,7 @@ func TestMain(m *testing.M) { } } defer func() { - err := recording.ResetSanitizers(nil) + err := recording.ResetProxy(nil) if err != nil { panic(err) } diff --git a/sdk/azidentity/managed_identity_credential_test.go b/sdk/azidentity/managed_identity_credential_test.go index 85ffe2ab98e5..e3b21bebdeaf 100644 --- a/sdk/azidentity/managed_identity_credential_test.go +++ b/sdk/azidentity/managed_identity_credential_test.go @@ -400,7 +400,7 @@ func TestManagedIdentityCredential_GetTokenIMDS400(t *testing.T) { for i := 0; i < 3; i++ { _, err = cred.GetToken(context.Background(), policy.TokenRequestOptions{Scopes: []string{liveTestScope}}) if !errors.As(err, &expected) { - t.Fatalf(`expected CredentialUnavailableError, got %T: "%s"`, err, err.Error()) + t.Fatalf(`expected credentialUnavailableError, got %T: "%s"`, err, err.Error()) } } } @@ -731,9 +731,9 @@ func TestManagedIdentityCredential_IMDSTimeoutExceeded(t *testing.T) { } cred.client.imdsTimeout = time.Nanosecond tk, err := cred.GetToken(context.Background(), policy.TokenRequestOptions{Scopes: []string{liveTestScope}}) - var expected CredentialUnavailableError + var expected credentialUnavailableError if !errors.As(err, &expected) { - t.Fatalf(`expected CredentialUnavailableError, got %T: "%v"`, err, err) + t.Fatalf(`expected credentialUnavailableError, got %T: "%v"`, err, err) } if tk != nil { t.Fatal("GetToken returned a token and an error") From e0c872c528c2f410df31939254d1d4866b49c94e Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Tue, 11 Jan 2022 10:26:36 -0800 Subject: [PATCH 5/6] remove empty changelog sections --- sdk/azidentity/CHANGELOG.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/sdk/azidentity/CHANGELOG.md b/sdk/azidentity/CHANGELOG.md index c0f8b49ff1f8..e81012f1c899 100644 --- a/sdk/azidentity/CHANGELOG.md +++ b/sdk/azidentity/CHANGELOG.md @@ -2,14 +2,10 @@ ## 0.13.0 (2022-01-11) -### Features Added - ### Breaking Changes * Replaced `AuthenticationFailedError.RawResponse()` with a field having the same name * Unexported `CredentialUnavailableError` -### Bugs Fixed - ### Other Changes * `ManagedIdentityCredential` no longer probes IMDS before requesting a token from it. Also, an error response from IMDS no longer disables a credential From 3669e314eb1ca48e2bcfc3b4cc0f1c1677132599 Mon Sep 17 00:00:00 2001 From: Charles Lowell <10964656+chlowell@users.noreply.github.com> Date: Tue, 11 Jan 2022 11:11:48 -0800 Subject: [PATCH 6/6] note dependency changes in changelog --- sdk/azidentity/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/azidentity/CHANGELOG.md b/sdk/azidentity/CHANGELOG.md index bbbb933374c5..11ab8b1caab0 100644 --- a/sdk/azidentity/CHANGELOG.md +++ b/sdk/azidentity/CHANGELOG.md @@ -16,6 +16,8 @@ from it. Also, an error response from IMDS no longer disables a credential instance. Following an error, a credential instance will continue to send requests to IMDS as necessary. +* Adopted MSAL for user and service principal authentication +* Updated `azcore` requirement to 0.21.0 ## 0.12.0 (2021-11-02) ### Breaking Changes