identity/oidc: reorder authorization endpoint validation for invalid …

…redirect uris (#16601) * identity/oidc: reorder authorization endpoint validation for invalid redirect uris * adds changelog * use provider.allowedClientID
hashicorp · Aug 8, 2022 · 4afd49b · 4afd49b
1 parent 7d0f0b2
commit 4afd49b
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 39 deletions.
diff --git a/changelog/16601.txt b/changelog/16601.txt
@@ -0,0 +1,4 @@
+```release-note:bug
+identity/oidc: Detect invalid `redirect_uri` values sooner in validation of the 
+Authorization Endpoint.
+```
diff --git a/vault/identity_store_oidc_provider.go b/vault/identity_store_oidc_provider.go
@@ -1617,11 +1617,27 @@ func (i *IdentityStore) keyIDsReferencedByTargetClientIDs(ctx context.Context, s
 func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
 	state := d.Get("state").(string)
 
-	// Get the namespace
-	ns, err := namespace.FromContext(ctx)
+	// Validate the client ID
+	clientID := d.Get("client_id").(string)
+	if clientID == "" {
+		return authResponse("", state, ErrAuthInvalidClientID, "client_id parameter is required")
+	}
+	client, err := i.clientByID(ctx, req.Storage, clientID)
 	if err != nil {
 		return authResponse("", state, ErrAuthServerError, err.Error())
 	}
+	if client == nil {
+		return authResponse("", state, ErrAuthInvalidClientID, "client with client_id not found")
+	}
+
+	// Validate the redirect URI
+	redirectURI := d.Get("redirect_uri").(string)
+	if redirectURI == "" {
+		return authResponse("", state, ErrAuthInvalidRequest, "redirect_uri parameter is required")
+	}
+	if !validRedirect(redirectURI, client.RedirectURIs) {
+		return authResponse("", state, ErrAuthInvalidRedirectURI, "redirect_uri is not allowed for the client")
+	}
 
 	// Get the OIDC provider
 	name := d.Get("name").(string)
@@ -1632,6 +1648,20 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ
 	if provider == nil {
 		return authResponse("", state, ErrAuthInvalidRequest, "provider not found")
 	}
+	if !provider.allowedClientID(clientID) {
+		return authResponse("", state, ErrAuthUnauthorizedClient, "client is not authorized to use the provider")
+	}
+
+	// We don't support the request or request_uri parameters. If they're provided,
+	// the appropriate errors must be returned. For details, see the spec at:
+	// https://openid.net/specs/openid-connect-core-1_0.html#RequestObject
+	// https://openid.net/specs/openid-connect-core-1_0.html#RequestUriParameter
+	if _, ok := d.Raw["request"]; ok {
+		return authResponse("", "", ErrAuthRequestNotSupported, "request parameter is not supported")
+	}
+	if _, ok := d.Raw["request_uri"]; ok {
+		return authResponse("", "", ErrAuthRequestURINotSupported, "request_uri parameter is not supported")
+	}
 
 	// Validate that a scope parameter is present and contains the openid scope value
 	requestedScopes := strutil.ParseDedupAndSortStrings(d.Get("scope").(string), scopesDelimiter)
@@ -1657,43 +1687,6 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ
 		return authResponse("", state, ErrAuthUnsupportedResponseType, "unsupported response_type value")
 	}
 
-	// Validate the client ID
-	clientID := d.Get("client_id").(string)
-	if clientID == "" {
-		return authResponse("", state, ErrAuthInvalidClientID, "client_id parameter is required")
-	}
-	client, err := i.clientByID(ctx, req.Storage, clientID)
-	if err != nil {
-		return authResponse("", state, ErrAuthServerError, err.Error())
-	}
-	if client == nil {
-		return authResponse("", state, ErrAuthInvalidClientID, "client with client_id not found")
-	}
-	if !provider.allowedClientID(clientID) {
-		return authResponse("", state, ErrAuthUnauthorizedClient, "client is not authorized to use the provider")
-	}
-
-	// Validate the redirect URI
-	redirectURI := d.Get("redirect_uri").(string)
-	if redirectURI == "" {
-		return authResponse("", state, ErrAuthInvalidRequest, "redirect_uri parameter is required")
-	}
-
-	if !validRedirect(redirectURI, client.RedirectURIs) {
-		return authResponse("", state, ErrAuthInvalidRedirectURI, "redirect_uri is not allowed for the client")
-	}
-
-	// We don't support the request or request_uri parameters. If they're provided,
-	// the appropriate errors must be returned. For details, see the spec at:
-	// https://openid.net/specs/openid-connect-core-1_0.html#RequestObject
-	// https://openid.net/specs/openid-connect-core-1_0.html#RequestUriParameter
-	if _, ok := d.Raw["request"]; ok {
-		return authResponse("", state, ErrAuthRequestNotSupported, "request parameter is not supported")
-	}
-	if _, ok := d.Raw["request_uri"]; ok {
-		return authResponse("", state, ErrAuthRequestURINotSupported, "request_uri parameter is not supported")
-	}
-
 	// Validate that there is an identity entity associated with the request
 	if req.EntityID == "" {
 		return authResponse("", state, ErrAuthAccessDenied, "identity entity must be associated with the request")
@@ -1797,6 +1790,12 @@ func (i *IdentityStore) pathOIDCAuthorize(ctx context.Context, req *logical.Requ
 		return authResponse("", state, ErrAuthServerError, err.Error())
 	}
 
+	// Get the namespace
+	ns, err := namespace.FromContext(ctx)
+	if err != nil {
+		return authResponse("", state, ErrAuthServerError, err.Error())
+	}
+
 	// Cache the authorization code for a subsequent token exchange
 	if err := i.oidcAuthCodeCache.SetDefault(ns, code, authCodeEntry); err != nil {
 		return authResponse("", state, ErrAuthServerError, err.Error())