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

make error responses less verbose #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1e9293
make error responses less verbose
fairclothjm Apr 19, 2023
3fdde12
fix uts
fairclothjm Apr 19, 2023
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
9 changes: 6 additions & 3 deletions path_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,8 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque
_, err = jwt.NewOIDCDiscoveryKeySet(ctx, config.OIDCDiscoveryURL, config.OIDCDiscoveryCAPEM)
}
if err != nil {
return logical.ErrorResponse("error checking oidc discovery URL: %s", err.Error()), nil
b.Logger().Error("error checking oidc discovery URL", "error", err)
return logical.ErrorResponse("error checking oidc discovery URL"), nil
}

case config.OIDCClientID != "" && config.OIDCDiscoveryURL == "":
Expand All @@ -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 👍

return logical.ErrorResponse("error checking jwks_ca_pem"), nil
}

// Try to verify a correctly formatted JWT. The signature will fail to match, but other
Expand All @@ -284,7 +286,8 @@ func (b *jwtAuthBackend) pathConfigWrite(ctx context.Context, req *logical.Reque
}

if !strings.Contains(err.Error(), "failed to verify id token signature") {
return logical.ErrorResponse(errwrap.Wrapf("error checking jwks URL: {{err}}", err).Error()), nil
b.Logger().Error("error checking jwks URL", "error", err)
return logical.ErrorResponse("error checking jwks URL"), nil
}

case len(config.JWTValidationPubKeys) != 0:
Expand Down
4 changes: 2 additions & 2 deletions path_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func TestConfig_JWKS_Update_Invalid(t *testing.T) {
if resp == nil || !resp.IsError() {
t.Fatal("expected error")
}
if !strings.Contains(resp.Error().Error(), "get keys failed") {
if !strings.Contains(resp.Error().Error(), "error checking jwks URL") {
t.Fatalf("got unexpected error: %v", resp.Error())
}

Expand All @@ -267,7 +267,7 @@ func TestConfig_JWKS_Update_Invalid(t *testing.T) {
if resp == nil || !resp.IsError() {
t.Fatal("expected error")
}
if !strings.Contains(resp.Error().Error(), "failed to decode keys") {
if !strings.Contains(resp.Error().Error(), "error checking jwks URL") {
t.Fatalf("got unexpected error: %v", resp.Error())
}
}
Expand Down