Repository navigation
Conversation
|
Happy to help if you want. I suppose there will be some failing tests as some Quarkus tests list all options in the help. |
0d5cfe0 to
22c2089
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
22c2089 to
d534594
Compare
|
@vmuzikar You had some concerns on the previous PR (#16747 (review)), let's try to resolve those here.
I think we need to stop supporting this use-case, folks should not be running Keycloak on HTTP unless they are behind a proxy that is serving over HTTPS, in which case this is also not a problem, as this would be considered a secure context. I also don't expect this to be a problem for local development, as long as folks are using a secure context, such as |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Thinking we shouldn't output |
+1 |
|
@pedroigor - replying to your comment:
Unfortunately in cloud environments this all depends on the proxy or if the browser is using a localhost or equivalent. All of this is undetectable by the server by looking at the CLI options only. The only real test is if the client provides the cookies, which might require one more round-trip. |
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
|
I pushed one more commit to set the same attributes on the Once I did that, Keycloak shows once I submit a login form the following message (see screen shot at the end):
@jonkoops - assuming this is the
@jonkoops - if you could supply me with a snippet of JavaScript and a possible text to display in the scenario described above, I can have a look how to pull the text out of the localized message files (haven't done that before, but I suppose I'll figure it out). |
This one in particular should be a first-party cookie all the time, I think. It is supposed to be used within the server domain boundaries and not shared across domains? If browsers are going to default to (That is kinda why I think we should avoid introducing strict in this PR because there are other things to discuss) |
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
|
@jonkoops - pushed one more commit that uses the JavaScript you suggested to have a message like the following:
As it might run in an enterprise environment, it shouldn't require a link to the Internet. |
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
Reading this, I'm now confused why the other cookies have I've now set the |
Same here ... However, an authentication session is used in brokering and there you have cross-origin requests where the user is redirected back to the server to continue with the flow. |
There was a problem hiding this comment.
Added few minor comments to the docs part.
|
|
||
| == `SameSite` attribute set to `Strict` by default | ||
|
|
||
| All cookies provided by Keycloak will now have the `SameSite` attribute set to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#strict[`Strict`] by default. This means that cookies will only be sent in a first-party context, that is, requests originating from the same site that set the cookie. Some specific cookies that are required in a third party contexts, such as the session cookie are exempt from this behavior for now, but this will likely change in the future as using cookies in a third-party context will no longer supported by browsers. |
There was a problem hiding this comment.
| All cookies provided by Keycloak will now have the `SameSite` attribute set to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#strict[`Strict`] by default. This means that cookies will only be sent in a first-party context, that is, requests originating from the same site that set the cookie. Some specific cookies that are required in a third party contexts, such as the session cookie are exempt from this behavior for now, but this will likely change in the future as using cookies in a third-party context will no longer supported by browsers. | |
| All cookies provided by Keycloak will now have the `SameSite` attribute set to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#strict[`Strict`] by default. This means that cookies will only be sent in a first-party context, that is, requests originating from the same site that set the cookie. Some specific cookies that are required in a third party contexts, such as the session cookie are exempt from this behavior for now, but this will likely change in the future as using cookies in a third-party context will no longer be supported by browsers. |
It looks the word be missing (or some other similar word)?
There was a problem hiding this comment.
See above - strict also has implications on cookies not being available after redirects/links to Keycloak
|
|
||
| == Support for insecure contexts dropped | ||
|
|
||
| Due to the aforementioned changes, we are officially dropping support for running Keycloak outside of a https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts[secure context]. You should therefore ensure that your Keycloak instance if always running on what is https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts#when_is_a_context_considered_secure[considered a secure context]. As part of this, all cookies set by Keycloak will have the https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#secure[`Secure`] attribute by default. |
There was a problem hiding this comment.
Does this mean that some more update might need to be done in the documentation in relation to this? I can think of some update for the option http-enabled documented here https://www.keycloak.org/server/enabletls (as per my understanding http-enabled won't work correctly unless people are running on localhost or legacy cookies feature is enabled).
There was a problem hiding this comment.
Does this mean that some more update might need to be done in the documentation in relation to this?
Yes, the documentation will have to be reviewed and modified in this regard. Once the implementation settles down that will be the next step.
There was a problem hiding this comment.
We should probably add something to cleanup *_LEGACY cookies as otherwise these are left until deleted manually by the user, or they expire (which could be a long time if remember-me is enabled).
|
I wonder if we should add some documentation/guidance around cookies, as the migration guides now contain useful tips to those reading those, but not to new users. |
|
I'm not a fan of the following: This is not really information aimed at the user, but rather to the admin/ops guy installing Keycloak. Would be better to just display a generic error page, and output an error to the log. |
Yes, see #26161 (comment). We'll have to dedicate a section of the getting started guide to ensuring users run Keycloak in a secure context. |
There was a problem hiding this comment.
Summary of issues from me:
- We can not use
strict(as that restricts access on redirects as well) - at this point this is only a note in the release notes, and any changes here should be done in a follow-up so straightforward thing is just remove this mention in the release notes for now - Support for insecure contexts dropped - this could probably be explained a little bit better in the release notes, as it's not obvious that it's the legacy cookies that somewhat works around this issue currently
- Lack of cleanup of *_LEGACY cookies
- Error message. I don't link the specific hard-coded bits for this error message in the error form) and it also shows more information to the end-user than it should
|
After chatting with @jonkoops we have some slightly different ideas to try out first, in either case we'll not target removing legacy cookies for 24, but rather look at getting this into 25. Leaving the PR open here for now. |
Closes #16181