Drop require_ssl_smart_proxies setting

Development RFCs
1

The short summary of this is that today there are 2 settings:

  • require_ssl - Force SSL is used for all non-unattended URLs
  • require_ssl_smart_proxies - Force SSL client certificates for Smart Proxies

My proposal is to drop these settings and make them mandatory (their current defaults)

Update: during this RFC it became clear there was still a use for the require_ssl.

Longer version

This was inspired by Rails 6.1 which removes the controller level SSL enforcement (deprecated in 6.0).

require_ssl setting

Let’s start with require_ssl. This can be turned off, in which case plain text HTTP is allowed. Today the installer defaults to true if ssl is on. While users can pass --foreman-ssl false, I don’t know how well this actually works.

Having fewer code paths, makes things easier. It’s also fairly standard to deploy applications using HTTPS nowadays.

Another benefit is that we can start to implement the HTTP → HTTPS redirect for non-unattended URLs in Apache instead of relying on Rails. While that’s out of scope for this PR and could technically already be done today, it’s worth pointing out.

It should be considered that many development environments don’t use HTTPS. That’s why I’m proposing to keep this limited to production environments (Rails.env.production?).

require_ssl_smart_proxies

If this setting is true (the default) The Smart Proxies authenticate using SSL client certificates.

If it’s false, other means can be used. Today reverse DNS can be used (foreman/smart_proxy_auth.rb at bde7047acb8a68a899f60585f751093c9713bb92 · theforeman/foreman · GitHub). I’d argue this is unsafe and we should not have this code at all.

For non-SSL we also have reverse DNS as a means (foreman/smart_proxy_auth.rb at bde7047acb8a68a899f60585f751093c9713bb92 · theforeman/foreman · GitHub). Perhaps this should be hidden behind !Rails.env.production? so developers can still use it in their development environments.

It should also be noted that prior to Feature #30779: Use ActionDispatch::RemoteIp when working as a reverse proxy - Foreman users could spoof the remote IP if they came from the local network. That in combination with reverse DNS checks means it’s easy to bypass these security checks. After this, it may actually be that the Katello HTTP reverse proxy setup that it ships opens this security risk. Removing the option for users to shoot themselves in the foot makes the project more secure.

Concrete proposal

  • We drop require_ssl_smart_proxies and make it always true.
  • Drop reverse DNS as a potential Smart Proxy authentication mechanism over SSL
  • Limit Smart Proxy authentication over HTTP to non-production mode

Update: the original RFC missed the word “Drop” in the second point. That caused some confusion.

2

I think it makes sense to do things this way. As an administrator, I would expect that the certificate configuration be transparent (i.e. it be explicit in the config which certs are being used and trusted).

In the concrete proposal, point 2 is reverse DNS as a potential smart proxy authentication mechanism - I read the proposal as a way to get away from that, though. Am I reading this correctly? (I’m a long time DNS guy, but “reverse DNS” and “authentication” give me hives, and NAT effectively breaks it, in addition to the other problems noted above).

This might be a longer term question, but assuming that the puppet stack at some point becomes optional in Foreman/Katello, has a framework been chosen for how to manage PKI without the puppetserver CA?

2 Likes
3

I indeed propose to get rid of it because I’m with you: while I like DNS, I get those same hives you mention. It’s in the current codebase (added in fixes #2121, #2069 - restrict importers and ENC to puppetmasters and … · theforeman/foreman@358ec5a · GitHub) but not reachable in the default configuration.

This is a bit off topic, but code wise there is no dependency on Puppet’s CA stack. We’ve always used it because historically Foreman was only deployed in environments where it was already present, so it was convenient. However, there is no code that actually ties it to Puppet. It’s just standard PKI and if you provide the right files in the right places with the right permissions, you can use any CA. We’ve just never really documented it and that makes it tricky to get right. In Installation of 3.1 without Puppet fails - #4 by ekohl I did show what I think should work, but it doesn’t talk about permissions. In my experiments I made a foreman-certs group and used --foreman-user-groups foreman-certs --foreman-proxy-groups foreman-certs. Technically you can use Let’s Encrypt as well but for authentication I would be cautious since wildcard certificates do make it less secure (Foreman supports that, Foreman Proxy doesn’t)

4

I’m all in favor of dropping the hacky solution and force SSL only in production.
We should remember to allow full HTTP in development tho, as you’ve mentioned.

But I need to agree, that we are making it difficult to setup full SSL with Puppet being optional. You’re saying we are not tied to PuppetCA, which is nice, but I as a User had have nice default and didn’t need to care about anything, installer set up everything for me. Now I as a User am strugling if I opt out of Puppet. Remember that we are supporting opt out now, so it is supported to not have Puppet. I believe there should continue to be reasonable default for me as a User who opts out from Puppet and I should not make bunch of certificates to appear on specific spot on disk for Foreman to install itself :slight_smile:

5

I’d like to keep the PuppetCA part out of this discussion, but I’ll just leave you with this as a thought: what is a good replacement? As you can see from RFC: Redesign Certificate Handling within Foreman Deployments there are a lot of details to work out.

6

I agree its not easy and I do not want to make it a prerequisite, just saying we should try hard to get that done :slight_smile:

7

This means that we always needs something in front of Foreman that sets the X-Forwarded-Proto: HTTPS (or whatever is exactly used for detection) header, right?

Background: Our Dockerfile explicitly starts Foreman with RAILS_ENV=production, but is often used in test-like setups where there is no reverse proxy in front of it.

8

That’s a valid point. Some thoughts:

HTTPS in containers is painful (well, maybe just in general). You probably don’t want the container itself to terminate it, but it would need client certificates if you want to authenticate to Smart Proxies. You also need the server certificates for the websockify process to expose compute consoles (Consolidating The Console could solve that). Technically Puma could also terminate HTTPS directly. So really, it’s mostly an unsolved problem and I’m not sure how much we need to account for it now.

That said, while reading through the Foreman source and tests, it actually looks like it may be useful to have something to toggle rather than stubbing Rails.env.production? (which is really blunt). So perhaps keeping require_ssl is not such a bad idea. It’s not used in many places and only shows up in the YAML files.

9

Just for the record, the latter change was merged yesterday and it broke few plugins. There is a possibility that this broke more, maybe we should issue a warning for plugin devs:

foreman_salt/test/functional/minions_controller_test.rb
9:      Setting[:require_ssl_smart_proxies] = false

foreman_puppet/test/controllers/foreman_puppet/hosts_controller_test.rb
204:        Setting[:require_ssl_smart_proxies] = false
214:        Setting[:require_ssl_smart_proxies] = false
224:        Setting[:require_ssl_smart_proxies] = true
237:        Setting[:require_ssl_smart_proxies] = true
251:        Setting[:require_ssl_smart_proxies] = true
265:        Setting[:require_ssl_smart_proxies] = true
279:        Setting[:require_ssl_smart_proxies] = true
291:        Setting[:require_ssl_smart_proxies] = true
300:      test 'when "require_ssl_smart_proxies" and "require_ssl" are true, HTTP requests should not be able to get externalNodes' do
303:        Setting[:require_ssl_smart_proxies] = true
311:      test 'when "require_ssl_smart_proxies" is true and "require_ssl" is false, HTTP requests should be able to get externalNodes' do
313:        # since require_ssl_smart_proxies is only applicable to HTTPS connections, both should be set
315:        Setting[:require_ssl_smart_proxies] = true
325:        Setting[:require_ssl_smart_proxies] = true
335:        Setting[:require_ssl_smart_proxies] = true

foreman_host_reports/test/controllers/api/v2/host_reports_controller_test.rb
130:      Setting[:require_ssl_smart_proxies] = false
144:      Setting[:require_ssl_smart_proxies] = false
158:      Setting[:require_ssl_smart_proxies] = true
174:      Setting[:require_ssl_smart_proxies] = true
190:      Setting[:require_ssl_smart_proxies] = true
204:    test 'when "require_ssl_smart_proxies" and "require_ssl" are true, HTTP requests should not be able to create a report' do
206:      Setting[:require_ssl_smart_proxies] = true
219:    test 'when "require_ssl_smart_proxies" is true and "require_ssl" is false, HTTP requests should be able to create reports' do
220:      # since require_ssl_smart_proxies is only applicable to HTTPS connections, both should be set
222:      Setting[:require_ssl_smart_proxies] = true
10

That reminds me: I need to revisit How to track developer changes and move forward on that.

11

Should we have deprecated the setting instead?

12

What’s done is done, no looking back. Perhaps just open a ticket for salt guys to know why their tests will fail.

I am going to finally look on the other patch (which is a similar situation).

13

I agree with all points, however, have you considered relaxing the require_ssl_smart_proxies for Rails development environment? This could ease initial development setup quite a bit for newcomers.

It looks like no plugins I have checked out have require_ssl, except foreman_host_reports which I will fix.

So we can merge this one too.

14

No. As the patch was merged now I think it doesn’t make sense to relax it. Either you have SSL and SSL must be used to authenticate or you don’t have SSL and then it falls back to DNS. I don’t see a valid use case where you went through the trouble of setting up SSL but then don’t use it.

15

Oh right. Do we want to remove this feature? I mean, we should probably test for Rails.env.development? and if not, simply an error would stop users from using this.

16

So regarding the proposals, does the first proposal still hold? My impression is, it does not. I suppose the proposal 2 and 3 were merged and broke plugins, right?

17

An update to this PR:

This has been merged:

I now reread this and can better understand @mhjacks’ confusion: I missed the word Drop. I’ll modify the original RFC to reflect this.

This has been merged in the same PR as dropping the setting.

This is partially implemented in:

The actual implementation changes from allowing the request and then send a forbidden to a blanket redirect to HTTPS. The Smart Proxy generally looks at the status codes (I checked most code) and expects 2xx, otherwise it fails. So it should handle the 3xx codes. And in general, when the Smart Proxy is configured to connect over HTTP, it’s generally a misconfiguration.

To be explicit, I think you’re talking about the “drop require_ssl setting and make it mandatory”-part. I also think that during the RFC I’ve come to the conclusion that there’s still a valid use case for this and there’s no need to further pursue this. However, require_ssl_smart_proxies has been dropped.

To be clear, they broke the plugin tests because they had explicit test cases for when require_ssl_smart_proxies was false. I should have better prepared for this since I had done a quick check on some plugins and I’ll try to do better next time.

With all that, I’m going to update the RFC to reflect the final state and mark it as resolved. Thanks everyone involved!

1 Like
18

Just to be sure: so when I do switch off require_ssl but do have reverse proxy that sends in HTTP requests, those are considered SSL? If not, who would then validate smart_proxy client cert? Isn’t that a valid use case for require_ssl = false and require_ssl_smart_proxies = true?