From 4c12c2bb42a58c93c733a3c96e5aa0ea93e721b5 Mon Sep 17 00:00:00 2001 From: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> Date: Mon, 7 Feb 2022 12:50:36 -0800 Subject: [PATCH] identity/oidc: adds tests for validation of loopback IP redirect URIs (#13939) * identity/oidc: adds tests for validation of loopback IP redirect URIs * Update vault/identity_store_oidc_provider_test.go Co-authored-by: John-Michael Faircloth Co-authored-by: John-Michael Faircloth --- vault/identity_store_oidc_provider.go | 36 ----------- vault/identity_store_oidc_provider_test.go | 72 ++++++++++++++++++++++ vault/identity_store_oidc_provider_util.go | 39 ++++++++++++ 3 files changed, 111 insertions(+), 36 deletions(-) diff --git a/vault/identity_store_oidc_provider.go b/vault/identity_store_oidc_provider.go index e3580a9e1..e2e911d93 100644 --- a/vault/identity_store_oidc_provider.go +++ b/vault/identity_store_oidc_provider.go @@ -2,14 +2,11 @@ package vault import ( "context" - "crypto/sha256" - "crypto/sha512" "crypto/subtle" "encoding/base64" "encoding/json" "errors" "fmt" - "hash" "net/http" "net/url" "sort" @@ -2159,39 +2156,6 @@ func (i *IdentityStore) populateScopeTemplates(ctx context.Context, s logical.St return populatedTemplates, false, nil } -// computeHashClaim computes the hash value to be used for the at_hash -// and c_hash claims. For details on how this value is computed and the -// class of attacks it's used to prevent, see the spec at -// - https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken -// - https://openid.net/specs/openid-connect-core-1_0.html#HybridIDToken -// - https://openid.net/specs/openid-connect-core-1_0.html#TokenSubstitution -func computeHashClaim(alg string, input string) (string, error) { - signatureAlgToHash := map[jose.SignatureAlgorithm]func() hash.Hash{ - jose.RS256: sha256.New, - jose.RS384: sha512.New384, - jose.RS512: sha512.New, - jose.ES256: sha256.New, - jose.ES384: sha512.New384, - jose.ES512: sha512.New, - - // We use the Ed25519 curve key for EdDSA, which uses - // SHA-512 for its digest algorithm. See details at - // https://bitbucket.org/openid/connect/issues/1125. - jose.EdDSA: sha512.New, - } - - newHash, ok := signatureAlgToHash[jose.SignatureAlgorithm(alg)] - if !ok { - return "", fmt.Errorf("unsupported signature algorithm: %q", alg) - } - h := newHash() - - // Writing to the hash will never return an error - _, _ = h.Write([]byte(input)) - sum := h.Sum(nil) - return base64.RawURLEncoding.EncodeToString(sum[:len(sum)/2]), nil -} - // entityHasAssignment returns true if the entity is enabled and a member of any // of the assignments' groups or entities. Otherwise, returns false or an error. func (i *IdentityStore) entityHasAssignment(ctx context.Context, s logical.Storage, entity *identity.Entity, assignments []string) (bool, error) { diff --git a/vault/identity_store_oidc_provider_test.go b/vault/identity_store_oidc_provider_test.go index e279b2143..af18c2792 100644 --- a/vault/identity_store_oidc_provider_test.go +++ b/vault/identity_store_oidc_provider_test.go @@ -805,6 +805,78 @@ func TestOIDC_Path_OIDC_Authorize(t *testing.T) { authorizeReq: testAuthorizeReq(s, clientID), }, }, + { + name: "valid authorize request with port-agnostic loopback redirect_uri 127.0.0.1", + args: args{ + entityID: entityID, + clientReq: func() *logical.Request { + req := testClientReq(s) + req.Data["redirect_uris"] = []string{"http://127.0.0.1/callback"} + return req + }(), + providerReq: testProviderReq(s, clientID), + assignmentReq: testAssignmentReq(s, entityID, groupID), + authorizeReq: func() *logical.Request { + req := testAuthorizeReq(s, clientID) + req.Data["redirect_uri"] = "http://127.0.0.1:51004/callback" + return req + }(), + }, + }, + { + name: "valid authorize request with port-agnostic loopback redirect_uri 127.0.0.1 with port", + args: args{ + entityID: entityID, + clientReq: func() *logical.Request { + req := testClientReq(s) + req.Data["redirect_uris"] = []string{"http://127.0.0.1:8251/callback"} + return req + }(), + providerReq: testProviderReq(s, clientID), + assignmentReq: testAssignmentReq(s, entityID, groupID), + authorizeReq: func() *logical.Request { + req := testAuthorizeReq(s, clientID) + req.Data["redirect_uri"] = "http://127.0.0.1:51005/callback" + return req + }(), + }, + }, + { + name: "valid authorize request with port-agnostic loopback redirect_uri localhost", + args: args{ + entityID: entityID, + clientReq: func() *logical.Request { + req := testClientReq(s) + req.Data["redirect_uris"] = []string{"http://localhost:8251/callback"} + return req + }(), + providerReq: testProviderReq(s, clientID), + assignmentReq: testAssignmentReq(s, entityID, groupID), + authorizeReq: func() *logical.Request { + req := testAuthorizeReq(s, clientID) + req.Data["redirect_uri"] = "http://localhost:51006/callback" + return req + }(), + }, + }, + { + name: "valid authorize request with port-agnostic loopback redirect_uri [::1]", + args: args{ + entityID: entityID, + clientReq: func() *logical.Request { + req := testClientReq(s) + req.Data["redirect_uris"] = []string{"http://[::1]:8251/callback"} + return req + }(), + providerReq: testProviderReq(s, clientID), + assignmentReq: testAssignmentReq(s, entityID, groupID), + authorizeReq: func() *logical.Request { + req := testAuthorizeReq(s, clientID) + req.Data["redirect_uri"] = "http://[::1]:51007/callback" + return req + }(), + }, + }, } for _, tt := range tests { diff --git a/vault/identity_store_oidc_provider_util.go b/vault/identity_store_oidc_provider_util.go index f5b0dadb8..76819d813 100644 --- a/vault/identity_store_oidc_provider_util.go +++ b/vault/identity_store_oidc_provider_util.go @@ -1,9 +1,15 @@ package vault import ( + "crypto/sha256" + "crypto/sha512" + "encoding/base64" + "fmt" + "hash" "net/url" "github.com/hashicorp/go-secure-stdlib/strutil" + "gopkg.in/square/go-jose.v2" ) // validRedirect checks whether uri is in allowed using special handling for loopback uris. @@ -36,3 +42,36 @@ func validRedirect(uri string, allowed []string) bool { return false } + +// computeHashClaim computes the hash value to be used for the at_hash +// and c_hash claims. For details on how this value is computed and the +// class of attacks it's used to prevent, see the spec at +// - https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken +// - https://openid.net/specs/openid-connect-core-1_0.html#HybridIDToken +// - https://openid.net/specs/openid-connect-core-1_0.html#TokenSubstitution +func computeHashClaim(alg string, input string) (string, error) { + signatureAlgToHash := map[jose.SignatureAlgorithm]func() hash.Hash{ + jose.RS256: sha256.New, + jose.RS384: sha512.New384, + jose.RS512: sha512.New, + jose.ES256: sha256.New, + jose.ES384: sha512.New384, + jose.ES512: sha512.New, + + // We use the Ed25519 curve key for EdDSA, which uses + // SHA-512 for its digest algorithm. See details at + // https://bitbucket.org/openid/connect/issues/1125. + jose.EdDSA: sha512.New, + } + + newHash, ok := signatureAlgToHash[jose.SignatureAlgorithm(alg)] + if !ok { + return "", fmt.Errorf("unsupported signature algorithm: %q", alg) + } + h := newHash() + + // Writing to the hash will never return an error + _, _ = h.Write([]byte(input)) + sum := h.Sum(nil) + return base64.RawURLEncoding.EncodeToString(sum[:len(sum)/2]), nil +}