-
Notifications
You must be signed in to change notification settings - Fork 71
THREESCALE- 6512: Allow custom CSP #4185
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
base: master
Are you sure you want to change the base?
Changes from all commits
ba09201
0a95ae6
04a1f96
a0de8cd
e3bae5f
66ad3c9
07418e8
e925683
537d028
7c69d35
95d16ba
ea2d6d5
6e89bd7
d355854
036fa9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| base: &default | ||
| # Admin portal policy - restrictive but allows unsafe-inline/eval for existing code | ||
| # Dynamically includes RAILS_ASSET_HOST for CDN assets if configured | ||
| admin_portal: | ||
| enabled: true | ||
| report_only: false | ||
| policy: &default_admin_policy | ||
| default_src: ["'self'"] | ||
| script_src: ["'self'", "'unsafe-inline'", "'unsafe-eval'", "<%= ENV['RAILS_ASSET_HOST'] %>"] | ||
| style_src: ["'self'", "'unsafe-inline'", "<%= ENV['RAILS_ASSET_HOST'] %>"] | ||
| font_src: ["'self'", "data:", "<%= ENV['RAILS_ASSET_HOST'] %>"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't we have some internal way to access this, instead of env variable btw?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the top of my head: most probably yes. But then we'll have to worry about config load order to ensure the relevant rails config is already set when this yaml is parsed. I think taking the env variable is simpler here. |
||
| img_src: ["'self'", "data:", "blob:", "https:", "<%= ENV['RAILS_ASSET_HOST'] %>"] | ||
| connect_src: ["*"] | ||
| frame_src: ["'self'"] | ||
| frame_ancestors: ["'none'"] | ||
| object_src: ["'none'"] | ||
| base_uri: ["'self'"] | ||
|
|
||
| # Developer portal policy - permissive defaults for customization | ||
| developer_portal: | ||
| enabled: true | ||
| report_only: false | ||
| policy: | ||
| default_src: ["*", "data:", "mediastream:", "blob:", "filesystem:", "ws:", "wss:", "'unsafe-eval'", "'unsafe-inline'"] | ||
|
|
||
| development: | ||
| <<: *default | ||
| admin_portal: | ||
| enabled: true | ||
| report_only: false | ||
| policy: | ||
| <<: *default_admin_policy # Overwrite default policy in development to add webpack as source | ||
| script_src: ["'self'", "'unsafe-inline'", "'unsafe-eval'", "<%= ENV['RAILS_ASSET_HOST'] %>", "localhost:3035", "ws://localhost:3035"] | ||
| style_src: ["'self'", "'unsafe-inline'", "<%= ENV['RAILS_ASSET_HOST'] %>", "localhost:3035", "ws://localhost:3035"] | ||
| font_src: ["'self'", "data:", "<%= ENV['RAILS_ASSET_HOST'] %>", "localhost:3035", "ws://localhost:3035"] | ||
| img_src: ["'self'", "data:", "blob:", "https:", "<%= ENV['RAILS_ASSET_HOST'] %>", "localhost:3035", "ws://localhost:3035"] | ||
| connect_src: ["*", "localhost:3035", "ws://localhost:3035"] | ||
|
|
||
| test: | ||
| <<: *default | ||
|
|
||
| production: | ||
| <<: *default | ||
| admin_portal: | ||
| enabled: false | ||
| developer_portal: | ||
| enabled: false | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,28 @@ | ||
| # Be sure to restart your server when you modify this file. | ||
| # frozen_string_literal: true | ||
|
|
||
| # Define an application-wide content security policy. | ||
| # See the Securing Rails Applications Guide for more information: | ||
| # https://guides.rubyonrails.org/security.html#content-security-policy-header | ||
| # Configure Content Security Policy headers | ||
| # See: https://guides.rubyonrails.org/security.html#content-security-policy-header | ||
|
|
||
| Rails.application.config.to_prepare do | ||
| Rails.application.config.content_security_policy do |policy| | ||
| policy.default_src '*', :data, :mediastream, :blob, :filesystem, :ws, :wss, :unsafe_eval, :unsafe_inline | ||
| require 'three_scale/content_security_policy' | ||
|
|
||
| Rails.application.configure do | ||
| if ThreeScale::ContentSecurityPolicy::AdminPortal.enabled? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this might be a bit confusing... So, if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I fixed it: 7c69d35 |
||
| policy_config = ThreeScale::ContentSecurityPolicy::AdminPortal.policy_config | ||
|
|
||
| if policy_config.present? | ||
| config.content_security_policy do |policy| | ||
| ThreeScale::ContentSecurityPolicy::AdminPortal.add_policy_config(policy, policy_config) | ||
| end | ||
| end | ||
|
|
||
| # Set report-only mode if configured | ||
| config.content_security_policy_report_only = true if ThreeScale::ContentSecurityPolicy::AdminPortal.report_only? | ||
| else | ||
| config.content_security_policy do |policy| | ||
| ThreeScale::ContentSecurityPolicy::AdminPortal.add_policy_config( | ||
| policy, | ||
| ThreeScale::ContentSecurityPolicy::AdminPortal::DEFAULT_POLICY | ||
| ) | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # Rails.application.configure do | ||
| # config.content_security_policy do |policy| | ||
| # policy.default_src :self, :https | ||
| # policy.font_src :self, :https, :data | ||
| # policy.img_src :self, :https, :data | ||
| # policy.object_src :none | ||
| # policy.script_src :self, :https | ||
| # policy.style_src :self, :https | ||
| # # Specify URI for violation reports | ||
| # # policy.report_uri "/csp-violation-report-endpoint" | ||
| # end | ||
| # | ||
| # # Generate session nonces for permitted importmap, inline scripts, and inline styles. | ||
| # config.content_security_policy_nonce_generator = ->(request) { request.session.id.to_s } | ||
| # config.content_security_policy_nonce_directives = %w(script-src style-src) | ||
| # | ||
| # # Report violations without enforcing the policy. | ||
| # # config.content_security_policy_report_only = true | ||
| # end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module ThreeScale | ||
| module Middleware | ||
| class DeveloperPortalCSP | ||
| def initialize(app) | ||
| @app = app | ||
|
|
||
| # Pre-compute the CSP header once at startup since we don't use nonces or dynamic sources | ||
| @csp_header_name, @csp_header_value = compute_csp_header | ||
| end | ||
|
|
||
| def call(env) | ||
| request = ActionDispatch::Request.new(env) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit complicated for me... because I have no idea how this is supposed to work. But it seems that most of the logic is similar to what https://github.com/rails/rails/blob/v7.1.5.2/actionpack/lib/action_dispatch/http/content_security_policy.rb#L35 does. I guess it's fine... It's unfortunate though that we (apparently) cannot reuse the existing logic.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, basically, I created this class and added it to the developer portal middleware stack.
Our This class basically generates the headers once on startup and then adds them to each request. We can't reuse the existing middleware because that one takes the CSP policy from the Rails global CSP configuration, but we need to take it from our yaml file. However, the Rails CSP middleware is in fact installed also in the stack, so we are calling it anyway, that's why we have this snippet: unless request.format.html?
request.content_security_policy = false
return @app.call(env)
endWhen the request is HTML, we handle it; when it's not, we don't, and ensure the rails middlware doesn't handle it neither.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| # We want to apply CSP only for HTML requests. However, we can't just return | ||
| # because Rails will add global CSP policy (admin portal policy) to the response | ||
| # if we don't do anything. We disable CSP for this request to prevent Rails middleware | ||
| # to interfere. | ||
| unless request.format.html? | ||
| request.content_security_policy = false | ||
| return @app.call(env) | ||
| end | ||
|
|
||
| status, headers, _body = response = @app.call(env) | ||
|
|
||
| # Don't apply CSP to 304 responses to avoid cache issues | ||
| return response if status == 304 | ||
|
|
||
| # Only apply if we have a pre-computed CSP header | ||
| headers[@csp_header_name] = @csp_header_value if @csp_header_value | ||
|
|
||
| response | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def compute_csp_header | ||
| # Only compute if enabled and there's a policy configured | ||
| policy_config = ThreeScale::ContentSecurityPolicy::DeveloperPortal.policy_config | ||
|
|
||
| unless ThreeScale::ContentSecurityPolicy::DeveloperPortal.enabled? && policy_config.present? | ||
| policy = ThreeScale::ContentSecurityPolicy::DeveloperPortal.build_policy( | ||
| ThreeScale::ContentSecurityPolicy::DeveloperPortal::DEFAULT_POLICY | ||
| ) | ||
|
|
||
| return [ | ||
| ActionDispatch::Constants::CONTENT_SECURITY_POLICY, | ||
| policy.build | ||
| ] | ||
| end | ||
|
|
||
| # Build the policy once at initialization | ||
| policy = ThreeScale::ContentSecurityPolicy::DeveloperPortal.build_policy(policy_config) | ||
| header_name = if ThreeScale::ContentSecurityPolicy::DeveloperPortal.report_only? | ||
| ActionDispatch::Constants::CONTENT_SECURITY_POLICY_REPORT_ONLY | ||
| else | ||
| ActionDispatch::Constants::CONTENT_SECURITY_POLICY | ||
| end | ||
| header_value = policy.build | ||
|
|
||
| [header_name, header_value] | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module ThreeScale | ||
| module ContentSecurityPolicy | ||
| class Base | ||
|
|
||
| DEFAULT_POLICY = { | ||
| default_src: ['*', :data, :mediastream, :blob, :filesystem, :ws, :wss, :unsafe_eval, :unsafe_inline] | ||
| }.freeze | ||
|
|
||
| class << self | ||
| def config | ||
| @config ||= Rails.configuration.three_scale.content_security_policy | ||
| end | ||
|
|
||
| def enabled? | ||
| raise NoMethodError, "#{__method__} not implemented in #{self.class}" | ||
| end | ||
|
|
||
| def policy_config | ||
| raise NoMethodError, "#{__method__} not implemented in #{self.class}" | ||
| end | ||
|
|
||
| def report_only? | ||
| raise NoMethodError, "#{__method__} not implemented in #{self.class}" | ||
| end | ||
|
|
||
| # Builds an ActionDispatch::ContentSecurityPolicy object from a policy configuration hash | ||
| def build_policy(policy_config) | ||
| ActionDispatch::ContentSecurityPolicy.new do |policy| | ||
| add_policy_config(policy, policy_config) | ||
| end | ||
| end | ||
|
|
||
| # Applies a policy configuration hash to an existing policy object | ||
| def add_policy_config(policy, policy_config) | ||
| policy_config.each do |directive, values| | ||
| method_name = directive.to_s | ||
| next unless policy.respond_to?(method_name) | ||
|
|
||
| # Handle directives with sources (arrays) vs boolean directives | ||
| if values.is_a?(Array) | ||
| policy.public_send(method_name, *values) | ||
| else | ||
| policy.public_send(method_name, values) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| class AdminPortal < Base | ||
| class << self | ||
| def enabled? | ||
| config&.admin_portal&.enabled == true | ||
| end | ||
|
|
||
| def policy_config | ||
| config&.admin_portal&.policy || {} | ||
| end | ||
|
|
||
| def report_only? | ||
| config&.admin_portal&.report_only == true | ||
| end | ||
| end | ||
| end | ||
|
|
||
| class DeveloperPortal < Base | ||
| class << self | ||
| def enabled? | ||
| config&.developer_portal&.enabled == true | ||
| end | ||
|
|
||
| def policy_config | ||
| config&.developer_portal&.policy || {} | ||
| end | ||
|
|
||
| def report_only? | ||
| config&.developer_portal&.report_only == true | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is that these security headers, including the Browser Permissions header, may require multiple files modification and corresponding Operator support. So I wonder if it makes sense to put this config in the database (e.g. the settings table) or maybe at least in a single config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit reluctant to create migrations if possible. Would this approach imply so much trouble for the operator? I think the CSP feature can be completely disabled by default, and whoever wants to enable it, would just need to add a new entry to one configmap. However, in our side we would need to add the migration and then the UI or API to CRUD the data, tests, etc. Which would imply probably rejecting this PR and start a new one. Is it worth it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't go the database/migration way.
We probably just need to either double-check what's the easiest way to customize the config with existing methods, or open an issue for operator to make it easier for customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is all secrets with the files inside like other config files. The issue with that is that as soon as we change something on our side to require a config change, customers will very likely miss to update the config on their end. For example if they want to enable a future captcha provider.
On the other hand, having it in a DB just makes it easier to fix that. But will also allow flexibility of a per-portal configuration 🤔
I would go with a separate account settings table, not like the current one, because current one if rigid and hard to extend. Something like:
I think proper extendable settings will make a lot of sense for 2.17 and avoid this issue to introduce new configuration files for every new feature we want to implement. With such a settings table we can dynamically add and remove settings options.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @akostadinov. I appreciate your efforts to annoy me and I have to admit you're becoming a true professional on this art.
How strong is your opinion on this? If it's really strong I'll close this PR and start working on your approach. But first please consider:
Are you pushing the red button?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't have a concept before. I think you did an amazing job with this. And 44hours is not lost. Just I don't find this maintenance friendly and not user friendly. That's why. I wouldn't hard stop you but to me it makes no sense to dig ourselves more into the same problem. So you decide.