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

CredentialsProvider errors cannot be unwrapped #2400

Closed
kgeckhart opened this issue Dec 1, 2023 · 2 comments · Fixed by #2403
Closed

CredentialsProvider errors cannot be unwrapped #2400

kgeckhart opened this issue Dec 1, 2023 · 2 comments · Fixed by #2403
Assignees
Labels
bug This issue is a bug. p1 This is a high priority issue

Comments

@kgeckhart
Copy link

kgeckhart commented Dec 1, 2023

Describe the bug

As of aws-sdk-go-v2 v1.23.0 any errors from the CredentialsProvider are being stringified instead of being wrapped. This breaks any code which might have been trying to unwrap these errors to an smithy.APIError for error handling purposes.

The docs about error handling mention all API errors should implement smithy.APIError https://aws.github.io/aws-sdk-go-v2/docs/handling-errors/#api-error-responses but due being stringified this no longer works.

Expected Behavior

Using the reproduction code on versions < 1.23 the error can be unwrapped to smithy.APIError

operation error STS: GetCallerIdentity, failed to sign request: failed to retrieve credentials: failed to refresh cached credentials, operation error STS: AssumeRole, https response error StatusCode: 403, RequestID: <not necessary>, api error AccessDenied: User: <redacted> is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::123456789:role/this-wont-have-access
error is api error

Current Behavior

Using the reproduction code on versions >= 1.23 the error can no longer be unwrapped to smithy.APIError

operation error STS: GetCallerIdentity, get identity: get credentials: failed to refresh cached credentials, operation error STS: AssumeRole, https response error StatusCode: 403, RequestID: <not necessary>, api error AccessDenied: User: <redacted> is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::123456789:role/this-wont-have-access
error is not api error

Reproduction Steps

package main

import (
	"context"
	"errors"
	"fmt"

	"github.com/aws/smithy-go"

	"github.com/aws/aws-sdk-go-v2/aws"
	"github.com/aws/aws-sdk-go-v2/config"
	"github.com/aws/aws-sdk-go-v2/credentials/stscreds"
	"github.com/aws/aws-sdk-go-v2/service/sts"
)

func main() {
	ctx := context.Background()
	c, err := config.LoadDefaultConfig(ctx)
	if err != nil {
		panic(err)
	}

	stsAssumeRole := sts.NewFromConfig(c)
	credentials := stscreds.NewAssumeRoleProvider(stsAssumeRole, "arn:aws:iam::123456789:role/this-wont-have-access")
	c.Credentials = aws.NewCredentialsCache(credentials)

	stsAccount := sts.NewFromConfig(c)
	_, err = stsAccount.GetCallerIdentity(ctx, &sts.GetCallerIdentityInput{})
	fmt.Println(err.Error())
	var awsError smithy.APIError
	if errors.As(err, &awsError) {
		fmt.Println("error is api error")
	} else {
		fmt.Println("error is not api error")
	}
}

Possible Solution

#2364 introduced some usages of fmt.Errorf(...: %v, err) which I think could be using %w to allow for wrapped errors.

  1. https://github.com/aws/aws-sdk-go-v2/blob/main/internal/auth/smithy/credentials_adapter.go#L42
  2. A few usages in the generated https://github.com/aws/aws-sdk-go-v2/blob/main/service/sts/auth.go
    1. https://github.com/aws/aws-sdk-go-v2/blob/main/service/sts/auth.go#L157
    2. https://github.com/aws/aws-sdk-go-v2/blob/main/service/sts/auth.go#L237: this one directly impacts CredentialsProvider but the others look like they could probably benefit from being %w as well
    3. https://github.com/aws/aws-sdk-go-v2/blob/main/service/sts/auth.go#L286

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

aws-sdk-go-v2 v1.23.0 and above

Compiler and Version used

n/a

Operating System and version

n/a

@kgeckhart kgeckhart added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 1, 2023
@lucix-aws lucix-aws added p1 This is a high priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Dec 1, 2023
@lucix-aws lucix-aws self-assigned this Dec 1, 2023
@lucix-aws
Copy link
Contributor

🤦 This is a silly mistake on my part. Thanks for the report. Fixing immediately.

Copy link

github-actions bot commented Dec 1, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p1 This is a high priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants