Skip to content

fix: add the proxy-protocol option#33276

Merged
mabartos merged 2 commits intokeycloak:mainfrom
shawkins:iss10492
Sep 27, 2024
Merged

fix: add the proxy-protocol option#33276
mabartos merged 2 commits intokeycloak:mainfrom
shawkins:iss10492

Conversation

@shawkins
Copy link
Copy Markdown
Contributor

@shawkins shawkins commented Sep 25, 2024 •
edited
Loading

closes: #10492

cc @keycloak/cloud-native-maintainers

Does an integration test for this make sense? The quarkus pr did not include a high level test either.

The quarkus name is use-proxy-protocol, but we generally want our proxy options to all start with proxy, so for us it's just proxy-protocol - but that does read oddly as a boolean. However proxy-use-proxy-protocol seems too long.

We potentially could be opinionated and set this to true when https is enabled and proxy-headers has no value - but we'd to understand if there are any risks / performance overhead before doing that.

@vmuzikar do you want to consider this for v26?

This was referenced Sep 25, 2024
Closed
Closed
Closed
Closed
keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Sep 25, 2024
View reviewed changes
Copy link
Copy Markdown

@keycloak-github-bot keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link
Copy Markdown

keycloak-github-bot Bot commented Sep 25, 2024

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessResidentKeyRegTest#residentKeyRequiredCorrect

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=26357eb9-518e-41d0-9e72-634fdd0b28cf&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

@shawkins shawkins marked this pull request as ready for review September 27, 2024 13:36
@shawkins shawkins requested review from a team as code owners September 27, 2024 13:36
@shawkins
Copy link
Copy Markdown
Contributor Author

shawkins commented Sep 27, 2024

@vmuzikar updated to proxy-protocol-enabled

This was referenced Sep 27, 2024
Closed
Closed
@keycloak-github-bot
Copy link
Copy Markdown

keycloak-github-bot Bot commented Sep 27, 2024

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.passwordless.WebAuthnPwdLessPropertyTest#changeAuthenticatorProperties

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=1b5fc5bf-198f-4e3b-aebc-213fe9b29994&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

org.keycloak.testsuite.webauthn.registration.PubKeySignRegisterTest#publicKeySignaturesEmpty

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=b880bba8-dbd2-4369-9f52-d836bd727e73&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessPubKeySignRegTest#publicKeySignaturesNonExisting

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=674d243b-ba5e-4092-bd78-09c8d367f994&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Sep 27, 2024
View reviewed changes
Copy link
Copy Markdown

@keycloak-github-bot keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

vmuzikar
vmuzikar previously approved these changes Sep 27, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@vmuzikar vmuzikar enabled auto-merge (squash) September 27, 2024 15:15
mabartos
mabartos reviewed Sep 27, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mabartos mabartos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shawkins Just a little thing

Comment thread docs/guides/server/reverseproxy.adoc Outdated
@mabartos mabartos disabled auto-merge September 27, 2024 15:40
This was referenced Sep 27, 2024
Closed
Closed
Closed
@shawkins @mabartos
Update docs/guides/server/reverseproxy.adoc
35c8907 
Co-authored-by: Martin Bartoš <[email protected]>
Signed-off-by: Steven Hawkins <[email protected]>
@mabartos mabartos enabled auto-merge (squash) September 27, 2024 16:25
This was referenced Sep 27, 2024
Closed
Closed
@keycloak-github-bot
Copy link
Copy Markdown

keycloak-github-bot Bot commented Sep 27, 2024

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessPubKeySignRegTest#publicKeySignaturesNonExisting

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=8dffc501-d852-4de5-a843-16825039feb5&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Sep 27, 2024
View reviewed changes
Copy link
Copy Markdown

@keycloak-github-bot keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@mabartos mabartos disabled auto-merge September 27, 2024 17:04
@mabartos mabartos enabled auto-merge (squash) September 27, 2024 17:04
@mabartos mabartos merged commit cf2ecf8 into keycloak:main Sep 27, 2024
@shawkins shawkins mentioned this pull request Oct 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar vmuzikar approved these changes

@mabartos mabartos mabartos approved these changes

+1 more reviewer

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support proxy_protocol

3 participants

@shawkins @vmuzikar @mabartos