Better management of the CSP header #24577
Conversation
ghost
added
|
|
||
| if (options.isAllowAnyFrameAncestor()) { | ||
| headers.remove(BrowserSecurityHeaders.X_FRAME_OPTIONS.getHeaderName()); | ||
|
|
||
| csp.frameAncestors(null); | ||
| if (csp.isDefaultFrameAncestors()) { | ||
| // only remove frame ancestors if defined to default 'self' |
There was a problem hiding this comment.
@rmartinc Can you elaborate on why this is the case? Isn't the intention of options.isAllowAnyFrameAncestor() so that we can allow some resources to be loaded under any third-party domains? If that is the case, then we should remove frame ancestors regardless whether it is defined to be default or not, right?
There was a problem hiding this comment.
The intention of this part is to just force the frame-ancestors if it's not modified by the admins. If admins are setting some default ancestors for example self https://*.example.com I suppose the admins know what they are doing and limiting to those ancestors on purpose (because they know the clients are always from those URLs or similar). But not writing in stone, if you have a better idea just let me know. This part is only used to get the IFrame for login.
There was a problem hiding this comment.
I see that currently there are three resources that are using the options.isAllowAnyFrameAncestor() feature to remove frame ancestors, so that they can be loaded in any iFrames:
It seems to me that it is safe to allow these resources to be hosted in any iFrames all the time. That is why I am thinking we should continue to treat these resources differently from the other pages (such as the actual login pages), even when the admins specifically are limiting the frame ancestors on purpose.
There was a problem hiding this comment.
Yes, all those resources are used by the keycloak-js client when the checkLoginIframe is default true. So, just used for the keycloak-js client login. I'm OK to always remove ancestors but then we should take in mind that this cannot be overridden.
| @@ -1029,4 +1033,35 @@ public void testExecuteActionIfSessionExists() { | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| @Test | |||
| public void testFrontChannelLogoutCustomCSP() throws Exception { | |||
There was a problem hiding this comment.
Is it possible to use rather RPInitiatedLogoutTest as it contains other tests related to OIDC frontchannel-logout? (I am actually thinking about moving frontchannel-logout tests to separate class, but I guess that is beyond the scope of this PR, but if you want to do it, it is welcome :-) )
There was a problem hiding this comment.
No, it's not possible. For this test I need the chrome/firefox driver to correctly follow the CSP directives. If the test uses default browser it is always OK because the CSP directives are not taken into account. So I need the forms or the wenauthn CI run (the ones that uses -Dbrowser=chrome). I remember I initially added the test in another class to realize that it was not tested in CI. If you want I can add it in a complete new class but in forms folder (something like CSPLoginTest for example).
Rebased too just in case.
There was a problem hiding this comment.
Created a RPInitiatedFrontChannelLogoutTest inside forms folder with the frontend tests of RPInitiatedLogoutTest and the new CSP method. The LoginTest is not modified now.
|
|
||
| public static ContentSecurityPolicyBuilder create() { | ||
| return new ContentSecurityPolicyBuilder(); | ||
| return new ContentSecurityPolicyBuilder() | ||
| .add("frame-src", "'self'") |
There was a problem hiding this comment.
Nitpick: Java constants for those fields (directive-names as well as stuff like 'self') can be nice.
Closes keycloak#24568 Signed-off-by: rmartinc <rmartinc@redhat.com>
Closes #24568
With this PR the CSP modifications are performed independently to the
frame-src(front-channel logout handler) andframe-ancestors(getting the iframe from resources) directives. In theframe-srccase, the different hostnames are added to the configured directive (for example if the realm defines'self' keycloak.orgthe final directive sent after adding the clients values is'self' keycloak.org client1 client2). In the case offrame-ancestorsthe directive is removed for iframe inclusion only if the default'self'value is configured (if the admin changed it to any other value, the directive is not modified). Added a test informsto use firefox and chrome (the test needs a real browser to check the CSP header).Parsing just manages spaces as separator to read the current CSP defined in the realm. The spec allows other things as separator (like tabs, new lines,...) but I didn't want to complicate it more. If you think it's necessary just let me know.
@mposolda @stianst Take a look when you have time.