Repository navigation
Conversation
3239ba8 to
fad2b69
Compare
There was a problem hiding this comment.
There will be likely some test failures in the Quarkus area for the generated help output with the new option.
I'd also like to see something in the release notes for this and I'm happy to contribute those. Anywhere else we would need to document this?
Feel free to add a commit to fix these, I am not too familiar with this area.
Probably the supported features page on the site. But I believe that is updated automatically? |
|
@jonkoops - there's now PR jonkoops#13 which fixes "Keycloak CI / Quarkus IT (zip)". The other failing tests are IMHO unstable tests. Please squash the commits after merging. Also, please add an issue number to the commit (the original commit for this PR doesn't contain an issue number). Thanks! |
|
@jonkoops - regarding docs, IMHO there should be an update to the release notes and the migration guide. See keycloak/keycloak-documentation#1761 for the PR. UPDATE: Yes, the features page will be updated automatically, no additional needs there. |
89ee1f4 to
6dbec32
Compare
|
Thanks for looking into that @ahus1, I've pulled in your changes and squashed the commits. |
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.
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.
There was a problem hiding this comment.
Thanks for implementing this. I reviewed it and also did a manual smoke test.
|
Thanks for the assist @ahus1. Java is not my native land so I appreciate it. |
|
@vmuzikar could you review this one? |
Co-authored-by: Alexander Schwartz <aschwart@redhat.com>
6dbec32 to
eab835b
Compare
There was a problem hiding this comment.
I'm afraid removing legacy cookies breaks HTTP (non-SSL) requests, at least to the admin console. That is because we have to force Secure anytime we use SameSite=None:
This also concerns the default
kc.sh start-dev profile.
I think we have to consider using SameSite=Lax when HTTP is explicitly enabled and document this behaviour.
eab835b
|
@vmuzikar You're right and I missed it. It works fine in most browsers (E.g.: FF and Chrome) but only if the domain name is However, this change as is should impact people not only running in dev mode (e.g.: Ideally, we should decide (instead of hard-coded as it is today) the same site policy (and if it is secure) for cookies depending on both realm and server settings. |
|
@vmuzikar Looking at it more closely looks like we can rely on the By default, realms are set with We could avoid setting In dev mode and using default realm settings ( In production mode and using default realm settings, we should not include However, when running behind a proxy, we are going to rely on the client address set (or not) by the proxy. If the proxy is opening connections through a non-secure channel, the server might end up not marking cookies as In other words, it needs a bit more analysis to make sure we are not breaking existing deployments and their assumptions about how we set those cookies. [1] |
|
@pedroigor I think we can make some assumptions to set |
|
@vmuzikar Not sure if we need to explicitly set samesite to |
|
This is growing a bit beyond the scope that I can pick up at the moment (have some other high prio tasks). If anyone wants to take this PR let me know. Personally I think the best approach is always to use the most secure default and document this properly. If users run into issues with these defaults they should opt-out explicitly (e.g. |
|
A note to those who might pick up this PR up later: There used to be a PR for the docs, which is no longer mergeable due to the move of the docs: keycloak/keycloak-documentation#1761 |
|
I am closing this PR, as it has been out there for too long. I'll try to do a second attempt at this soon. |
|
Hello @jonkoops @pedroigor, I'm coming from this discussion and the topic that the From what I read here and from this code comment , it seems as of today, it's not possible to set it as HttpOnly. So my questions are,
Related discussions:
Best regards |
I think that OWASP can give you the best answer to this question. We need those cookies to be accessible to JavaScript so we can verify the user has an active session, or otherwise redirect the user to restart the login flow.
Not at this time. But we're going to be re-evaluating the defaults on cookies in the coming year or so, which may or may not cause this to change. |
|
@jonkoops Thanks a lot for the response. |
|
The latter, there is currently no mitigation you can take. In the future we will likely add an SPI so you can control cookie behaviour in a granular manner, at your own risk of course. |
|
@jonkoops thanks again, so I guess that's a risk we have to live with for now. From what I understand, this cookie can't be set as HttpOnly because some Keycloak core elements relies on it (Admin Console, User Account, Login flow, etc). |
Closes #16181