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

[mayorova]

Works well for me now.
I haven't tested the situation when CDN is set.

Also, the default configuration doesn't work in local development environment when using with webpack, but that's fine, as not copying the config file, or disabling the custom policies is easy.

[jlledom]

Works well for me now. I haven't tested the situation when CDN is set.

Also, the default configuration doesn't work in local development environment when using with webpack, but that's fine, as not copying the config file, or disabling the custom policies is easy.

Fixed: ea2d6d5

[mayorova]

Fixed: ea2d6d5

OK, so you decided to fix it... 😅

It mostly works, but not completely. Some more violating occurrences:

Dashboard:

Connecting to 'http://provider-admin.3scale.localhost:3000/p/admin/dashboard/new_accounts' violates the following Content Security Policy directive: "connect-src localhost:3035 ws://localhost:3035". The action has been blocked.
Connecting to 'http://provider-admin.3scale.localhost:3000/p/admin/dashboard/potential_upgrades' violates the following Content Security Policy directive: "connect-src localhost:3035 ws://localhost:3035". The action has been blocked.
Connecting to 'http://provider-admin.3scale.localhost:3000/.well-known/appspecific/com.chrome.devtools.json' violates the following Content Security Policy directive: "connect-src localhost:3035 ws://localhost:3035". The request has been blocked.
Connecting to 'http://provider-admin.3scale.localhost:3000/assets/provider/layout/provider.js-4f87882317ebf43dcb87b1d4e95c550d43dcf1318b063764e4ee9cf8fc40e6d0.map' violates the following Content Security Policy directive: "connect-src localhost:3035 ws://localhost:3035". The request has been blocked.
Connecting to 'http://provider-admin.3scale.localhost:3000/assets/vendor/c3.css-10f1f5b56836976a99b4d704b778059d4ede730e9941818835eb7ffa172c3267.map' violates the following Content Security Policy directive: "connect-src localhost:3035 ws://localhost:3035". The request has been blocked.
Connecting to 'http://provider-admin.3scale.localhost:3000/assets/provider/themes/main.css-50e0f0601d2b79d0be40872f528f8d72f0ed40cc85fabe14bb4f7daf6604b736.map' violates the following Content Security Policy directive: "connect-src localhost:3035 ws://localhost:3035". The request has been blocked.

ActiveDocs page:

Connecting to 'http://provider-admin.3scale.localhost:3000/p/admin/api_docs/account_data.json' violates the following Content Security Policy directive: "connect-src localhost:3035 ws://localhost:3035". The action has been blocked.

I guess adding self to connect-src might fix it, but I haven't checked.

[jlledom]

OK, so you decided to fix it... 😅

It mostly works, but not completely. Some more violating occurrences:

Super fixed this time for good 100% real no fake: 6e89bd7

[mayorova]

Super fixed this time for good 100% real no fake: 6e89bd7

Requests within ActiveDocs still don't work (including Service Management API), e.g.

Connecting to 'http://api-2.apicast.dev.localhost:8000/?user_key=asdf' violates the following Content Security Policy directive: "connect-src 'self' localhost:3035 ws://localhost:3035". The action has been blocked.

Connecting to 'http://127.0.0.1:3001/transactions/authorize.xml?service_token=abc&service_id=2&user_key=123&usage%5Bhits%5D=1' violates the following Content Security Policy directive: "connect-src 'self' localhost:3035 ws://localhost:3035". The action has been blocked.

But honestly... I'm fine with it 😅

It would be pretty hard to figure out the exhaustive list.

[jlledom]

Requests within ActiveDocs still don't work (including Service Management API), e.g.

But honestly... I'm fine with it 😅

It would be pretty hard to figure out the exhaustive list.

Challenge accepted!

[jlledom]

Super fixed this time for good 100% real no fake: 6e89bd7

Requests within ActiveDocs still don't work (including Service Management API), e.g.

Connecting to 'http://api-2.apicast.dev.localhost:8000/?user_key=asdf' violates the following Content Security Policy directive: "connect-src 'self' localhost:3035 ws://localhost:3035". The action has been blocked.

Connecting to 'http://127.0.0.1:3001/transactions/authorize.xml?service_token=abc&service_id=2&user_key=123&usage%5Bhits%5D=1' violates the following Content Security Policy directive: "connect-src 'self' localhost:3035 ws://localhost:3035". The action has been blocked.

Good catch. These two are not due to webpack:dev, these happen always and need to be fixed because they are breaking the two API Docs screens in the admin portal. We can't really add the hosts to the whitelist because the values for backend host and per-service apicast host are computed in runtime, and apparently that's not accessible whenever the yaml file is processed. So I just added a wildcard *.

d355854

[mayorova]

Dev env seems to be working OK now.

A concern though - I see that by default the policies are enabled both for dev portal, and admin portal, in production.
I am a bit reluctant to do that, because I am not 100% sure we are not missing something (like payment gateways, or maybe some other external services that are being called).

I though that it was mainly an example for the customers to customize the config to their liking, and if they want they can enable it, but not enabling it by default... WDYT?

[jlledom]

I though that it was mainly an example for the customers to customize the config to their liking, and if they want they can enable it, but not enabling it by default... WDYT?

Fine for me, however, I think I'm gonna try the payment gateways and maybe modify the example config if required. I'll try also logo upload and invoice downloading.

Even if we leave it disabled by default, I think we must make our best to provide an example that really works OOB, otherwise that can be confusing for the user.

Do you know any other problematic features?

[mayorova]

Fine for me, however, I think I'm gonna try the payment gateways and maybe modify the example config if required. I'll try also logo upload and invoice downloading.

Yeah, these are good examples, because they interact with S3 potentially (if configured) and the payment gateways.

I have a feeling there might be something else... but on the other hand, I am not sure what 😬 that's why I am hesitant.

[akostadinov]

don't we have some internal way to access this, instead of env variable btw?

[jlledom]

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.

[mayorova]

Do you know any other problematic features?

Some other things Claude suggested:

• SSO integration (not sure though, as I think we just access the link directly, might not be affected)
• ReCAPTCHA (seems legit)

Then, I also remembered in on-prem we have service discovery, it allows pulling the service config from other namespaces in OpenShift, which have specific annotations, I think this might be potentially broken.

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