From ed143c5678ac4180352e8b0284f70c81ece70730 Mon Sep 17 00:00:00 2001
From: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
Date: Mon, 8 Aug 2022 09:02:18 -0700
Subject: [PATCH] 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
---
 changelog/16601.txt                   |  4 ++
 vault/identity_store_oidc_provider.go | 77 +++++++++++++--------------
 2 files changed, 42 insertions(+), 39 deletions(-)
 create mode 100644 changelog/16601.txt

diff --git a/changelog/16601.txt b/changelog/16601.txt
new file mode 100644
index 000000000..ce0e77b9f
--- /dev/null
+++ 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
index 319638f33..a8320c29c 100644
--- 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())