fix: oidc authentication endpoint was overwritten by discovered value (envoyproxy#7460)

fix: oid authentication endpoint was overriden by discovered value

Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: jukie <[email protected]>

Author: zhaohuabing

internal/gatewayapi/securitypolicy.go

@@ -1334,15 +1334,42 @@ func (t *Translator) buildOIDCProvider(policy *egv1a1.SecurityPolicy, resources
 	// Discover the token and authorization endpoints from the issuer's well-known url if not explicitly specified.
 	// EG assumes that the issuer url uses the same protocol and CA as the token endpoint.
 	// If we need to support different protocols or CAs, we need to add more fields to the OIDCProvider CRD.
-	if provider.TokenEndpoint == nil || provider.AuthorizationEndpoint == nil {
+	var (
+		userProvidedAuthorizationEndpoint = ptr.Deref(provider.AuthorizationEndpoint, "")
+		userProvidedTokenEndpoint         = ptr.Deref(provider.TokenEndpoint, "")
+		userProvidedEndSessionEndpoint    = ptr.Deref(provider.EndSessionEndpoint, "")
+	)
+
+	// Authorization endpoint and token endpoint are required fields.
+	// If either of them is not provided, we need to fetch them from the issuer's well-known url.
+	if userProvidedAuthorizationEndpoint == "" || userProvidedTokenEndpoint == "" {
+		// Fetch the endpoints from the issuer's well-known url.
 		discoveredConfig, err := fetchEndpointsFromIssuer(provider.Issuer, providerTLS)
 		if err != nil {
 			return nil, fmt.Errorf("error fetching endpoints from issuer: %w", err)
 		}
-		tokenEndpoint = discoveredConfig.TokenEndpoint
-		authorizationEndpoint = discoveredConfig.AuthorizationEndpoint
-		// endSessionEndpoint is optional, and we prioritize using the one provided in the well-known configuration.
-		if discoveredConfig.EndSessionEndpoint != nil && *discoveredConfig.EndSessionEndpoint != "" {
+
+		// Prioritize using the explicitly provided authorization endpoints if available.
+		// This allows users to add extra parameters to the authorization endpoint if needed.
+		if userProvidedAuthorizationEndpoint != "" {
+			authorizationEndpoint = userProvidedAuthorizationEndpoint
+		} else {
+			authorizationEndpoint = discoveredConfig.AuthorizationEndpoint
+		}
+
+		// Prioritize using the explicitly provided token endpoints if available.
+		// This may not be necessary, but we do it for consistency with authorization endpoint.
+		if userProvidedTokenEndpoint != "" {
+			tokenEndpoint = userProvidedTokenEndpoint
+		} else {
+			tokenEndpoint = discoveredConfig.TokenEndpoint
+		}
+
+		// Prioritize using the explicitly provided end session endpoints if available.
+		// This may not be necessary, but we do it for consistency with other endpoints.
+		if userProvidedEndSessionEndpoint != "" {
+			endSessionEndpoint = &userProvidedEndSessionEndpoint
+		} else {
 			endSessionEndpoint = discoveredConfig.EndSessionEndpoint
 		}
 	} else {

internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml

@@ -14,6 +14,13 @@ secrets:
   data:
     client-secret: Y2xpZW50MTpzZWNyZXQK
     client-id: Y2xpZW50Mi5vYXV0aC5mb28uY29t
+- apiVersion: v1
+  kind: Secret
+  metadata:
+    namespace: default
+    name: client3-secret
+  data:
+    client-secret: Y2xpZW50MTpzZWNyZXQK
 - apiVersion: v1
   kind: Secret
   metadata:
@@ -75,6 +82,25 @@ httpRoutes:
       backendRefs:
       - name: service-1
         port: 8080
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: HTTPRoute
+  metadata:
+    namespace: default
+    name: httproute-3
+  spec:
+    hostnames:
+    - www.example.com
+    parentRefs:
+    - namespace: envoy-gateway
+      name: gateway-1
+      sectionName: http
+    rules:
+    - matches:
+      - path:
+          value: "/baz"
+      backendRefs:
+      - name: service-1
+        port: 8080
 securityPolicies:
 - apiVersion: gateway.envoyproxy.io/v1alpha1
   kind: SecurityPolicy
@@ -128,3 +154,26 @@ securityPolicies:
       refreshToken: true
       defaultRefreshTokenTTL: 48h
       cookieDomain: "example.com"
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: SecurityPolicy
+  metadata:
+    namespace: default
+    name: policy-for-http-route-3
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: HTTPRoute
+      name: httproute-3
+    oidc:
+      provider:
+        issuer: "https://accounts.google.com"
+        authorizationEndpoint: "https://accounts.google.com/o/oauth2/v2/auth?foo=bar"       # custom auth endpoint with query params, should be used as is
+      clientID: "client1.apps.googleusercontent.com"
+      clientSecret:
+        name: "client3-secret"
+      redirectURL: "https://www.example.com/bar/oauth2/callback"
+      logoutPath: "/bar/logout"
+      forwardAccessToken: true
+      defaultTokenTTL: 30m
+      refreshToken: true
+      defaultRefreshTokenTTL: 24h

internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml

@@ -16,7 +16,7 @@ gateways:
       protocol: HTTP
   status:
     listeners:
-    - attachedRoutes: 2
+    - attachedRoutes: 3
       conditions:
       - lastTransitionTime: null
         message: Sending translated listener configuration to the data plane
@@ -116,6 +116,44 @@ httpRoutes:
         name: gateway-1
         namespace: envoy-gateway
         sectionName: http
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: HTTPRoute
+  metadata:
+    creationTimestamp: null
+    name: httproute-3
+    namespace: default
+  spec:
+    hostnames:
+    - www.example.com
+    parentRefs:
+    - name: gateway-1
+      namespace: envoy-gateway
+      sectionName: http
+    rules:
+    - backendRefs:
+      - name: service-1
+        port: 8080
+      matches:
+      - path:
+          value: /baz
+  status:
+    parents:
+    - conditions:
+      - lastTransitionTime: null
+        message: Route is accepted
+        reason: Accepted
+        status: "True"
+        type: Accepted
+      - lastTransitionTime: null
+        message: Resolved all the Object references for the Route
+        reason: ResolvedRefs
+        status: "True"
+        type: ResolvedRefs
+      controllerName: gateway.envoyproxy.io/gatewayclass-controller
+      parentRef:
+        name: gateway-1
+        namespace: envoy-gateway
+        sectionName: http
 infraIR:
   envoy-gateway/gateway-1:
     proxy:
@@ -190,6 +228,47 @@ securityPolicies:
         status: "True"
         type: Accepted
       controllerName: gateway.envoyproxy.io/gatewayclass-controller
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: SecurityPolicy
+  metadata:
+    creationTimestamp: null
+    name: policy-for-http-route-3
+    namespace: default
+  spec:
+    oidc:
+      clientID: client1.apps.googleusercontent.com
+      clientSecret:
+        group: null
+        kind: null
+        name: client3-secret
+      defaultRefreshTokenTTL: 24h
+      defaultTokenTTL: 30m
+      forwardAccessToken: true
+      logoutPath: /bar/logout
+      provider:
+        authorizationEndpoint: https://accounts.google.com/o/oauth2/v2/auth?foo=bar
+        issuer: https://accounts.google.com
+      redirectURL: https://www.example.com/bar/oauth2/callback
+      refreshToken: true
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: HTTPRoute
+      name: httproute-3
+  status:
+    ancestors:
+    - ancestorRef:
+        group: gateway.networking.k8s.io
+        kind: Gateway
+        name: gateway-1
+        namespace: envoy-gateway
+        sectionName: http
+      conditions:
+      - lastTransitionTime: null
+        message: Policy has been accepted.
+        reason: Accepted
+        status: "True"
+        type: Accepted
+      controllerName: gateway.envoyproxy.io/gatewayclass-controller
 - apiVersion: gateway.envoyproxy.io/v1alpha1
   kind: SecurityPolicy
   metadata:
@@ -231,7 +310,7 @@ securityPolicies:
         type: Accepted
       - lastTransitionTime: null
         message: 'This policy is being overridden by other securityPolicies for these
-          routes: [default/httproute-1]'
+          routes: [default/httproute-1 default/httproute-3]'
         reason: Overridden
         status: "True"
         type: Overridden
@@ -374,6 +453,54 @@ xdsIR:
             refreshToken: true
             scopes:
             - openid
+      - destination:
+          metadata:
+            kind: HTTPRoute
+            name: httproute-3
+            namespace: default
+          name: httproute/default/httproute-3/rule/0
+          settings:
+          - addressType: IP
+            endpoints:
+            - host: 7.7.7.7
+              port: 8080
+            metadata:
+              name: service-1
+              namespace: default
+              sectionName: "8080"
+            name: httproute/default/httproute-3/rule/0/backend/0
+            protocol: HTTP
+            weight: 1
+        hostname: www.example.com
+        isHTTP2: false
+        metadata:
+          kind: HTTPRoute
+          name: httproute-3
+          namespace: default
+        name: httproute/default/httproute-3/rule/0/match/0/www_example_com
+        pathMatch:
+          distinct: false
+          name: ""
+          prefix: /baz
+        security:
+          oidc:
+            clientID: client1.apps.googleusercontent.com
+            clientSecret: '[redacted]'
+            cookieSuffix: 811c9dc5
+            defaultRefreshTokenTTL: 24h0m0s
+            defaultTokenTTL: 30m0s
+            forwardAccessToken: true
+            hmacSecret: '[redacted]'
+            logoutPath: /bar/logout
+            name: securitypolicy/default/policy-for-http-route-3
+            provider:
+              authorizationEndpoint: https://accounts.google.com/o/oauth2/v2/auth?foo=bar
+              tokenEndpoint: https://oauth2.googleapis.com/token
+            redirectPath: /bar/oauth2/callback
+            redirectURL: https://www.example.com/bar/oauth2/callback
+            refreshToken: true
+            scopes:
+            - openid
     readyListener:
       address: 0.0.0.0
       ipFamily: IPv4