Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ExpiredAuthenticationToken errors when using Azure CLI credentials. Fixes #521 #544

Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 8 additions & 18 deletions azure/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ func GetNewSession(ctx context.Context, d *plugin.QueryData, tokenAudience strin
if cachedData, ok := d.ConnectionManager.Cache.Get(cacheKey); ok {
session = cachedData.(*Session)
if session.Expires != nil && WillExpireIn(*session.Expires, 0) {
logger.Info("GetNewSession", "cache expired", "delete cache and obtain new session token")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still want this log message? If so, should this be Trace?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will convert this and others to the required levels.
Sorry, left over from testing

d.ConnectionManager.Cache.Delete(cacheKey)
} else {
return cachedData.(*Session), nil
Expand Down Expand Up @@ -160,45 +161,34 @@ func GetNewSession(ctx context.Context, d *plugin.QueryData, tokenAudience strin

// Get the subscription ID and tenant ID for "GRAPH" token audience
case "CLI":
logger.Trace("Creating new session authorizer from Azure CLI")
authorizer, err = auth.NewAuthorizerFromCLIWithResource(resource)
if err != nil {
logger.Error("GetNewSession", "NewAuthorizerFromCLIWithResource error", err)

// Check if the password was changed and the session token is stored in
// the system, or if the CLI is outdated
if strings.Contains(err.Error(), "invalid_grant") {
return nil, fmt.Errorf("ValidationError: The credential data used by the CLI has expired because you might have changed or reset the password. Please clear your browser's cookies and run 'az login'.")
}
return nil, err
}
default:
logger.Trace("Getting token for authorizer from Azure CLI")
logger.Warn("Getting token for authorizer from Azure CLI")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Warn("Getting token for authorizer from Azure CLI")
logger.Trace("Getting token for authorizer from Azure CLI")

token, err := cli.GetTokenFromCLI(resource)
if err != nil {
plugin.Logger(ctx).Error("GetNewSession", "cli.GetTokenFromCLI error", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
plugin.Logger(ctx).Error("GetNewSession", "cli.GetTokenFromCLI error", err)
plugin.Logger(ctx).Error("GetNewSession", "get_token_from_cli_error", err)

return nil, err
}

adalToken, err := token.ToADALToken()
expiresOn = types.Time(adalToken.Expires())
logger.Warn("GetNewSession", "Getting token for authorizer from Azure CLI, expiresOn", expiresOn.Local())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Warn("GetNewSession", "Getting token for authorizer from Azure CLI, expiresOn", expiresOn.Local())
logger.Trace("GetNewSession", "Getting token for authorizer from Azure CLI, expiresOn", expiresOn.Local())

Should this be Trace?


if err != nil {
logger.Error("GetNewSession", "Get token from Azure CLI error", err)

// Check if the password was changed and the session token is stored in
// the system, or if the CLI is outdated
// Check if the password was changed and the session token is stored in the system, or if the CLI is outdated
if strings.Contains(err.Error(), "invalid_grant") {
return nil, fmt.Errorf("ValidationError: The credential data used by the CLI has expired because you might have changed or reset the password. Please clear your browser's cookies and run 'az login'.")
}
return nil, err
}
authorizer = autorest.NewBearerAuthorizer(&adalToken)
default:
return nil, fmt.Errorf("GetNewSession. invalid authenticaion method, please check plugin configuration and restart plugin.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, fmt.Errorf("GetNewSession. invalid authenticaion method, please check plugin configuration and restart plugin.")
return nil, fmt.Errorf("invalid Azure authentication method: %w")

}

// Get the subscription ID and tenant ID from CLI if not set in connection
// config or environment variables
if authMethod == "CLI" && (settings.Values[auth.SubscriptionID] == "" || settings.Values[auth.TenantID] == "") {
logger.Trace("Getting subscription ID and/or tenant ID from from Azure CLI")
logger.Debug("Getting subscription ID and/or tenant ID from from Azure CLI")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Debug("Getting subscription ID and/or tenant ID from from Azure CLI")
logger.Trace("Getting subscription ID and/or tenant ID from from Azure CLI")

Should this be Trace or Debug? It seems like we use both throughout this function, so I'm a bit confused.

subscription, err := getSubscriptionFromCLI(resource)
if err != nil {
logger.Error("GetNewSession", "getSubscriptionFromCLI error", err)
Expand Down