Skip to content
New issue

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

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

Already on GitHub? # to your account

make error responses less verbose #233

Merged
merged 2 commits into from
Apr 19, 2023
Merged

Conversation

fairclothjm
Copy link
Contributor

No description provided.

Copy link
Contributor

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @fairclothjm!

@@ -272,7 +273,8 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque
case config.JWKSURL != "":
keyset, err := jwt.NewJSONWebKeySet(ctx, config.JWKSURL, config.JWKSCAPEM)
if err != nil {
return logical.ErrorResponse(errwrap.Wrapf("error checking jwks_ca_pem: {{err}}", err).Error()), nil
b.Logger().Error("error checking jwks_ca_pem", "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.

I realize it was like this before, but I think this log and error should be like error checking jwks_url. I guess it could be a problem with the CA too.. So maybe error checking jwks_url or jwks_ca_pem? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like NewJSONWebKeySet() returns an error if jwks url is empty or if there is an error with the CA. On line 273 we check that the jwks url is not empty so I think this log is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Missed that we already check if it's not empty 👍

@fairclothjm fairclothjm merged commit 476196d into main Apr 19, 2023
@fairclothjm fairclothjm deleted the less-verbose-error-resp branch April 19, 2023 21:06
@fairclothjm
Copy link
Contributor Author

Thanks!

fairclothjm added a commit that referenced this pull request Apr 20, 2023
* make error responses less verbose

* fix uts
fairclothjm added a commit that referenced this pull request Apr 20, 2023
* make error responses less verbose

* fix uts
jameshartig pushed a commit to jameshartig/vault-plugin-auth-jwt that referenced this pull request May 10, 2023
* make error responses less verbose

* fix uts
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants