From 2bc1f99331e7b63bdbefca7e71c3a7c3a6004fda Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 22 Jun 2022 10:17:08 -0700 Subject: [PATCH 1/5] Return a 403 for a bad SSCT instead of 500 --- vault/request_handling.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/vault/request_handling.go b/vault/request_handling.go index dbbe5b21cf8a..5c6e789cdb81 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -582,13 +582,16 @@ func (c *Core) handleCancelableRequest(ctx context.Context, req *logical.Request if token == nil { return logical.ErrorResponse("invalid token"), logical.ErrPermissionDenied } - // We don't care if the token is an server side consistent token or not. Either way, we're going + // We don't care if the token is a server side consistent token or not. Either way, we're going // to be returning it for these paths instead of the short token stored in vault. requestBodyToken = token.(string) if IsSSCToken(token.(string)) { token, err = c.CheckSSCToken(ctx, token.(string), c.isLoginRequest(ctx, req), c.perfStandby) + + // If we receive an error from CheckSSCToken, we can assume the token is bad somehow, and the client + // should receive a 403 bad token error like they do for all other invalid tokens. if err != nil { - return nil, fmt.Errorf("server side consistent token check failed: %w", err) + return logical.ErrorResponse(fmt.Sprintf("bad token: %s", err.Error())), logical.ErrPermissionDenied } req.Data["token"] = token } From 7c908e83619165179d102de0be7ea334543b95fb Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 22 Jun 2022 10:30:36 -0700 Subject: [PATCH 2/5] log the error instead of returning it --- vault/request_handling.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vault/request_handling.go b/vault/request_handling.go index 5c6e789cdb81..4fddec9864bd 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -591,7 +591,8 @@ func (c *Core) handleCancelableRequest(ctx context.Context, req *logical.Request // If we receive an error from CheckSSCToken, we can assume the token is bad somehow, and the client // should receive a 403 bad token error like they do for all other invalid tokens. if err != nil { - return logical.ErrorResponse(fmt.Sprintf("bad token: %s", err.Error())), logical.ErrPermissionDenied + c.Logger().Error("bad token", "error", err) + return logical.ErrorResponse("bad token"), logical.ErrPermissionDenied } req.Data["token"] = token } From e207e3250e9822afea7b06aaf348d67510f9f11c Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 22 Jun 2022 13:10:55 -0700 Subject: [PATCH 3/5] remove the logging --- vault/request_handling.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/vault/request_handling.go b/vault/request_handling.go index 4fddec9864bd..4c75efe0e002 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -10,13 +10,9 @@ import ( "strings" "time" - "github.com/armon/go-metrics" "github.com/golang/protobuf/proto" "github.com/hashicorp/errwrap" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-secure-stdlib/strutil" - "github.com/hashicorp/go-sockaddr" - "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/identity/mfa" "github.com/hashicorp/vault/helper/metricsutil" @@ -591,7 +587,6 @@ func (c *Core) handleCancelableRequest(ctx context.Context, req *logical.Request // If we receive an error from CheckSSCToken, we can assume the token is bad somehow, and the client // should receive a 403 bad token error like they do for all other invalid tokens. if err != nil { - c.Logger().Error("bad token", "error", err) return logical.ErrorResponse("bad token"), logical.ErrPermissionDenied } req.Data["token"] = token From 3c15fd0fe33188913a85f0e34ddbd1b9a8c8ed2f Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 22 Jun 2022 13:12:42 -0700 Subject: [PATCH 4/5] correct weird thing where my IDE removed some imports wth --- vault/request_handling.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vault/request_handling.go b/vault/request_handling.go index 4c75efe0e002..b40bde28e47a 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -10,9 +10,13 @@ import ( "strings" "time" + "github.com/armon/go-metrics" "github.com/golang/protobuf/proto" "github.com/hashicorp/errwrap" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-secure-stdlib/strutil" + "github.com/hashicorp/go-sockaddr" + "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/identity/mfa" "github.com/hashicorp/vault/helper/metricsutil" From 8849c6d26996ba90cd934e746688ebabf93ec113 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Wed, 22 Jun 2022 17:30:01 -0700 Subject: [PATCH 5/5] add changelog --- changelog/16112.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/16112.txt diff --git a/changelog/16112.txt b/changelog/16112.txt new file mode 100644 index 000000000000..3b61c6b89e3d --- /dev/null +++ b/changelog/16112.txt @@ -0,0 +1,3 @@ +```release-note:bug +core/auth: Return a 403 instead of a 500 for a malformed SSCT +```