Fixes #23211 - Add PuppetCa TokenWhitelisting provider #592

juliantodt · 2018-06-21T13:54:07Z

This adds an alternative PuppetCa provider.
Instead of signing CSRs based on whitelist of hosts (directly via puppet), we will create a signed JWT and return it to foreman that will provision the token on the host so that it is included in the CSR the hosts sends to the PuppetCa. The PuppetCa will then use a script (rather than the textfile) (see puppet-foreman_proxy) that calls the SmartProxy with the CSR that will then validate the token inside it. This should be much more secure and easier to debug than the current hostname_whitelisting provider.
Note: For this to work, you will need #5730 in your foreman instance.

ekohl

How do we prevent concurrency issues with writing to the hosts file?

ekohl · 2018-06-25T10:55:50Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+    # Create a new token for a certname
+    def autosign certname
+      ensure_hostsfile
+      payload = { certname: certname, exp: Time.now.to_i + 10 * 60 * 60 }


Perhaps this time should be configurable. IMHO at least defined as a constant so you know what this magic value is.

Yes, I agree. Do you think 10 hours is a good default?

Currently we allow 6 hours in foreman core as a default (for build tokens). I'd suggest do use the same.

If Foreman has a setting for this, should we have it as an API parameter instead? You could still apply a default there but that way you would have a single setting.

That would mean that the API would be different for the providers, right?

I think an optional API parameter for the TTL would still be compatible enough. You may want to raise a 400 Bad Request on the old provider if a TTL is provided.

Raising an 400 on the old provider isn't a good idea, as foreman doesnt know which provider is used and will (if we add that) always include the TTL, meaning the old provider would not work. Will add the optional parameter.

That's a fair point. #585 is my take at solving this but I don't see this being integrated any time soon.

ekohl · 2018-06-25T10:58:11Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+      payload = { certname: certname, exp: Time.now.to_i + 10 * 60 * 60 }
+      token = JWT.encode payload, smartproxy_cert, 'RS512'
+      File.write hosts_file, autosign_list.push(certname).to_yaml
+      { generated_token: token }.to_json


Should the JSON representation be done in this function? IMHO it's something that you do in the API class

Moving the .to_json to the API class would mean that we might change the behaviour of the old provider.

ekohl · 2018-06-25T10:59:22Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+
+    def ensure_hostsfile
+      unless File.exist?(hosts_file)
+        FileUtils.mkdir_p hosts_file.rpartition('/').first


Isn't the rpartition('/').first equivalent to dirname()?

Yeah, sometimes I just remember the complicated ways to do thinks. Will change this.

ekohl · 2018-06-25T11:09:21Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_plugin.rb

+    plugin :puppetca_token_whitelisting, ::Proxy::VERSION
+
+    requires :puppetca, ::Proxy::VERSION
+    default_settings :sign_all => false, :hosts_file => '/tmp/foreman-proxy/hosts.yml'


I don't like this default because /tmp is a shared directory. Another user on the system can create /tmp/foreman-proxy prior to startup and have control. That makes it a security risk. We do have /var/lib/foreman-proxy in some packages (notably REX/Ansible I think) and perhaps we should standardize this. An alternative is doing it in /etc/foreman-proxy but I don't like it because it gives the impression users can/should modify it.

We also weren't sure where to put the file. Its not code, it's not config, it does not belong in the puppet dir. I settled for this as it is a temporary file, but am not quite happy, so I'm open to suggestions. You think /var/lib/foreman-proxy/ would be the best idea?

/tmp is wiped after a reboot. Losing that kind of state doesn't sound right for this file. /var/tmp would be better but to me it sounds like this is actually a state file. For that /var/lib is generally used.

ekohl · 2018-06-25T11:18:52Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+    include ::Proxy::Util
+
+    def sign_all
+      Proxy::PuppetCa::TokenWhitelisting::Plugin.settings.sign_all


I am still wondering if it should allow reading a whitelist from the actual autosign file but this might be enough.

I think falling back to the autosign.conf could be confusing. Allowing this for people who really really want to put the * in the file should be fine.

ekohl · 2018-06-25T11:22:18Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+    def autosign certname
+      ensure_hostsfile
+      payload = { certname: certname, exp: Time.now.to_i + 10 * 60 * 60 }
+      token = JWT.encode payload, smartproxy_cert, 'RS512'


I think the RS512 algorithm should be stored in a constant.

ekohl · 2018-06-25T11:23:59Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+      unless File.exist?(hosts_file)
+        FileUtils.mkdir_p hosts_file.rpartition('/').first
+        FileUtils.touch hosts_file
+        File.write(hosts_file, [].to_yaml)


I'd move writing to this file to a (private) helper function rather than duplicate it in 3 places.

ekohl · 2018-06-25T11:28:06Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+    # List the hosts that are currently valid
+    def autosign_list
+      ensure_hostsfile
+      YAML.load_file(hosts_file).to_a


Should safe_load be used? Especially if the default location is in /tmp there's a risk of executing arbitrary code as the foreman-proxy user as long as you can write anything in /tmp. Because of it I'd actually consider JSON rather than YAML. Since no user has to actually modify the file, it doesn't matter that it's not very human friendly.

timogoebel

@juliantodt: Thanks, I left some comments inline.

timogoebel · 2018-06-25T12:57:27Z

modules/puppetca/puppetca_api.rb

+    post "/validate" do
+      content_type :json
+      unless autosigner.respond_to?('validate_csr')
+        log_halt 401, "Provider only supports trivial autosigning"


401 is unauthorized, how about 406 or 412?

406 would be incorrect because that's on Accept headers. I'd consider HTTP 501 Not Implemented.

I was thinking Bad Request but that would have been 400. 501 would be fine for me.

timogoebel · 2018-06-25T12:58:28Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+        return false
+      end
+      if sign_all
+        logger.warn "Signing CSR without token verification."


Should be info I think.

We had that when we talked about the first SmartProxyPR #576 where witlessbird suggested warnings.

timogoebel · 2018-06-25T13:01:59Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+    def disable certname
+      hosts = autosign_list
+      hosts.delete(certname)
+      File.write hosts_file, hosts.to_yaml


I think we should extract this to a dedicated storage class.

timogoebel · 2018-06-25T13:08:10Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+    end
+
+    def smartproxy_cert
+      OpenSSL::PKey::RSA.new File.read Proxy::SETTINGS.ssl_private_key.to_s


What happens if the smart proxy is run without ssl?

Well, that would be a problem. I guess we would have to ensure that there is a RSA certificate there if used for SSL or not...

timogoebel · 2018-06-25T13:09:15Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+
+    # Invalidate a token based on the certname
+    def disable certname
+      hosts = autosign_list


I believe we should store the (in)valid tokens here instead of the certnames. If a token is lost, a user has no way of actually revoking it for good.

Agreed, I think storing valid tokens is the way to go. It allows us to do everything we do now (since we can always extract the certname from the tokens) while we still don't have to clean up periodically because the list would auto-clean itself + additionally we can revoke specific tokens manually.

ekohl · 2018-06-27T09:33:18Z

test/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner_test.rb

+    end
+    @autosigner = Proxy::PuppetCa::TokenWhitelisting::Autosigner.new
+    @autosigner.stubs(:hosts_file).returns(@file.path)
+    rsa_cert = OpenSSL::PKey::RSA.generate 2048


Does this mean a RSA cert is generated for every test in this file? Isn't that somewhat slow?

Not noticeable (at least for me). But since we decided to change the implementation to save the tokens instead of certnames, we will need to supply a fixtured cert anyway.

juliantodt · 2018-06-27T11:32:20Z

Updated:

Saving tokens instead of certnames
Moving TokenStorage to external class
Locking file during read/writing
Configurable TTL
Moved default location of tokens file

TODO:

Add tests for storage class
Handle case where SmartProxy doesnt use SSL
Fix tests, rubocop ^^

juliantodt · 2018-06-28T13:28:59Z

Updated. This should now satisfy all your concerns. Thanks @ekohl @timogoebel! Tests are now green as well.

Question: If we decide to add the token ttl to the foreman api call, we would break compatibility with old SmartProxy versions as they would not recognise the url, right?

timogoebel · 2018-06-28T13:44:54Z

Question: If we decide to add the token ttl to the foreman api call, we would break compatibility with old SmartProxy versions as they would not recognise the url, right?

I think we can live with that. We could check the smart proxy version in foreman and add a switch for that. Or just try again if we get a 404. @ekohl: WDYT?

ekohl · 2018-06-28T13:40:58Z

modules/puppetca/puppetca_api.rb

      content_type :json
-      certname = params[:certname]
+      certname = params['captures'][0]
+      ttl = params['captures'][2]


Isn't the TTL more of a body parameter in REST?

Yeah, we could make this a parameter and not part of the URL. That would make this even more backward compatible.

ekohl · 2018-06-28T13:50:35Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+        rescue JWT::ExpiredSignature
+          nil
+        end
+      end.reject { |certname| certname.nil? }


Isn't this equal reject equal to compact?

timogoebel

@juliantodt: Yet another round of comments. :-)

timogoebel · 2018-06-28T14:25:25Z

bundler.d/puppetca_token_whitelisting.rb

@@ -0,0 +1,7 @@
+group :puppetca_token_whitelisting do
+  if RUBY_VERSION < '2.1'
+    gem 'jwt', '1.5.6'


Can we use ~> 1.5.6?

timogoebel · 2018-06-28T14:26:57Z

modules/puppetca/puppetca_api.rb

      rescue => e
        log_halt 406, "Failed to enable autosign for #{certname}: #{e}"
      end
    end

+    post "/validate" do
+      content_type :json
+      unless autosigner.respond_to?('validate_csr')


Nit: I'd us a symbol here: respond_to?(:validate_csr)

timogoebel · 2018-06-28T14:38:59Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_token_storage.rb

+    end
+
+    def ensure_file
+      unless File.exist?(@tokens_file)


you can use a guard clause here, return of File.exists?(@token_file)

timogoebel · 2018-06-28T14:39:44Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_token_storage.rb

+      end
+    end
+
+    def locked_write content


I think the method name is misleading, as the write is not actually locked here.

juliantodt · 2018-07-02T13:08:04Z

Updated based on your comments, thank you!
Token TTL is now a body parameter which should make the foreman implementation backwards-compatible.

ekohl · 2018-07-02T13:25:58Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+
+      file = Proxy::PuppetCa::TokenWhitelisting::Plugin.settings.certificate
+      unless File.exist?(file)
+        File.write file, OpenSSL::PKey::RSA.generate(2048)


It's probably good to do this with a umask so it's not world readable (or some other way). The number of bits should also be at least a constant IMHO.

ekohl · 2018-07-02T13:28:22Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_token_storage.rb

+      end
+    end
+
+    def unsave_write content


ekohl · 2018-07-02T13:30:46Z

modules/puppetca_token_whitelisting/puppetca_token_whitelisting_autosigner.rb

+            true
+          end
+        end
+        storage.unsave_write tokens


Any reason this should be unsafe?

The whole block is already locked so nothing changes the file between read and write. You would not be able to safely write here, as that would try to lock the file again which is already locked.

I wonder if the storage should have an operation where you pass in a block to delete tokens. It feels a bit like you're doing storage operations in the signer here.

Added a TokenStorage::remove_if method, take a look.

juliantodt · 2018-07-27T11:31:52Z

As requested, I moved the puppet_sign script from the Puppet-Foreman_Proxy PR to here and added a couple of improvements.
@ekohl Is extra/puppet_sign.rb the correct place for it?

timogoebel · 2018-08-24T08:39:14Z

@ekohl: Any further comments?

ekohl

Overall it looks good and I'd be OK with merging. Just small edge cases you might want to look at.

ekohl · 2018-08-24T14:05:23Z

extra/puppet_sign.rb

+end
+SETTINGS         = YAML.load_file(settings_file)
+PUPPETCA         = YAML.load_file(SETTINGS[:settings_directory] + '/puppetca.yml')
+protocol         = PUPPETCA[:enabled] == true ? 'https' : PUPPETCA[:enabled]


Probably good if this also handles the value false and print a proper error

ekohl · 2018-08-24T14:14:02Z

modules/puppetca/puppetca_api.rb

+      end
+      begin
+        request.body.rewind
+        autosigner.validate_csr(request.body.read) ? 200 : 404


I'm wondering about the 404 code; will there be a need to distinguish between a proxy without this feature and one with? Probably not but it might be worth to consider 400 Bad Request for invalid CSRs.

Well the 404 here only gets returned when the CSR is valid but the token is not. A proxy without this feature returns 501 (line 43) and an invalid CSR raises an Exception leading to a 406 (line 49). Does that work for you?

timogoebel · 2018-08-27T09:30:08Z

@ekohl: Final ACK? You still "request changes" on this PR. :-)

timogoebel · 2018-08-27T10:45:01Z

Thanks, @juliantodt.

theforeman-bot added Needs testing Not yet reviewed labels Jun 21, 2018

juliantodt changed the title ~~Fixes #23211 - Add PuppetCa TokenVerify provider~~ Fixes #23211 - Add PuppetCa TokenWhitelisting provider Jun 21, 2018

juliantodt force-pushed the 23211_tokenverify branch from 5bf0c2b to 034eded Compare June 21, 2018 13:55

ekohl requested changes Jun 25, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jun 25, 2018

timogoebel requested changes Jun 25, 2018

View reviewed changes

ekohl reviewed Jun 27, 2018

View reviewed changes

juliantodt force-pushed the 23211_tokenverify branch from 034eded to f5636ed Compare June 27, 2018 11:29

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 27, 2018

juliantodt force-pushed the 23211_tokenverify branch 3 times, most recently from 0e2cdf4 to 289d77f Compare June 28, 2018 13:22

ekohl reviewed Jun 29, 2018

View reviewed changes

timogoebel requested changes Jun 29, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jun 29, 2018

juliantodt force-pushed the 23211_tokenverify branch from 289d77f to 2ea6d64 Compare July 2, 2018 13:06

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jul 2, 2018

ekohl reviewed Jul 2, 2018

View reviewed changes

juliantodt force-pushed the 23211_tokenverify branch from 2ea6d64 to dde2024 Compare July 2, 2018 14:03

juliantodt force-pushed the 23211_tokenverify branch from dde2024 to ac7198a Compare July 4, 2018 07:33

juliantodt mentioned this pull request Jul 5, 2018

Fixes #24012 - Add PuppetCA providers settings theforeman/puppet-foreman_proxy#433

Merged

juliantodt force-pushed the 23211_tokenverify branch from ac7198a to ef0e7bb Compare July 27, 2018 11:28

juliantodt force-pushed the 23211_tokenverify branch from ef0e7bb to 9e4a9ec Compare July 27, 2018 11:34

timogoebel approved these changes Aug 24, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Aug 24, 2018

ekohl reviewed Aug 24, 2018

View reviewed changes

Fixes #23211 - Add PuppetCa TokenWhitelisting provider

a7ac112

juliantodt force-pushed the 23211_tokenverify branch from 9e4a9ec to a7ac112 Compare August 27, 2018 07:17

ekohl approved these changes Aug 27, 2018

View reviewed changes

timogoebel merged commit 124af3d into theforeman:develop Aug 27, 2018

timogoebel mentioned this pull request Sep 13, 2018

refs #23211 - foreman-proxy: puppetca token whitelisting theforeman/foreman-packaging#2986

Merged

Fixes #23211 - Add PuppetCa TokenWhitelisting provider #592

Fixes #23211 - Add PuppetCa TokenWhitelisting provider #592

Conversation

juliantodt commented Jun 21, 2018

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliantodt commented Jun 27, 2018 • edited Loading

juliantodt commented Jun 28, 2018 • edited Loading

timogoebel commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliantodt commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliantodt commented Jul 27, 2018

timogoebel commented Aug 24, 2018

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel commented Aug 27, 2018

timogoebel commented Aug 27, 2018

timogoebel Jun 25, 2018 •

edited

Loading

juliantodt commented Jun 27, 2018 •

edited

Loading

juliantodt commented Jun 28, 2018 •

edited

Loading