Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add Content-Security-Policy configuration option #8943

Merged
merged 3 commits into from Mar 31, 2022

Conversation

zachaller
Copy link
Contributor

This should finish up the work on issue #2706 by adding a configurable
Content-Security-Policy header which defaults to frame-ancestors 'self';

This matches what we do with X-Frame-Options=sameorigin some reference information found
here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors

Signed-off-by: zachaller zachaller@hotmail.com

This should finish up the work on issue argoproj#2706 by adding a configurable
Content-Security-Policy header which defaults to frame-ancestors 'self';

This matches what we do with X-Frame-Options=sameorigin some reference information found
here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors

Signed-off-by: zachaller <zachaller@hotmail.com>
@codecov
Copy link

codecov bot commented Mar 30, 2022

Codecov Report

Merging #8943 (d0d4875) into master (8847a31) will increase coverage by 1.52%.
The diff coverage is 63.37%.

@@            Coverage Diff             @@
##           master    #8943      +/-   ##
==========================================
+ Coverage   43.40%   44.92%   +1.52%     
==========================================
  Files         186      212      +26     
  Lines       23373    25263    +1890     
==========================================
+ Hits        10145    11350    +1205     
- Misses      11779    12309     +530     
- Partials     1449     1604     +155     
Impacted Files Coverage Δ
applicationset/services/pull_request/fake.go 0.00% <0.00%> (ø)
applicationset/utils/createOrUpdate.go 0.00% <0.00%> (ø)
applicationset/utils/policy.go 0.00% <0.00%> (ø)
applicationset/services/pull_request/github.go 14.00% <14.00%> (ø)
...is/applicationset/v1alpha1/applicationset_types.go 34.69% <34.69%> (ø)
applicationset/generators/scm_provider.go 53.44% <53.44%> (ø)
applicationset/generators/pull_request.go 56.25% <56.25%> (ø)
...cationset/controllers/applicationset_controller.go 56.75% <56.75%> (ø)
applicationset/services/repo_service.go 57.14% <57.14%> (ø)
...licationset/generators/generator_spec_processor.go 57.57% <57.57%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e864652...d0d4875. Read the comment docs.

Signed-off-by: zachaller <zachaller@hotmail.com>
@crenshaw-dev
Copy link
Collaborator

This directive is similar to the X-Frame-Options header that several user agents have implemented. The 'none' source expression is roughly equivalent to that header’s DENY, 'self' to SAMEORIGIN, and so on. The major difference is that many user agents implement SAMEORIGIN such that it only matches against the top-level document’s location. This directive checks each ancestor. If any ancestor doesn’t match, the load is cancelled. [RFC7034]

- https://www.w3.org/TR/CSP2/#frame-ancestors-and-frame-options

So if someone out there is serving Argo CD via nested iframes like company.com/argo-cd-frame-top -> other.com/argo-cd-frame-inner -> company.com/argo-cd, this is a breaking change for them.

I'm okay with that. :-)

Copy link
Collaborator

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

Want to add this var to manifests/base/server/argocd-server-deployment.yaml so this can be configured via argocd-cmd-params-cm like x-frame-options?

…ed via configmap

Signed-off-by: zachaller <zachaller@hotmail.com>
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Nice! Thank you @zachaller

LGTM

@crenshaw-dev crenshaw-dev merged commit e9fae0d into argoproj:master Mar 31, 2022
@zachaller zachaller deleted the fix-add-csp-header branch March 31, 2022 19:04
wojtekidd pushed a commit to wojtekidd/argo-cd that referenced this pull request Apr 25, 2022
* fix: Add Content-Security-Policy configuration

This should finish up the work on issue argoproj#2706 by adding a configurable
Content-Security-Policy header which defaults to frame-ancestors 'self';

This matches what we do with X-Frame-Options=sameorigin some reference information found
here https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors

Signed-off-by: zachaller <zachaller@hotmail.com>

* Run codegen

Signed-off-by: zachaller <zachaller@hotmail.com>

* fix: add ARGOCD_SERVER_CONTENT_SECURITY_POLICY env var to be configured via configmap

Signed-off-by: zachaller <zachaller@hotmail.com>
Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants