Skip to content

Iss11579 hostname strict https contexts fix#11823

Closed
DGuhr wants to merge 2 commits intokeycloak:mainfrom
DGuhr:iss11579_hostname_strict_https_contexts_fix
Closed

Iss11579 hostname strict https contexts fix#11823
DGuhr wants to merge 2 commits intokeycloak:mainfrom
DGuhr:iss11579_hostname_strict_https_contexts_fix

Conversation

@DGuhr
Copy link
Copy Markdown
Contributor

@DGuhr DGuhr commented May 4, 2022 •
edited
Loading

UPDATE: to get a cross-link (thought that was done automatically): This PR is on hold atm. See the discussion here: #11856

This Draft PR should finally get rid of all the HOSTNAME_STRICT_HTTPS issues.

As said e.g. in #11579, the aforementioned config property was never meant to be visible to users. But as a matter of fact we messed up and it is now needed for many environments to actually work the way it is expected.

I would also love to get some feedback if this configuration would work for ppl without setting hostname-strict-https=false manually.

While I work on a way to automate this feedback, I uploaded an image containing these changes to quay.io, so feel free to try it out and provide feedback.

  • Link: here
  • use e.g. docker pull quay.io/dguhr/keycloak:test.

This PR removes the need to set this property completely, it is "moved under the hood" depending on the rules in the following table:

Assumption: Keycloak gets started in production mode with http enabled. So invoked using: kc.sh start --http-enabled=true followed by the configuration values you see in the table.

Proxy Existing HTTPS Setup HOSTNAME-STRICT-HTTPS gets set to:  Result Comment
none no Throw Error Startup Error Without a proxy and without TLS setup, this is not a valid production setup
none yes FALSE + Add Warning at startup Start + Warning Mostly for development purposes, "start" is often also used for development right now, but at least a HTTPS setup should be there.
edge no FALSE Startup normally Valid config, proxy terminates TLS, Keycloak only gets http requests is totally fine.
edge yes FALSE Startup normally Valid config, proxy terminates TLS, Keycloak only gets http requests is totally fine.
passthrough no Throw Error Startup error Passthrough indicates an existing HTTPS Setup has to be configured.
passthrough yes TRUE Startup normally Normal startup, Keycloak only reachable using HTTPS.
reencrypt no Throw Error Startup Error reencrypt also indicates an existing HTTPS setup has to be configured.
reencrypt yes TRUE Startup normally Normal startup, Keycloak only reachable using HTTPS.

@DGuhr DGuhr force-pushed the iss11579_hostname_strict_https_contexts_fix branch from 7ee66e4 to 2fdd1cc Compare May 4, 2022 09:40
DGuhr
DGuhr commented May 4, 2022
View reviewed changes
Comment thread quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/HostnameDistTest.java
}

@Test
@Launch({ "start-dev", "--hostname=mykeycloak.127.0.0.1.nip.io", "--proxy=edge", "--hostname-strict-https=true" })
Copy link
Copy Markdown
Contributor Author

@DGuhr DGuhr May 4, 2022

Choose a reason for hiding this comment

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

this test made no sense anymore, bc strict https and proxy=edge is not valid.

@tsaarni
Copy link
Copy Markdown
Contributor

tsaarni commented May 6, 2022 •
edited
Loading

The image quay.io/dguhr/keycloak:test works well for me. I tested Keycloak behind Contour proxy with HTTP internally, and now admin console works (unlike previously) regardless if HTTP or HTTPS was configured externally at the proxy. I've only set KC_PROXY to edge and KC_HOSTNAME to the external hostname.

EDIT: I now also tested with HTTPS internally between Keycloak and proxy. Setting KC_PROXY to edge allows both HTTP/HTTPS externally between browser and proxy, while setting reencrypt worked only when HTTPS was used externally - otherwise I will get white page instead of admin console. These setting makes sense to me, though I guess it might be bit unusual that backend service behind a proxy has so strict controls.

@DGuhr DGuhr added the status/hold PR should not be merged. On hold for later. label May 6, 2022
@DGuhr
Copy link
Copy Markdown
Contributor Author

DGuhr commented May 6, 2022 •
edited
Loading

to get a cross-link (thought that was done automatically): This PR is on hold atm. See the discussion here: #11856

@Wolfsrudel Wolfsrudel mentioned this pull request May 10, 2022
Closed
@DGuhr
Copy link
Copy Markdown
Contributor Author

DGuhr commented May 16, 2022

closed, superseeded by #11872

@DGuhr DGuhr closed this May 16, 2022
@pedroigor pedroigor self-assigned this Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pedroigor pedroigor Awaiting requested review from pedroigor

@andreaTP andreaTP Awaiting requested review from andreaTP

@vmuzikar vmuzikar Awaiting requested review from vmuzikar

@stianst stianst Awaiting requested review from stianst

Assignees

@pedroigor pedroigor

Labels

status/hold PR should not be merged. On hold for later.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DGuhr @tsaarni @pedroigor