Fixes #23799 - Refactor: Make PuppetCa pluggable #586

juliantodt · 2018-06-07T10:52:16Z

Since we are planning to offer alternative options for autosinging in the future (token based instead of hostname based) (see redmine issue) and want to allow users to choose their autosigning variant (primarily to allow compatibility with old versions, old foreman versions etc) we need to make the PuppetCA module pluggable, which means moving the autosinging functionality to a provider, which can then be swapped using the SmartProxy settings. We also want to clean up the module a bit and use dependency injections etc. The common logic for listing/signing/cleaning certificates (not autosign-entries) that uses the puppet cert command will remain in the puppetca-module.

This is btw an alternative approach to #576 which should allow for backwards compatibility and a compatible API between the autosigning variants.

timogoebel

@juliantodt: Thanks. Looks like solid work. I just left some small comments regarding naming. If they are resolved this looks ready to 🚢.

timogoebel · 2018-06-13T09:55:53Z

config/settings.d/puppetca.yml.example

@@ -2,7 +2,9 @@
 # Can be true, false, or http/https to enable just one of the protocols
 :enabled: false

+# valid providers:
+#   - puppetca_hostverify (verify CSRs based on hostnames)


Puppet calls this basic autosigning. I think we should stick with the term or use something like autosignfile.

I don't like basic autosigning because it says nothing about how it works, autosignfile is better, but because the token based approach will also use a file it is not clear which is which. The thing that will distinguish the providers is the attribute which is compared during autosigning which is certname vs. token.
So imho the best name would be puppetca_hostnamebasedautosigning (or certnamebased) but this is a pain to read.

How about HostnameWhitelisting?

I like that, but wouldn't we want to use snake_case?

timogoebel · 2018-06-13T09:59:28Z

modules/puppetca/puppetca_certmanager.rb


  class NotPresent < RuntimeError; end

-  class << self
+  class Certmanager


Can we find a better name for this? It's not clear what this is doing. Maybe something like CaInterface or CertificateAuthority?

timogoebel · 2018-06-13T10:02:02Z

modules/puppetca_hostverify/puppetca_hostverify_autosigner.rb

@@ -0,0 +1,57 @@
+module ::Proxy::PuppetCa::Hostverify
+  class Autosigner


Sorry to be so picky about the names, but what about FileBasedAutosign.

The token-based-autosigning-approach will also use a file, so that would not work. See above.

timogoebel · 2018-06-13T10:03:45Z

modules/puppetca/puppetca_api.rb

@@ -2,14 +2,18 @@

 module Proxy::PuppetCa
  class Api < ::Sinatra::Base
+    extend Proxy::PuppetCa::DependencyInjection
+    inject_attr :cert_manager, :cert_manager


I'd call this ca or ca_interface or something similar.

E.g. inject_attr :ca_provider, :ca reads a lot cleaner in my opinion.

Well ca is the whole thing, which consists of managing certificates and autosign-entries. Since the cert_manager basically uses the puppet cert command for everything it does, I thought this would be matching name.

I don't see autosigning as part of the CA. Basically you have two separate strings:

Interface to the CA (ask the ca to issue/remove/... a certificate, ...)

Interface to an autosigning provider (list whitelisted hosts, whitelist a host, ...). This is typically not part of the CA, but of the RA.

Certmanager is definitely misleading. Please use something else.

Well, its part of the puppetCA module and under the puppet/ca/ api endpoint...
But I can understand that that may be confusing, how about just puppet_cert so its obvious it uses the puppet cert command?

Ok, works for me.

timogoebel · 2018-06-13T10:04:30Z

modules/puppetca/puppetca_api.rb

@@ -2,14 +2,18 @@

 module Proxy::PuppetCa
  class Api < ::Sinatra::Base
+    extend Proxy::PuppetCa::DependencyInjection
+    inject_attr :cert_manager, :cert_manager
+    inject_attr :autosigner, :autosigner


How about just autosign?

I like to refer to objects with nouns, makes it better to read imho ^^

Ok, works for me.

timogoebel · 2018-06-13T10:08:25Z

test/puppetca_hostverify/puppetca_hostverify_config_test.rb

+require 'puppetca_hostverify/puppetca_hostverify'
+require 'puppetca_hostverify/puppetca_hostverify_plugin'
+
+class PuppetCAHostverifyConfigTest < Test::Unit::TestCase


Nit: Please stick either to Ca or CA. I'd prefer the former.

juliantodt

Thanks! Left some thoughts about the naming inline @timogoebel

juliantodt · 2018-06-13T10:26:36Z

config/settings.d/puppetca.yml.example

@@ -2,7 +2,9 @@
 # Can be true, false, or http/https to enable just one of the protocols
 :enabled: false

+# valid providers:
+#   - puppetca_hostverify (verify CSRs based on hostnames)


I don't like basic autosigning because it says nothing about how it works, autosignfile is better, but because the token based approach will also use a file it is not clear which is which. The thing that will distinguish the providers is the attribute which is compared during autosigning which is certname vs. token.
So imho the best name would be puppetca_hostnamebasedautosigning (or certnamebased) but this is a pain to read.

juliantodt · 2018-06-13T10:29:05Z

modules/puppetca/puppetca_api.rb

@@ -2,14 +2,18 @@

 module Proxy::PuppetCa
  class Api < ::Sinatra::Base
+    extend Proxy::PuppetCa::DependencyInjection
+    inject_attr :cert_manager, :cert_manager


Well ca is the whole thing, which consists of managing certificates and autosign-entries. Since the cert_manager basically uses the puppet cert command for everything it does, I thought this would be matching name.

juliantodt · 2018-06-13T10:31:10Z

modules/puppetca/puppetca_api.rb

@@ -2,14 +2,18 @@

 module Proxy::PuppetCa
  class Api < ::Sinatra::Base
+    extend Proxy::PuppetCa::DependencyInjection
+    inject_attr :cert_manager, :cert_manager
+    inject_attr :autosigner, :autosigner


I like to refer to objects with nouns, makes it better to read imho ^^

juliantodt · 2018-06-13T10:33:22Z

modules/puppetca_hostverify/puppetca_hostverify_autosigner.rb

@@ -0,0 +1,57 @@
+module ::Proxy::PuppetCa::Hostverify
+  class Autosigner


The token-based-autosigning-approach will also use a file, so that would not work. See above.

juliantodt · 2018-06-14T13:22:03Z

Updated the PR with the requested naming changes.

dgoetz · 2018-06-20T15:07:02Z

modules/puppetca/puppetca_api.rb

@@ -2,14 +2,18 @@



The line above has to be changed to require 'puppetca/puppetca_puppet_cert' as modules/puppetca/puppetca_main.rb is renamed to modules/puppetca/puppetca_puppet_cert.rb so it can load the feature.

puppetca/puppetca_puppet_cert is loaded via plugin_configuration::load_classes, so we don't need it here. This require can therefore be deleted.

You are right, deleting it works also.

juliantodt · 2018-06-21T11:49:05Z

Fixed @dgoetz. Added a migration for the settings, so that users who had set the autosignfile setting will not experience problems.

dgoetz

Tested the latest commit including the migration and it works.

timogoebel · 2018-06-21T13:32:44Z

Thanks, @juliantodt.

tbrisker · 2018-06-24T11:02:42Z

Looks like nightly bats tests are now failing with an error regarding PuppetCA, could it be related to this change? http://ci.theforeman.org/blue/rest/organizations/jenkins/pipelines/foreman-nightly-release/runs/248/nodes/34/steps/41/log/?start=0

dgoetz · 2018-06-25T06:42:19Z

@tbrisker: Yes, it is probably related. The proxy logs could be helpful.
@timogoebel, @juliantodt: Puppet code needs to be adjusted as it adds still autosign file to puppetca after installation and migration.

timogoebel · 2018-06-25T07:12:15Z

We already have a PR open to fix the puppet modules, see theforeman/puppet-foreman_proxy#433.
@juliantodt is going to address the comments today.

tbrisker · 2018-06-25T07:19:03Z

Thanks @juliantodt and @timogoebel! in the future, when something requires changes in multiple repos, please make sure all PRs are ready to merge first and are merged in coordination to avoid breakages such as this.

tbrisker · 2018-12-30T18:07:18Z

@juliantodt @timogoebel Care to update the manuals for 1.19 forward with this change? Looks like some users are stumbling on it - https://community.theforeman.org/t/puppetca-foreman-proxy-disabled-beacuse-of-autosign-issue/12290

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

timogoebel requested changes Jun 13, 2018

View reviewed changes

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

juliantodt commented Jun 13, 2018

View reviewed changes

juliantodt force-pushed the 23211_capluggable branch from 4ce8ee0 to e490faf Compare June 14, 2018 13:10

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

This was referenced Jun 20, 2018

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

Closed

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

Merged

dgoetz reviewed Jun 20, 2018

View reviewed changes

juliantodt force-pushed the 23211_capluggable branch from e490faf to 0dcd549 Compare June 21, 2018 11:45

juliantodt force-pushed the 23211_capluggable branch from 0dcd549 to 58e983b Compare June 21, 2018 11:51

Fixes #23799 - Refactor: Make PuppetCa pluggable

641f2e4

juliantodt force-pushed the 23211_capluggable branch from 58e983b to 641f2e4 Compare June 21, 2018 12:22

dgoetz approved these changes Jun 21, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 21, 2018

timogoebel approved these changes Jun 21, 2018

View reviewed changes

timogoebel added Ready to merge and removed Needs testing labels Jun 21, 2018

timogoebel merged commit e325683 into theforeman:develop Jun 21, 2018

dgoetz mentioned this pull request Jan 8, 2019

Update manual for Smart Proxy puppetca and the new providers theforeman/theforeman.org#1272

Merged

		@@ -0,0 +1,57 @@
		module ::Proxy::PuppetCa::Hostverify
		class Autosigner

Fixes #23799 - Refactor: Make PuppetCa pluggable #586

Fixes #23799 - Refactor: Make PuppetCa pluggable #586

Conversation

juliantodt commented Jun 7, 2018 • edited Loading

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

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

juliantodt 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 Jun 14, 2018

Choose a reason for hiding this comment

juliantodt Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliantodt commented Jun 21, 2018

dgoetz left a comment

Choose a reason for hiding this comment

timogoebel commented Jun 21, 2018

tbrisker commented Jun 24, 2018

dgoetz commented Jun 25, 2018

timogoebel commented Jun 25, 2018

tbrisker commented Jun 25, 2018

tbrisker commented Dec 30, 2018

juliantodt commented Jun 7, 2018 •

edited

Loading

juliantodt Jun 21, 2018 •

edited

Loading