dexidp
Add frame ancestor configuration for web app to prevent clickjacking
Signed-off-by: ariary [email protected]
Overview
Provide a way to configure the Content Security Policy frame-ancestor context to prevent clickjacking
What this PR does / why we need it
This PR enables the configuration of the Content-Security policy to prevent clickjacking. By filling dex configuration with the specific fields the application will send csp headers in responses defining the content security policy.
available CSP: ○ No domain could frame the content (recommended unless a specific need has been identified for framing.) DEFAULT ○ Only the current site could frame the content ○ Only certain site could frame the content (must specify the protocol) ○ No CSP (ie any domain could frame the content, INSECURE)
Special notes for your reviewer
The most critical endpoints for clickjacking is the /dex/auth one (as a user interaction is needed to provide credential) but by default it is a good point to apply the same policy for all endpoints
Does this PR introduce a user-facing change?
NONE
@ariary thanks a lot for submitting this PR.
I have to admit, I'm not that familiar with CSP, but this looks mostly good. I'll let other maintainers jump in, maybe they are more familiar with CSP.
Also, can you please rebase your PR. CI wasn't triggered for some reason. Thanks!
LGTM, but it would be worthwhile for @nabokihms to take a look before merging.
As for me, it does not look right to add more headers customization to Dex, besides the ones described in the oauth2 RFC. There are many options for CSP configuration (and for other security measurements) with myriads of use-cases that we will not be able to cover in Dex code.
Most of the time, we have a reverse proxy in front of Dex, e.g., ingress controllers in Kubernetes clusters or just Nginx installation for standalone deployments. This looks like the right place for adding headers modifications.
Anyway, I see no objections to merging this PR right now, but we definitely need to consider whether to accept headers modification settings in the future.
Hm, I wonder if accepting this change is the right thing to do then. If we decide to remove it later, it's going to be harder to reason about.
I guess the alternative is providing sufficient documentation about configuring these headers.
I don't have a strong opinion on merging/rejecting this though. I'll leave the decision to you @nabokihms and @justaugustus
We probably need this sooner or later, but we have to be careful about it, so moving to next release.
I agree with @nabokihms: this would be better handled at ingress controllers/reverse proxies.
As a first step, I'll write some documentation to help with installation.
In the future, we might want to add a more general, http layer configuration (we do support a TLS server after all).
I'm going to leave this open for now (maybe we can still merge it before we find a more robust HTTP config solution).
Hi, We are facing the same issue and the frame ancestor configuration would be very helpful for our uses. Is the merge planned for this pull request ?
Thanks a lot Best Regards
It should be fixed now starting from Dex v2.39.0. I'm closing this PR as superseded.