fix: use tls config from BTP when connecting to the OIDC provider's well-known endpoint. #4857

zhaohuabing · 2024-12-06T14:11:02Z

Fixes #4838

Release Notes: Yes

Signed-off-by: Huabing Zhao <[email protected]>

test/e2e/tests/oidc_testclient.go

Signed-off-by: Huabing Zhao <[email protected]>

codecov · 2024-12-10T06:39:24Z

Codecov Report

Attention: Patch coverage is 40.00000% with 51 lines in your changes missing coverage. Please review.

Project coverage is 66.72%. Comparing base (2a5ecaf) to head (f4fbc4a).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/ir/xds.go	0.00%	37 Missing ⚠️
internal/gatewayapi/securitypolicy.go	70.83%	10 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4857      +/-   ##
==========================================
- Coverage   66.77%   66.72%   -0.05%     
==========================================
  Files         209      209              
  Lines       32100    32162      +62     
==========================================
+ Hits        21434    21460      +26     
- Misses       9379     9416      +37     
+ Partials     1287     1286       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing · 2024-12-10T12:42:44Z

test/e2e/testdata/oidc-securitypolicy-backendcluster.yaml

-      issuer: "http://keycloak.gateway-conformance-infra/realms/master"
-      authorizationEndpoint: "http://keycloak.gateway-conformance-infra/realms/master/protocol/openid-connect/auth"
-      tokenEndpoint: "http://keycloak.gateway-conformance-infra/realms/master/protocol/openid-connect/token"
+      issuer: "https://keycloak.gateway-conformance-infra/realms/master"   # Test fetching auth endpoint from the issuer url


Removing tokenEndpoint and authorizationEndpoint so they will be fetched by the Gateway API translator. This is used to verify that EG uses the CA from BackendTLSPolicy when connecting to the OIDC provider's well-known endpoint.

zhaohuabing · 2024-12-10T12:45:02Z

test/e2e/testdata/oidc-keycloak.yaml

-          image: busybox:stable
-          command: ["sh", "-c", "until nc -v -z -w3 keycloak 80; do sleep 2; done"]
+          image: curlimages/curl:latest
+          command: ["sh", "-c", "until curl -s -o /dev/null -w '%{http_code}' http://keycloak:80; do sleep 2; done"]


Using curl instead of nc to verify that keycload is ready. This change fixed some flaky OIDC tests.

arkodg · 2024-12-10T19:11:53Z

test/e2e/tests/oidc-backendcluster.go

@@ -23,7 +23,7 @@ func init() {
 var OIDCBackendClusterTest = suite.ConformanceTest{
 	ShortName:   "OIDC with BackendCluster",
 	Description: "Test OIDC authentication",
-	Manifests:   []string{"testdata/oidc-keycloak.yaml", "testdata/oidc-securitypolicy-backendcluster.yaml"},
+	Manifests:   []string{"testdata/oidc-keycloak.yaml"},


see test/e2e/testdata/oidc-securitypolicy-backendcluster.yaml being updated but not in use

It's used here:

Test: func(t *testing.T, suite *suite.ConformanceTestSuite) { t.Run("oidc provider represented by a BackendCluster", func(t *testing.T) { testOIDC(t, suite, "testdata/oidc-securitypolicy-backendcluster.yaml")

The creation of SecurityPolicy is now started after keycloak pod is ready, so EG can fetch the auth and token points from the keycloak's well-known endpoint.

Signed-off-by: Huabing Zhao <[email protected]>

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

Signed-off-by: Huabing Zhao <[email protected]>

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

internal/gatewayapi/securitypolicy.go

arkodg

LGTM thanks

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

shawnh2

lgtm

…ell-known endpoint. (envoyproxy#4857) * add e2e test for OIDC provider with TLS Signed-off-by: Huabing Zhao <[email protected]> * delete file Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> * use TLS config from BTLPolicy to fetch auth endpoint Signed-off-by: Huabing Zhao <[email protected]> * refactor Signed-off-by: Huabing Zhao <[email protected]> * update release note Signed-off-by: Huabing Zhao <[email protected]> * update release note Signed-off-by: Huabing Zhao <[email protected]> * fix test Signed-off-by: Huabing Zhao <[email protected]> * fix test Signed-off-by: Huabing Zhao <[email protected]> * fix test Signed-off-by: Huabing Zhao <[email protected]> * fix test Signed-off-by: Huabing Zhao <[email protected]> * fix test Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]> Signed-off-by: Huabing (Robin) Zhao <[email protected]> (cherry picked from commit 3a39c35) Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing requested a review from a team as a code owner December 6, 2024 14:11

zhaohuabing marked this pull request as draft December 6, 2024 14:11

zhaohuabing force-pushed the fix-oidc-issuer-tls branch from ef1e9fe to d9dc4eb Compare December 10, 2024 02:06

add e2e test for OIDC provider with TLS

a5092a4

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing force-pushed the fix-oidc-issuer-tls branch from d9dc4eb to a5092a4 Compare December 10, 2024 05:28

delete file

1e5bdcd

Signed-off-by: Huabing Zhao <[email protected]>

github-advanced-security bot found potential problems Dec 10, 2024

View reviewed changes

test/e2e/tests/oidc_testclient.go Dismissed Show dismissed Hide dismissed

fix lint

e3c0ae9

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing added 2 commits December 10, 2024 07:23

use TLS config from BTLPolicy to fetch auth endpoint

9350110

Signed-off-by: Huabing Zhao <[email protected]>

refactor

26ee93a

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing force-pushed the fix-oidc-issuer-tls branch from 2195b65 to 26ee93a Compare December 10, 2024 08:15

zhaohuabing marked this pull request as ready for review December 10, 2024 08:16

update release note

a13a3c3

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing changed the title ~~fix: use tls config from BTP to fetch oidc provider endpoints~~ fix: use tls config from BTP when connecting to the OIDC provider's well-known endpoint. Dec 10, 2024

zhaohuabing force-pushed the fix-oidc-issuer-tls branch from b62874d to 45aca68 Compare December 10, 2024 08:33

update release note

f82c92b

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing force-pushed the fix-oidc-issuer-tls branch from 45aca68 to f82c92b Compare December 10, 2024 08:34

zhaohuabing marked this pull request as draft December 10, 2024 09:51

zhaohuabing added 3 commits December 10, 2024 10:04

fix test

756ea75

Signed-off-by: Huabing Zhao <[email protected]>

fix test

102c441

Signed-off-by: Huabing Zhao <[email protected]>

fix test

f20b102

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing marked this pull request as ready for review December 10, 2024 11:22

zhaohuabing added 2 commits December 10, 2024 12:23

fix test

6aac528

Signed-off-by: Huabing Zhao <[email protected]>

fix test

bf40e40

Signed-off-by: Huabing Zhao <[email protected]>

zhaohuabing commented Dec 10, 2024

View reviewed changes

arkodg reviewed Dec 10, 2024

View reviewed changes

zhaohuabing requested a review from arkodg December 11, 2024 00:49

Merge branch 'main' into fix-oidc-issuer-tls

e1e6de9

zhaohuabing added 8 commits December 11, 2024 11:27

Merge branch 'main' into fix-oidc-issuer-tls

ebfb042

Signed-off-by: Huabing Zhao <[email protected]>

Merge branch 'main' into fix-oidc-issuer-tls

f5de772

Merge branch 'main' into fix-oidc-issuer-tls

c609c11

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

Merge remote-tracking branch 'origin/main' into fix-oidc-issuer-tls

23277a4

fix lint

88a3e09

Signed-off-by: Huabing Zhao <[email protected]>

Merge branch 'main' into fix-oidc-issuer-tls

68faf2a

Merge branch 'main' into fix-oidc-issuer-tls

350efce

Merge branch 'main' into fix-oidc-issuer-tls

cc64e5d

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

arkodg reviewed Jan 6, 2025

View reviewed changes

internal/gatewayapi/securitypolicy.go Show resolved Hide resolved

arkodg previously approved these changes Jan 6, 2025

View reviewed changes

arkodg requested review from a team January 6, 2025 22:03

zirain previously approved these changes Jan 7, 2025

View reviewed changes

Merge branch 'main' into fix-oidc-issuer-tls

f4fbc4a

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

zhaohuabing dismissed stale reviews from zirain and arkodg via f4fbc4a January 7, 2025 02:13

zirain approved these changes Jan 7, 2025

View reviewed changes

zhaohuabing requested a review from arkodg January 7, 2025 02:15

arkodg approved these changes Jan 7, 2025

View reviewed changes

shawnh2 approved these changes Jan 7, 2025

View reviewed changes

zirain merged commit 3a39c35 into envoyproxy:main Jan 7, 2025
21 of 22 checks passed

arkodg mentioned this pull request Jan 10, 2025

[release/v1.2] cherry pick for v1.2.5 #5029

Open

zhaohuabing deleted the fix-oidc-issuer-tls branch January 13, 2025 01:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use tls config from BTP when connecting to the OIDC provider's well-known endpoint. #4857

fix: use tls config from BTP when connecting to the OIDC provider's well-known endpoint. #4857

zhaohuabing commented Dec 6, 2024

codecov bot commented Dec 10, 2024 •

edited

Loading

zhaohuabing Dec 10, 2024 •

edited

Loading

zhaohuabing Dec 10, 2024 •

edited

Loading

arkodg Dec 10, 2024 •

edited

Loading

zhaohuabing Dec 11, 2024

arkodg left a comment

shawnh2 left a comment

fix: use tls config from BTP when connecting to the OIDC provider's well-known endpoint. #4857

fix: use tls config from BTP when connecting to the OIDC provider's well-known endpoint. #4857

Conversation

zhaohuabing commented Dec 6, 2024

codecov bot commented Dec 10, 2024 • edited Loading

Codecov Report

zhaohuabing Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

arkodg Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing Dec 11, 2024

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

shawnh2 left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2024 •

edited

Loading

zhaohuabing Dec 10, 2024 •

edited

Loading

zhaohuabing Dec 10, 2024 •

edited

Loading

arkodg Dec 10, 2024 •

edited

Loading