[WIP] Fixes #23211 - Token based PuppetCA autosigning #576

juliantodt · 2018-04-18T10:22:55Z

Removes old autosigning endpoints and adds new ones
that take a incoming CSR from puppet, extract the token
and forward it to foreman for verfication.

See also PRs in foreman-core, community-templates, puppet-foreman_proxy & puppet-puppet.

We would like to see some feedback early on while we are testing this thoroughly.

dmitri-d · 2018-04-18T18:34:56Z

Please do not remove old api endpoints, log a warning about them being deprecated instead. We usually keep api for one or two releases after the deprecation announcement.

dmitri-d · 2018-04-18T19:03:07Z

modules/puppetca/puppetca_api.rb

-    get "/autosign" do
-      content_type :json
+    post "/autosign" do
+      params = JSON.parse(request.env["rack.input"].read)


Since the api is being changed, why not change it to something like:

post "/autosign/:csr" instead of JSON encoding and then decoding a single parameter.

I'm pretty sure that CSRs are not URL-safe, as they include slashes and line-breaks.

You could UR-encode the parameter? Alternatively, instead of putting the parameter in the url, you could put it in the body of the request (would still need to be url-encoded though), which is a bit cleaner.

dmitri-d · 2018-04-18T19:05:58Z

modules/puppetca/puppetca_csr.rb

+      @csr.attributes.map do |attr|
+        {
+          oid: attr.oid,
+          value: attr.value.value.first.value


This looks fragile: are all of the intermediate calls guaranteed to not return nils?

I think this should be fine. Openssl is not known for its nice code.

Yeah, would probably be better to catch possible errors from this.

dmitri-d · 2018-04-18T19:11:55Z

modules/puppetca/puppetca_main.rb

+        logger.debug "Request did not include a CSR"
+        return false
+      end
+      return true if signall


Is signall needed? If so, please put warnings both when it's triggered and during initialization.

signall is the equivalent of putting * in your autosign.conf at the moment. As there are currently people using this it was a requested feature, so we're adding this as a setting. I don't think we need warnings for this as it is disabled by default and only is used if you manually update the settings, so you need to know what you're doing.

I think a warning would be a good thing -- people forget to switch things back all the time. This flag completely circumvents token authorization, a warning would be a nice reminder (if verbose).

dmitri-d · 2018-04-18T19:24:03Z

modules/puppetca/puppetca_main.rb

-        v =~ /^\s*#.*|^$/ ## Remove comments and empty lines
-      end.map do |v|
-        v.chomp ## Strip trailing spaces
+    def foreman_callback token


Could you use something more descriptive for the method name, something like csr_recognised?

dmitri-d · 2018-04-18T19:26:27Z

modules/puppetca/puppetca_main.rb

+    def foreman_callback token
+      logger.debug "Calling foreman with token '#{token}'"
+      fr = Proxy::HttpRequest::ForemanRequest.new
+      req = fr.request_factory.create_delete("api/puppetca_token", :id => token)


I would suggested splitting the check and cleanup into two separate actions: not only removal of data on check not something that is expected, but it might make sense to keep the token until the host has been successfully created.

Removing the token is part of the design. For security reasons, it should just be able to use the token once.

In what scenario do you think would it make sense to still have the token after the CSR was verified with it? If anything goes wrong during the verification, the token will not be deleted, so we just deleted it when the host has his cert signed, when we don't need the token anymore.

It felt to me that this is a call with a (big) side-effect, which also broke the pattern of how we use "delete" method in smart-proxy.

If you renamed foreman_callback to something like delete_token, this would resolve the whole side-effect issue.

dmitri-d · 2018-04-18T19:26:51Z

modules/puppetca/puppetca_main.rb

+          return true
+        end
+      rescue => e
+        logger.debug "Error while calling foreman: " + e.to_s


Please log this at the error level.

Will change that. Do you think that the other logs are correct on debug? Considering you normally don't run it on debug-loglevel, you won't be able to trace back why a CSR wasn't verified if you wanted.

I'm ok either way, but perhaps a warning would be a bit more helpful.

dmitri-d · 2018-04-18T19:32:49Z

I would also suggest replacing all class-methods in Proxy::PuppetCa with instance ones in a dedicated class, and use dependency injection to configure the module/api controller (please see dhcp, dns, or other modules for examples. Also see http://projects.theforeman.org/projects/foreman/wiki/How_to_Create_a_Smart-Proxy_Plugin#How-to-Load-Dependencies).

timogoebel · 2018-04-19T06:59:57Z

lib/proxy/request.rb

@@ -20,6 +20,12 @@ def create_get(path, query={}, headers={})
      req
    end

+    def create_delete(path, query={}, headers={})
+      req = Net::HTTP::Delete.new(uri(path).path + "/" + query[:id])


tap might come in handy here.

It's the same as in all the other functions there...

timogoebel · 2018-04-19T07:02:30Z

modules/puppetca/puppetca_csr.rb

+      @csr.attributes.map do |attr|
+        {
+          oid: attr.oid,
+          value: attr.value.value.first.value


I think this should be fine. Openssl is not known for its nice code.

timogoebel · 2018-04-19T07:03:12Z

modules/puppetca/puppetca_main.rb

-        autosign.puts certname unless found
-      end
-      logger.debug "Added #{certname} to autosign"
+    def signall


Can we rename this to sign_all? I always read it as signal and start wondering where the second l comes from.

timogoebel · 2018-04-19T07:04:11Z

modules/puppetca/puppetca_main.rb

+    def foreman_callback token
+      logger.debug "Calling foreman with token '#{token}'"
+      fr = Proxy::HttpRequest::ForemanRequest.new
+      req = fr.request_factory.create_delete("api/puppetca_token", :id => token)


Removing the token is part of the design. For security reasons, it should just be able to use the token once.

juliantodt · 2018-04-19T09:56:19Z

@witlessbird
Removing old API-Endpoints: Puppet use the autosign.conf XOR the autosign-script. When we change to the script, the autosign.conf will be useless and with it all functions editing it. Sure we could keep the API endpoints and log warnings but they will have no effect aswell.

Outsourcing Methods: I will take a look at this.

Also some comments inline.
Could you take a look at the test? I don't think the fails are related to my changes, are they?

timogoebel · 2018-04-19T09:58:48Z

Could you take a look at the test? I don't think the fails are related to my changes, are they?

Don't worry, [test] failures are unrelated.

dmitri-d · 2018-04-19T17:33:18Z

modules/puppetca/puppetca_main.rb

+      req = fr.request_factory.create_delete("api/puppetca_token", :id => token)
+      begin
+        response = fr.send_request(req)
+        if response.code.to_i == 204


I'd say this is too specific, any 2xx would be ok?

Since we write the Foreman-API at the same time, why not be specific?

Foreman side of things can change which would require this code to be updated. Since we don't care about particulars of the response as long as it is an "ok" (aka 2xx) one, making it too specific makes it more fragile.

timogoebel

@juliantodt: Thanks. Some more comments that came up when I read the code. The DI stuff looks good now.

timogoebel · 2018-05-02T15:01:37Z

modules/puppetca/puppetca_certmanager.rb

+    # parameter is csr to use
+    def autosign csr
+      if csr.nil?
+        logger.warn "Request did not include a CSR"


Nit: missing .

timogoebel · 2018-05-02T15:03:07Z

modules/puppetca/puppetca_csr.rb

@@ -0,0 +1,25 @@
+module Proxy::PuppetCa
+  class CertificateRequest


The class name should match the name of the file.

dmitri-d · 2018-05-02T18:01:05Z

Removing old API-Endpoints: Puppet use the autosign.conf XOR the autosign-script. When we change to the script, the autosign.conf will be useless and with it all functions editing it. Sure we could keep the API endpoints and log warnings but they will have no effect aswell.

If old behavior cannot be preserved at all, then all of these changes must reside in a new module/provider.

timogoebel · 2018-05-03T06:23:12Z

If old behavior cannot be preserved at all, then all of these changes must reside in a new module/provider.

@witlessbird: Why is that? Do we want to continue to support the old behavior so that users can use a newer smart-proxy version with an older Foreman version? Or is this meant for all the standalone users who use smart-proxy without Foreman?

Removes old autosigning endpoints and adds new ones that take a incoming CSR from puppet, extract the token and forward it to foreman for verfication.

juliantodt · 2018-05-03T11:39:46Z

Took care of all the comments as well as the test failures.

@witlessbird I can see that removing the old functionality might seem problematic, but it will be same with the updates to the foreman-core and I don't think we need to support older foreman versions with newer smart-proxy versions. Also allowing both signing options to be chosen by the user (-> will need to allow this in the foreman-core as well), will just cause problems like foreman & smart-proxy using tokens but puppet is configured to use the autosign.conf. When we are strict with these new changes and remove old functions we get obvious error messages at misconfigurations and the solution will be to just update to the latest version. I know it's not perfect to make these changes that affect multiple components that are backwards-incompatible, but I prefer it over the alternatives.

ekohl · 2018-05-03T12:07:12Z

I agree with @witlessbird that the API shouldn't change behavior of endpoints. If it changes, then it should use other URLs so Foreman gets a clear error. There are good reasons for this: until now we've always been flexible with versions. Running a 1.17 proxy with a 1.16 foreman worked, but also a 1.16 proxy with a 1.17 foreman. We know there are users with many proxies and upgrading them all at once is not feasible.

That said, I think there's still room for a compatible API. The one that's currently changed is GET /autosign. It could return an empty list.

dmitri-d · 2018-05-03T15:08:06Z

What @ekohl said. There is also a number of people who use smart-proxy api for integration with their own software -- they'll be affected by such a break too.

juliantodt · 2018-05-07T11:47:35Z

@witlessbird @ekohl So what you're saying is that you don't necessarily have a problem with removing old endpoints in favour of new ones but with keeping some and changing how they work. So change the name of the /autosign endpoint and done?

dmitri-d · 2018-05-07T17:56:39Z

@juliantodt: not quite. Old endpoints have to keep names and preserve functionality for a couple releases after being declared as deprecated.

If new functionality is being added to an existing end-point, the old behaviour must also be preserved.

timogoebel · 2018-05-08T08:24:01Z

@juliantodt: It looks like there is a lot of resistance in getting this in, which is unfortunate as we did ask if there were any concerns before starting the implementation. I'd suggest to split this up and try to get the refactorings in (moving the code to a provider) so the puppetca api is pluggable. Maybe we can then either support both providers or move this to a plugin.

juliantodt · 2018-06-20T09:41:04Z

Closing this in favour of the pluggable, backwards compatible approach #586.

theforeman-bot added Needs testing Not yet reviewed labels Apr 18, 2018

juliantodt force-pushed the 23210-puppetca branch 2 times, most recently from 178c986 to cde95ae Compare April 18, 2018 10:57

This was referenced Apr 18, 2018

Refs #23210 - Provision PuppetCA-Token on Host theforeman/community-templates#475

Merged

[WIP] Fixes #23210 - Token based PuppetCA autosigning theforeman/foreman#5465

Closed

dmitri-d reviewed Apr 18, 2018

View reviewed changes

timogoebel reviewed Apr 19, 2018

View reviewed changes

juliantodt mentioned this pull request Apr 19, 2018

Fixes #23477 - Add puppet user to foreman-proxy group theforeman/puppet-foreman_proxy#425

Closed

dmitri-d reviewed Apr 19, 2018

View reviewed changes

juliantodt mentioned this pull request Apr 26, 2018

[WIP] Refs #23477 - Set PuppetCA-signscript as default theforeman/puppet-puppet#591

Closed

juliantodt force-pushed the 23210-puppetca branch from cde95ae to 141c451 Compare May 2, 2018 10:52

timogoebel reviewed May 2, 2018

View reviewed changes

Fixes #23211 - Token based PuppetCA autosigning

f693783

Removes old autosigning endpoints and adds new ones that take a incoming CSR from puppet, extract the token and forward it to foreman for verfication.

juliantodt force-pushed the 23210-puppetca branch from 141c451 to f693783 Compare May 3, 2018 11:27

juliantodt mentioned this pull request Jun 7, 2018

Fixes #23799 - Refactor: Make PuppetCa pluggable #586

Merged

juliantodt closed this Jun 20, 2018

juliantodt mentioned this pull request Jun 25, 2018

Fixes #23211 - Add PuppetCa TokenWhitelisting provider #592

Merged

		@@ -0,0 +1,25 @@
		module Proxy::PuppetCa
		class CertificateRequest

[WIP] Fixes #23211 - Token based PuppetCA autosigning #576

[WIP] Fixes #23211 - Token based PuppetCA autosigning #576

Conversation

juliantodt commented Apr 18, 2018 • edited Loading

dmitri-d commented Apr 18, 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

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

dmitri-d commented Apr 18, 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

juliantodt commented Apr 19, 2018

timogoebel commented Apr 19, 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

dmitri-d commented May 2, 2018

timogoebel commented May 3, 2018

juliantodt commented May 3, 2018

ekohl commented May 3, 2018

dmitri-d commented May 3, 2018

juliantodt commented May 7, 2018 • edited Loading

dmitri-d commented May 7, 2018

timogoebel commented May 8, 2018

juliantodt commented Jun 20, 2018

juliantodt commented Apr 18, 2018 •

edited

Loading

juliantodt commented May 7, 2018 •

edited

Loading