add redirectURL and signoutPath to OIDC (#2409)

* add redirectURL and signoutPath to OIDC Signed-off-by: huabing zhao <[email protected]> * address comments Signed-off-by: huabing zhao <[email protected]> * change signoutpath to logoutpath Signed-off-by: huabing zhao <[email protected]> * fix check Signed-off-by: huabing zhao <[email protected]> * modify oidc docs Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Co-authored-by: zirain <[email protected]>
envoyproxy · Jan 12, 2024 · 896d6a6 · 896d6a6
1 parent 1e57665
commit 896d6a6
Show file tree

Hide file tree

Showing 16 changed files with 187 additions and 253 deletions.
diff --git a/api/v1alpha1/oidc_types.go b/api/v1alpha1/oidc_types.go
@@ -36,20 +36,18 @@ type OIDC struct {
 	// specified.
 	// +optional
 	Scopes []string `json:"scopes,omitempty"`
+
+	// The redirect URL to be used in the OIDC
+	// [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
+	// If not specified, uses the default redirect URI "%REQ(x-forwarded-proto)%://%REQ(:authority)%/oauth2/callback"
+	RedirectURL *string `json:"redirectURL,omitempty"`
+
+	// The path to log a user out, clearing their credential cookies.
+	// If not specified, uses a default logout path "/logout"
+	LogoutPath *string `json:"logoutPath,omitempty"`
 }
 
 // OIDCProvider defines the OIDC Provider configuration.
-// To make the EG OIDC config easy to use, some of the low-level ouath2 filter
-// configuration knobs are hidden from the user, and default values will be provided
-// when translating to XDS. For example:
-//
-// * redirect_uri: uses a default redirect URI "%REQ(x-forwarded-proto)%://%REQ(:authority)%/oauth2/callback"
-//
-// * signout_path: uses a default signout path "/signout"
-//
-// * redirect_path_matcher: uses a default redirect path matcher "/oauth2/callback"
-//
-// If we get requests to expose these knobs, we can always do so later.
 type OIDCProvider struct {
 	// The OIDC Provider's [issuer identifier](https://openid.net/specs/openid-connect-discovery-1_0.html#IssuerDiscovery).
 	// Issuer MUST be a URI RFC 3986 [RFC3986] with a scheme component that MUST

diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
diff --git a/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml
@@ -300,6 +300,10 @@ spec:
                     required:
                     - name
                     type: object
+                  logoutPath:
+                    description: The path to log a user out, clearing their credential
+                      cookies. If not specified, uses a default logout path "/logout"
+                    type: string
                   provider:
                     description: The OIDC Provider configuration.
                     properties:
@@ -323,6 +327,11 @@ spec:
                     required:
                     - issuer
                     type: object
+                  redirectURL:
+                    description: The redirect URL to be used in the OIDC [Authentication
+                      Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
+                      If not specified, uses the default redirect URI "%REQ(x-forwarded-proto)%://%REQ(:authority)%/oauth2/callback"
+                    type: string
                   scopes:
                     description: The OIDC scopes to be used in the [Authentication
                       Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).

diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go
@@ -29,6 +29,12 @@ import (
 	"github.com/envoyproxy/gateway/internal/status"
 )
 
+const (
+	defaultRedirectURL  = "%REQ(x-forwarded-proto)%://%REQ(:authority)%/oauth2/callback"
+	defaultRedirectPath = "/oauth2/callback"
+	defaultLogoutPath   = "/logout"
+)
+
 func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.SecurityPolicy,
 	gateways []*GatewayContext,
 	routes []RouteContext,
@@ -447,19 +453,58 @@ func (t *Translator) buildOIDC(
 		return nil, err
 	}
 
-	if err := validateTokenEndpoint(provider.TokenEndpoint); err != nil {
+	if err = validateTokenEndpoint(provider.TokenEndpoint); err != nil {
 		return nil, err
 	}
 	scopes := appendOpenidScopeIfNotExist(oidc.Scopes)
 
+	var (
+		redirectURL  = defaultRedirectURL
+		redirectPath = defaultRedirectPath
+		logoutPath   = defaultLogoutPath
+	)
+
+	if oidc.RedirectURL != nil {
+		path, err := extractRedirectPath(*oidc.RedirectURL)
+		if err != nil {
+			return nil, err
+		}
+		redirectURL = *oidc.RedirectURL
+		redirectPath = path
+		logoutPath = *oidc.LogoutPath
+	}
+
 	return &ir.OIDC{
 		Provider:     *provider,
 		ClientID:     oidc.ClientID,
 		ClientSecret: clientSecretBytes,
 		Scopes:       scopes,
+		RedirectURL:  redirectURL,
+		RedirectPath: redirectPath,
+		LogoutPath:   logoutPath,
 	}, nil
 }
 
+func extractRedirectPath(redirectURL string) (string, error) {
+	schemeDelimiter := strings.Index(redirectURL, "://")
+	if schemeDelimiter <= 0 {
+		return "", fmt.Errorf("invalid redirect URL %s", redirectURL)
+	}
+	scheme := redirectURL[:schemeDelimiter]
+	if scheme != "http" && scheme != "https" && scheme != "%REQ(x-forwarded-proto)%" {
+		return "", fmt.Errorf("invalid redirect URL %s", redirectURL)
+	}
+	hostDelimiter := strings.Index(redirectURL[schemeDelimiter+3:], "/")
+	if hostDelimiter <= 0 {
+		return "", fmt.Errorf("invalid redirect URL %s", redirectURL)
+	}
+	path := redirectURL[schemeDelimiter+3+hostDelimiter:]
+	if path == "/" {
+		return "", fmt.Errorf("invalid redirect URL %s", redirectURL)
+	}
+	return path, nil
+}
+
 // appendOpenidScopeIfNotExist appends the openid scope to the provided scopes
 // if it is not already present.
 // `openid` is a required scope for OIDC.

diff --git a/internal/gatewayapi/securitypolicy_test.go b/internal/gatewayapi/securitypolicy_test.go
@@ -74,3 +74,61 @@ func Test_wildcard2regex(t *testing.T) {
 		})
 	}
 }
+
+func Test_extractRedirectPath(t *testing.T) {
+	tests := []struct {
+		name        string
+		redirectURL string
+		want        string
+		wantErr     bool
+	}{
+		{
+			name:        "header value syntax",
+			redirectURL: "%REQ(x-forwarded-proto)%://%REQ(:authority)%/petstore/oauth2/callback",
+			want:        "/petstore/oauth2/callback",
+			wantErr:     false,
+		},
+		{
+			name:        "without header value syntax",
+			redirectURL: "https://www.example.com/petstore/oauth2/callback",
+			want:        "/petstore/oauth2/callback",
+			wantErr:     false,
+		},
+		{
+			name:        "with port",
+			redirectURL: "https://www.example.com:9080/petstore/oauth2/callback",
+			want:        "/petstore/oauth2/callback",
+			wantErr:     false,
+		},
+		{
+			name:        "without path",
+			redirectURL: "https://www.example.com/",
+			want:        "",
+			wantErr:     true,
+		},
+		{
+			name:        "without path",
+			redirectURL: "https://www.example.com",
+			want:        "",
+			wantErr:     true,
+		},
+		{
+			name:        "without scheme",
+			redirectURL: "://www.example.com",
+			want:        "",
+			wantErr:     true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := extractRedirectPath(tt.redirectURL)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("extractRedirectPath() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if err == nil {
+				assert.Equalf(t, tt.want, got, "extractRedirectPath(%v)", tt.redirectURL)
+			}
+		})
+	}
+}
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml
@@ -43,7 +43,7 @@ httpRoutes:
     name: httproute-1
   spec:
     hostnames:
-    - gateway.envoyproxy.io
+    - www.example.com
     parentRefs:
     - namespace: envoy-gateway
       name: gateway-1
@@ -62,7 +62,7 @@ httpRoutes:
     name: httproute-2
   spec:
     hostnames:
-    - gateway.envoyproxy.io
+    - www.example.com
     parentRefs:
     - namespace: envoy-gateway
       name: gateway-1
@@ -74,21 +74,6 @@ httpRoutes:
       backendRefs:
       - name: service-1
         port: 8080
-grpcRoutes:
-- apiVersion: gateway.networking.k8s.io/v1alpha2
-  kind: GRPCRoute
-  metadata:
-    namespace: default
-    name: grpcroute-1
-  spec:
-    parentRefs:
-    - namespace: envoy-gateway
-      name: gateway-1
-      sectionName: http
-    rules:
-    - backendRefs:
-      - name: service-1
-        port: 8080
 securityPolicies:
 - apiVersion: gateway.envoyproxy.io/v1alpha1
   kind: SecurityPolicy
@@ -107,6 +92,8 @@ securityPolicies:
       clientID: "client1.apps.googleusercontent.com"
       clientSecret:
         name: "client1-secret"
+      redirectURL: "https://www.example.com/bar/oauth2/callback"
+      logoutPath: "/bar/logout"
 - apiVersion: gateway.envoyproxy.io/v1alpha1
   kind: SecurityPolicy
   metadata:
@@ -127,22 +114,5 @@ securityPolicies:
       clientSecret:
         name: "client2-secret"
       scopes: ["openid", "email", "profile"]
-- apiVersion: gateway.envoyproxy.io/v1alpha1
-  kind: SecurityPolicy
-  metadata:
-    namespace: default
-    name: policy-cross-namespace-secretRef  # This policy should attach grpcroute-1
-  spec:
-    targetRef:
-      group: gateway.networking.k8s.io
-      kind: GRPCRoute
-      name: grpcroute-1
-    oidc:
-      provider:
-        issuer: "https://oauth.bar.com"
-        authorizationEndpoint: "https://oauth.bar.com/oauth2/v2/auth"
-        tokenEndpoint: "https://oauth.bar.com/token"
-      clientID: "client3.bar.foo.com"
-      clientSecret:
-        namespace: default
-        name: "client3-secret"
+      redirectURL: "https://www.example.com/foo/oauth2/callback"
+      logoutPath: "/foo/logout"