-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
return 403 for wrapping requests when no token provided #18859
Conversation
changelog/18859.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:bug | |||
core/auth: Return a 403 instead of a 500 for wrapping requests when token is provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should say "when token isn't provided"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! Thank you for pointing that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I might suggest getting someone else's approval as well since I'm so new to the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just a couple of nits. Feel free to ignore.
http/sys_wrapping_test.go
Outdated
t.Fatal("expected error") | ||
} | ||
|
||
if respError, ok := err.(*api.ResponseError); !ok || respError.StatusCode != 403 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use errors.As
here instead? The go docs have some guidance about using As
over this pattern, "because the former will succeed if err wraps an *fs.PathError." Obviously in this case it would apply to api.ResponseError
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh neat! Thanks for sharing. I'll change.
vault/request_handling.go
Outdated
@@ -561,7 +561,7 @@ func (c *Core) handleCancelableRequest(ctx context.Context, req *logical.Request | |||
// be revoked after the call. So we have to do the validation here. | |||
valid, err := c.validateWrappingToken(ctx, req) | |||
if err != nil { | |||
return nil, fmt.Errorf("error validating wrapping token: %w", err) | |||
return logical.ErrorResponse(fmt.Errorf("error validating wrapping token: %w", err).Error()), logical.ErrPermissionDenied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to worry about constructing an error/wrapping the error here, since we're just calling .Error()
to get the string out of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, that's a good point. I'll change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Requests to wrapping APIs will respond with a 500 status code if a token is not provided. This PR ensures that Vault properly responds with a 403.
Fixes #18852