THREESCALE- 6512: Allow custom CSP

[jlledom]

What this PR does / why we need it:

We previously added an all-allowed policy for convenience (#3861), but we provided no way to configure any other CSP policy. This PR accepts a new config file under config/content_security_policy.yml where porta loads its CSP policy from. Example:

  admin_portal_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'] %>"]
    img_src: ["'self'", "data:", "blob:", "https:"]
    connect_src: ["'self'"]
    frame_src: ["'self'"]
    frame_ancestors: ["'none'"]
    object_src: ["'none'"]
    base_uri: ["'self'"]

The PR consists basically in four changes:

• Read the new config file from application.rb
• New CSP config loader classes, for convenience.
• Update the initializer to get the global config from the CSP config class.
• New middleware to set a different policy for the developer portal.

If no CSP config file, it will fall back to the previous all-allowed policy. When present, it will apply admin_portal_policy To master and provider portals, and developer_portal_policy to developer portals.

In fact, admin_portal_policy is set on rails as global policy, so if no developer_portal_policy is provided, admin portal CSP will be applied to dev portal as well.

By default, I'm setting the all-allowed policy to dev portal anyway, because we can't know what clients will publish there. About admin portal, I'm setting a more restrictive defaults that would be valid for the whole portal, tests pass, however I might have missed something, let's see.

By default the CSP policy is enabled in all environments, including development and test, the reason is for future developments that could introduce new CSP violations to be revealed before they reach production.

Additional comments

At the beginning I discarded allowing different CSP policy for admin and developer portal, because this CSP cusomization only makes sense for Dev portal actually, since clients can't decide the contents of the admin portal, other than allowing the CDN url. However, if we only have one global policy, whatever clients set for their developer portals would affect also the admin portal, over which they have no control. So I finally opted for allowing different policies.

There are a few features I discarded for different reasons:

1. Add support for new report-to directive

We support report-uri (docs), which is marked as deprecated, however, its replacement report-to is not supported by rails yet (docs only mention report_uri, also, the :report_to method is not defined in the class).

The CSP directive and the required HTTP header are also not widely supported for all browsers yet, e.g. not supported at all in Firefox.

I could add a small implementation, via middleware, but I think it's better to just use what rails provide today, that way the code will be easier to maintain.

2. Use secure_headers gem rather than Rails builtin support.

We are currently depending on the secure_headers gem. And that gem allows to add support for CSP. However, its CSP support doesn't offer any advantage over rails builtin support, so I opted for Rails, for better maintainability. Also, we would need to update the gem to a newer major version in order to get the same features rails provides.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-6512

Verification steps

1. Try different policies, verify the Content-Security-Policy HTTP header is actually set to what is configured in the yaml file.
2. In the yaml file, set enabled to false. The CSP header should contain the old all-allowed policy.
3. Remove the yaml file. The CSP header should contain the old all-allowed policy.

[jlledom]

@mayorova yes to all: 04a1f96

[mayorova]

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.

[jlledom]

Well, basically, I created this class and added it to the developer portal middleware stack.

• On startup, a new instance will be initialized.
• On every request, the call method will be called.

Our call method is called by the previous middleware in the stack. By calling @app.call(env) we yield control to the next middleware in the stack, and eventually to the controller.

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)
end

When the request is HTML, we handle it; when it's not, we don't, and ensure the rails middlware doesn't handle it neither.

[jlledom]

• Admin portal: Yaml -> Rails Global CSP config -> Rails CSP middleware
• Dev portal: Yaml -> Our dev portal CSP middleware

[akostadinov]

I'm wondering, does this need to be per instance or per tenant? I assume the admin/master portals need to be per instance because we don't have custom stuff in there. For dev portals, it would make some sense to be individual. Just asking whether this makes practical sense. It will be a little more user friendly if configured from UI but given it will probably rarely be used, perhaps doesn't make much practical sense... thinking out loud.

[jlledom]

I'm wondering, does this need to be per instance or per tenant?

Yeah it would be good to be per tenant, specially the dev portal ones. But that would require more effort: Adding columns to the settings table, or maybe a new table; adapt models, creating the API endpoints or UI + Controllers... Do you want to create an issue?

[akostadinov]

Do you want to create an issue?

Only if you think that the benefit will outweigh the effort. Otherwise we can go like this and see if requests come.

[jlledom]

Do you want to create an issue?

Only if you think that the benefit will outweigh the effort. Otherwise we can go like this and see if requests come.

It would be useful for SaaS, of course. Not sure about On premises. Do on premises clients have more than one tenant usually?

I wouldn't do it only for SaaS

[mayorova]

I think this might be a bit confusing...

So, if admin_portal.enabled: true, and developer_portal.enabled: false, the admin portal's custom policy from YAML is applied for the developer portal too.

[jlledom]

Right, I fixed it: 7c69d35

[akostadinov]

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.

[jlledom]

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?

[mayorova]

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.

[akostadinov]

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:

class CreateSettings < ActiveRecord::Migration[7.1]
  def change
    create_table :settings do |t|
      t.integer :account_id, null: false
      t.string :name, null: false
      t.text :value

      t.timestamps
    end

    add_index :settings, [:account_id, :name], unique: true
  end
end

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.

[jlledom]

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:

1. I already spent 44 hours and 10 minutes on this PR (I have a counter), that could be wasted if I close the PR.
2. It would hurt me in the bottom of my heart, in the exact spot where feelings are born, to close this after I spent so much effort on it.
3. Man, the PR is open for more than a month now, you could have stopped me before.

Are you pushing the red button?

[akostadinov]

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.

[jlledom]

This is the list of known scenarios where the admin portal makes requests to external servers:

• Assets from CDN: Works
• API Docs: Works
• File storage, e.g. S3: Works
• Payment gateways: Works
• SSO Interactions: Works
• Recaptcha
  • Doesn't work, because www.google.com and www.gstatic.com must be added to the CSP headers
• Openshift Service discovery
  • Not tested

I disabled CSP headers by default: 036fa9f

I think I'm gonna leave it here. Maybe we can add some release notes, IDK.

[akostadinov]

Openshift Service discovery
Does it have anything to do with the browser?

[akostadinov]

You did all the work, I'm +1 for making these headers the default in this case. Maybe in SaaS do NOT enable for dev portals.

[jlledom]

Openshift Service discovery

Does it have anything to do with the browser?

I don't think the client UI is involved, but I'm not sure. @mayorova do you know?

[jlledom]

You did all the work, I'm +1 for making these headers the default in this case. Maybe in SaaS do NOT enable for dev portals.

I'm not sure, to make it default we should add the Recaptcha endpoints to the default CSP, and those endpoints could change tomorrow without any previous warning and break captcha for our clients overnight. My idea was to make CSP enabled by default, look for all corner cases and fix them. but the recaptcha one made me change my opinion, we don't want to be responsible for what google decides to do.

The issue was about allowing clients to provide their own CSP if they want, so that's done.

[mayorova]

I don't think the client UI is involved, but I'm not sure. @mayorova do you know?

I was not sure, but I I've checked and apparently indeed the interaction with OpenShift happens on the backend https://github.com/3scale/porta/blob/1e9473b16cdbf30875ca15e78b25e1c51788629d/app/controllers/provider/admin/service_discovery/services_controller.rb

[akostadinov]

I think it is more work now but then will be much easier to add/remove settings, also much better user experience, if we do some database improvement like: #4185 (comment)

[jlledom]

@akostadinov @mayorova I made a change to fix a CVE I found:

https://discuss.rubyonrails.org/t/cve-2024-28103-permissions-policy-is-only-served-on-html-content-type/85948

Commit: b5b64a2

[akostadinov]

cve-2024-28103 seems to only apply to Permissions-Policy and this is the CSP PR. I assume CSP will just be ignored if served for other content types. Either way, what other content-type would require CSP headers present?

[jlledom]

cve-2024-28103 seems to only apply to Permissions-Policy and this is the CSP PR.

Correct. I observed that rails actually adds CSP headers not only for HTML but all responses. Initially I didn't understand why rails was doing this, and when I found this CVE I assumed this was the reason, but it's true the CVE doesn't mention CSP.

Our custom middleware for the developer portal was sending headers only for HTML, but now it mimics rails behavior.

I assume CSP will just be ignored if served for other content types. Either way, what other content-type would require CSP headers present?

I'm not sure, maybe there's some scenario where the client downloads a script directly, or a CSS with links, and process it.

I don't think it harms to send the headers but I can revert the commits if you insist.

[akostadinov]

I don't really understand. How does your commit prevent CSP to be set?

[jlledom]

I don't really understand. How does your commit prevent CSP to be set?

You mean b5b64a2 ?

This doesn't prevent CSP to be sent, this allows it, because it removes the code that was preventing CSP to be set on non-HTML responses.

On the dev portal middleware, we were checking for HTML and rmeoving CSP on purpose, now I removed that

[akostadinov]

This doesn't prevent CSP to be sent, this allows it, because it removes the code that was preventing CSP to be set on non-HTML responses.

This is how I read it the first time, but then you wrote "I observed that rails actually adds CSP headers not only for HTML but all responses." so I thought you were trying to prevent that.

Ok, so I would question the need to send CSP with non-html responses? Do you still find this needed? Do you find any information that these should be sent regardless?

[jlledom]

This doesn't prevent CSP to be sent, this allows it, because it removes the code that was preventing CSP to be set on non-HTML responses.

This is how I read it the first time, but then you wrote "I observed that rails actually adds CSP headers not only for HTML but all responses." so I thought you were trying to prevent that.

Oh, the confusion here might be because there are two middlewares to insert CSP headers. One for the admin portal and one for the developer portal, since they can have different CSP policies.

The admin portal one is implemented by rails. And this PR adds the middleware for the dev portal. In the admin portal, only the rails middlware runs, but in the developer portal both middlewares run, first ours, then rails'.

The Rails middleware injects CSP headers for all responses, not only HTML, now the dev portal middleware mimics that behavior.

Ok, so I would question the need to send CSP with non-html responses? Do you still find this needed? Do you find any information that these should be sent regardless?

I think it doesn't harm, and if rails does it, we should do it as well I think.

[akostadinov]

can we disable rails middleware in portal? Or is it only possible in opensift where we run all separately?

[jlledom]

can we disable rails middleware in portal? Or is it only possible in opensift where we run all separately?

I think the only way is to write some black magic altering rails internals. Not worth it. What we can do instead is to not actually edit headers in our middleware and just overwrite the CSP config for the current request, then the Rails middleware will read that config and decide when and how add the headers

[akostadinov]

just overwrite the CSP config for the current request, then the Rails middleware will read that config and decide when and how add the headers

Sounds dangerous, although ruby is not really multi-threading and unicorn is even less multi-threading, I fear of some concurrency issues if we change global configuration.

I'd rather disable the built-in CSP middleware and have a custom one handle both portals than juggling the global config.

[jlledom]

just overwrite the CSP config for the current request, then the Rails middleware will read that config and decide when and how add the headers

Sounds dangerous, although ruby is not really multi-threading and unicorn is even less multi-threading, I fear of some concurrency issues if we change global configuration.

I'd rather disable the built-in CSP middleware and have a custom one handle both portals than juggling the global config.

I wouldn't mess up with rails internals. Better leave it as it is.

[akostadinov]

I wouldn't mess up with rails internals. Better leave it as it is.

Why mess with Rails internals?

Either disable the config

Rails.application.configure do
  config.content_security_policy = nil
  config.content_security_policy_report_only = nil
end

or remove the Middleware

    config.middleware.delete ActionDispatch::ContentSecurityPolicy::Middleware