-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixes #25065 - use puppetca http api #615
Conversation
cc: @quba42 |
aa0f5f2
to
4ceaae7
Compare
Yesterday I added a lot of tests and refactored the implementation to providers. I also added a settings migration. |
For reference: This is the (current) documentation of the API endpoint. https://puppet.com/docs/puppet/6.0/http_api/http_certificate_status.html |
4ceaae7
to
e11f5b3
Compare
Would it be possible to get an indication of when this fix will be released? |
@trenta: Would you be able to assist with testing this? |
I'd like to but probably a bit beyond my current skill set. I could do some testing if that helps? |
@trenta: You should be able to apply this patch to an rpm deployment by something like this:
Let us know if this works for you. |
@timogoebel Thanks for that. I've tried applying the patch and receive the following:
|
@trenta: Just ignore the files it cannot find. The example files do not exist on a production deployment. Tests will be missing as well. You can skip them, just press enter a couple of times. :-D |
@timogoebel After patching and restarting, I refreshed the smart-proxy features and got the following error.
The relevant line in /etc/foreman-proxy/settings.d/puppet.yml is: I’m using the puppet module https://forge.puppet.com/theforeman/foreman_proxy to manage this. And for more info I’m running Ubuntu 18.04 |
@trenta: You need to also set this in |
@timogoebel I got it working thanks. One thing I noticed and I'm not sure how much of a problem it would be but when I went to rebuild a machine that didn't have a certificate in the ca it failed and wouldn't go into boot mode. Deleting the machine had the same error. The message was
|
@timogoebel That certificate deletion issue if there isn't one is a problem. If you initiate a build, make a change, cancel build and try to build again it tries to remove the certificate that isn't there and because it fails it doesn't go into build mode. |
@trenta: Thanks for testing this. This is a huge help. If I see it correctly, the issue is that the API returns "404" when we clean the certificate. Our code should probably simply accept this instead of throwing an error. |
e11f5b3
to
b8d1d5c
Compare
@trenta: Pushed a version that contains a fix for the issue you have found. Do you mind to retest? You can revert the first version using patch and then apply the new version. |
@timogoebel Yep that patch fixes that problem. Nice work! Thanks for your work. |
@timogoebel I have most of the Puppet 6 support in the installer (critically theforeman-puppet) so I intend to test this soon and work on the installer configuration aspect. |
@timogoebel When trying to test this I ran into an issue from puppet |
@juliantodt: That's very interesting. Thanks for finding this. Do you have a recommendation on how to fix this? |
@timogoebel I would consider this a bug in the puppet-ca-API of the puppetserver - so no fixing this on the smartproxy's side. I would test this with puppet 6. If it works there then set the new provider as default only for puppet_version >= 6 and keep the old provider for the older puppet versions. If it also doesn't work on puppet 6, open an issue with puppet. I don't think there is a lot we can do about that. |
|
||
use_provider = settings[:use_provider] | ||
use_provider = [use_provider].compact unless use_provider.is_a?(Array) | ||
use_provider << (settings[:puppet_version].to_s >= '4.6' ? :puppetca_http_api : :puppetca_puppet_cert) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change this 6.0 per @juliantodt's suggestion? I'd comfortable merging this after that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekohl: Done.
When the patch is at this stage of testing is it still the same method to apply it? wget -O /tmp/615.patch https://github.com/theforeman/smart-proxy/pull/615.patch I don't seem to be able to get it working again after the latest upgrade |
Sorry missed the step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.21 branching is behind us so i'm fine with merging this now (though i did not test it). @ekohl i believe your concerns have been addressed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's on my agenda to test this
#:sudo_command: /usr/bin/sudo | ||
|
||
# Puppet version used | ||
#:puppet_version: 4.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this needs quotes? Otherwise it'll be a float and 4.10
will be 4.1
.
|
||
use_provider = settings[:use_provider] | ||
use_provider = [use_provider].compact unless use_provider.is_a?(Array) | ||
use_provider << (settings[:puppet_version].to_s >= '6.0' ? :puppetca_http_api : :puppetca_puppet_cert) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparison using Gem::Version
is probably better so we can deal with 4.10.1
as well
response = client.search | ||
response.each_with_object({}) do |entry, hsh| | ||
name = entry['name'] | ||
# serial, not_before and not_after are not available via http api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like they will be. Should we already start reading them anyway so we'll support it when it is? Should return nil if it's not found either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait until a version is released that contains this data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just merged so I think we can start assuming the field names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't add any new features to this PR. Let's merge this first.
We have to make sure our code can handle the old and new versions, so we need appropriate tests etc. I'd prefer to do this later. Keep in mind this PR was opened in October, we have user testing, unit tests, code reviews. What is keeping us from merging?
assert_equal 'puppetca_hostname_whitelisting', Proxy::PuppetCa::Plugin.settings.use_provider | ||
end | ||
|
||
def test_set_puppet_cert_ca_provider_for_4_2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you copy this test and add 4.10.1
as well? Our installer uses the puppetversion
fact which is a full version.
cec48ea
to
e73bcdc
Compare
@ekohl: Thanks for reviewing. Addressed your comments. The lines you commented about are actually copy-and-paste-code, we also have it in other places. |
I've tried this using forklift: centos7-foreman-puppet-6:
box: centos7-foreman-nightly
ansible:
variables:
# foreman-proxy with PR 615
koji_task_ids:
- 172818
foreman_installer_module_prs:
- theforeman/foreman_proxy/488
# Needed for puppet-systemd: https://github.com/camptocamp/puppet-systemd/pull/97
foreman_installer_options:
- "--skip-puppet-version-check"
puppet_repositories_version: 6 This applies two few PRs: Initial observation are UI bugs on the smart proxy detail page: theforeman/foreman#6494 fixes the iso8601 issue. Then I see the following: If you select I haven't had time to provision nodes yet. On a side note: Kafo needs some Puppet 6 love in the progress bar. |
After a bit of research it looks like the possible states are I'd propose we map Whether this should block the merge, I'm not sure. |
As well as mapping |
@timogoebel I was thinking about something like ekohl@a7c682b but I don't know if it's really correct since I can't find how the old way handled expired certs. |
@ekohl: Thanks for testing this! The code you sent me is an excellent starting point. If you can send me some data I can use for fixtures (basically a api response for 6.3.0 is fine), I should be able to write tests for this tomorrow. I won't have time or connectivity to set up a proper development environment myself 🚆 . Hope we can get this in after that. |
If the cert is already in the Puppetserver logs the following.
|
Fortunately, I can confirm that calling |
@alexjfisher: We should be able to mitigate that by allowing |
@timogoebel Yes, I've just tested that change, and it works great now. |
Amended these changes:
f0218175 ~/foremandev/smart-proxy (25065-puppetca-http-api) $ git diff
diff --git a/modules/puppetca_http_api/ca_v1_api_request.rb b/modules/puppetca_http_api/ca_v1_api_request.rb
index 3aa4e69..84976ce 100644
--- a/modules/puppetca_http_api/ca_v1_api_request.rb
+++ b/modules/puppetca_http_api/ca_v1_api_request.rb
@@ -12,7 +12,7 @@ module ::Proxy::PuppetCa::PuppetcaHttpApi
end
def clean(certname)
- handle_response(put_data("/puppet-ca/v1/certificate_status/#{certname}", 'desired_state' => 'revoked'), [Net::HTTPNoContent, Net::HTTPNotFound], 'Failed to set Puppet CA certificate_status v1 API')
+ handle_response(put_data("/puppet-ca/v1/certificate_status/#{certname}", 'desired_state' => 'revoked'), [Net::HTTPNoContent, Net::HTTPNotFound, Net::HTTPConflict], 'Failed to set Puppet CA certificate_status v1 API')
handle_response(delete("/puppet-ca/v1/certificate_status/#{certname}"), [Net::HTTPNoContent, Net::HTTPNotFound], 'Failed to delete Puppet CA certificate_status v1 API')
end
diff --git a/modules/puppetca_http_api/puppetca_impl.rb b/modules/puppetca_http_api/puppetca_impl.rb
index 9ec3476..3fcaf0f 100644
--- a/modules/puppetca_http_api/puppetca_impl.rb
+++ b/modules/puppetca_http_api/puppetca_impl.rb
@@ -17,15 +17,39 @@ module ::Proxy::PuppetCa::PuppetcaHttpApi
response.each_with_object({}) do |entry, hsh|
name = entry['name']
# serial, not_before and not_after are not available via http api
+ # for puppetserver < 6.3
# see https://tickets.puppetlabs.com/browse/SERVER-2370
hsh[name] = {
- 'state' => entry['state'],
+ 'state' => normalized_state(entry),
'fingerprint' => entry['fingerprint'],
- 'serial' => nil,
- 'not_before' => nil,
- 'not_after' => nil
+ 'serial' => entry['serial_number'],
+ 'not_before' => entry['not_before'],
+ 'not_after' => entry['not_after']
}
end
end
+
+ private
+
+ # Normalize the state to match the Puppet < 6 command line values since
+ # that's what Foreman expects.
+ #
+ # Puppet CA API sends: requested, signed, revoked
+ # Foreman expects: pending, valid, revoked
+ def normalized_state(entry)
+ case entry['state']
+ when 'signed'
+ # Versions before puppetserver 6.3 do not send not_after
+ if entry['not_after'] && Time.parse(entry['not_after']) > Time.now
+ 'revoked'
+ else
+ 'valid'
+ end
+ when 'requested'
+ 'pending'
+ else
+ entry['state']
+ end
+ end
end
end
diff --git a/test/puppetca_http_api/ca_v1_api_request_test.rb b/test/puppetca_http_api/ca_v1_api_request_test.rb
index 398fae4..a302753 100644
--- a/test/puppetca_http_api/ca_v1_api_request_test.rb
+++ b/test/puppetca_http_api/ca_v1_api_request_test.rb
@@ -37,6 +37,17 @@ class CaApiv1RequestTest < Test::Unit::TestCase
assert_nil @client.clean('puppet.example.com')
end
+ def test_clean_with_certs_in_requested_state
+ stub_request(:put, 'https://puppet:8140/puppet-ca/v1/certificate_status/puppet.example.com').
+ with(:body => "{\"desired_state\":\"revoked\"}").
+ to_return(:status => 409, :body => 'Cannot revoke certificate for host puppet.example.com without a signed certificate')
+
+ stub_request(:delete, 'https://puppet:8140/puppet-ca/v1/certificate_status/puppet.example.com').
+ to_return(:status => 204)
+
+ assert_nil @client.clean('puppet.example.com')
+ end
+
def test_search
stub_request(:get, 'https://puppet:8140/puppet-ca/v1/certificate_statuses/foreman').
to_return(:status => 200, :body => fixture('ca_search.json'))
diff --git a/test/puppetca_http_api/puppetca_http_impl_test.rb b/test/puppetca_http_api/puppetca_http_impl_test.rb
index 87ea1be..5058829 100644
--- a/test/puppetca_http_api/puppetca_http_impl_test.rb
+++ b/test/puppetca_http_api/puppetca_http_impl_test.rb
@@ -26,8 +26,32 @@ class PuppetCaHttpImplTest < Test::Unit::TestCase
}
]
end
- end
# rubocop:enable Metrics/LineLength
+ end
+
+ class FakeCaApiV1Request63 < FakeCaApiV1Request
+ # rubocop:disable Metrics/LineLength
+ def search(key = 'foreman')
+ [
+ {
+ 'name' => 'puppet.example.com',
+ 'state' => 'signed',
+ 'dns_alt_names' => ['DNS:puppet', 'DNS:puppet.example.com'],
+ 'subject_alt_names' => ['DNS:puppet', 'DNS:puppet.example.com'],
+ 'fingerprint' => 'F8:DA:15:EA:BD:2F:2D:D3:05:71:73:55:96:74:A4:97:2B:04:06:47:A8:8E:D2:C4:AB:8F:EC:3B:7C:0F:0A:EE',
+ 'fingerprints' => {
+ 'SHA1' => '4F:C2:4B:C5:B3:AD:36:64:8D:70:65:85:0B:F9:29:9E:96:67:4B:6F',
+ 'SHA256' => 'F8:DA:15:EA:BD:2F:2D:D3:05:71:73:55:96:74:A4:97:2B:04:06:47:A8:8E:D2:C4:AB:8F:EC:3B:7C:0F:0A:EE',
+ 'SHA512' => '83:BD:D2:32:30:F3:3E:69:7D:61:ED:A8:3F:3D:29:81:1C:96:AC:39:9B:A3:09:9E:61:9F:17:78:91:69:73:12:84:51:59:EE:93:42:AB:A8:34:72:41:43:B5:48:32:E7:3C:DE:85:13:5E:78:A5:C9:FD:A3:FF:54:53:7C:E6:03',
+ 'default' => 'F8:DA:15:EA:BD:2F:2D:D3:05:71:73:55:96:74:A4:97:2B:04:06:47:A8:8E:D2:C4:AB:8F:EC:3B:7C:0F:0A:EE'
+ },
+ 'not_after' => '2019-08-25T19:25:29UTC',
+ 'not_before' => '2014-08-25T19:25:29UTC',
+ 'serial_number' => 4
+ }
+ ]
+ end
+ end
def setup
@api = Proxy::PuppetCa::PuppetcaHttpApi::PuppetcaImpl.new
@@ -51,7 +75,25 @@ class PuppetCaHttpImplTest < Test::Unit::TestCase
'not_after' => nil,
'not_before' => nil,
'serial' => nil,
- 'state' => 'signed'
+ 'state' => 'valid'
+ }
+ }
+
+ assert_equal expected, @api.list
+ end
+
+ def test_list_puppetserver_6_3
+ client = FakeCaApiV1Request63.new
+ @api.stubs(:client).returns(client)
+
+ expected = {
+ 'puppet.example.com' => {
+ 'fingerprint' =>
+ 'F8:DA:15:EA:BD:2F:2D:D3:05:71:73:55:96:74:A4:97:2B:04:06:47:A8:8E:D2:C4:AB:8F:EC:3B:7C:0F:0A:EE',
+ 'not_after' => '2019-08-25T19:25:29UTC',
+ 'not_before' => '2014-08-25T19:25:29UTC',
+ 'serial' => 4,
+ 'state' => 'revoked'
}
} |
e73bcdc
to
5c01050
Compare
Looking good. |
Should it be possible to delete/clean a revoked certificate? I'm not getting an option to. |
I've successfully provisioned a new host with both host whitelisting and token based provisioning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failures are unrelated. I'm going to spend time today to finish the puppet-foreman_proxy change today and then we should merge it.
I think theforeman/puppet-foreman_proxy#488 is now ready |
Thanks! |
Thanks everyone! I'm assuming this also needs updates in the manual? |
This is required for Puppet 6 support as the legacy cli commands aren't available anymore.