Don't use a hidden value of certname if use_uuid_for_certificates has been toggled on/off #925

sodabrew · 2013-10-02T23:58:40Z

Expected:

Enable use_uuid_for_certificates
Create a host
Host has a UUID now
Set host to Build, pull the host's template, watch the certname UUID value be added to the Puppet Autosign file.
... realize that uuid certificates aren't right for your organization ...
Disable use_uuid_for_certificates
Set host to Build, pull the host's template, watch the certname hostname value be added to the autosign file.

Actual:

Set host to Build, pull the host's template, watch the certname UUID value be added to the Puppet Autosign file.
Watch as your Puppet run fails to get a certificate because the client has submitted a certificate request using its hostname now.

Patch fixes this by checking for state of the use_uuid_for_certificates setting in the certname model method.

domcleal · 2013-10-04T16:24:06Z

So the scenario here is that you enable the setting, create the host, disable the setting, you provision the host and it stays in effect. I'm worried about another case where you enable the setting, provision the host, disable it and then Foreman doesn't know the correct certname for the host.

In that case you'd want Foreman to still know the old UUID certname, but not issue UUID certnames to new hosts. Does that make sense? I'm not sure which is correct if so.

@GregSutcliffe, what are your thoughts?

sodabrew · 2013-10-04T16:33:14Z

The UI hides the certname when you disable that option, so you cannot see or edit it. The behavior should be made consistent - the other way around is never to hide the certname if it has been set, that way I can see that I have to delete it.

domcleal · 2013-10-07T07:58:07Z

Yes, having the ability to view and edit the certname would be useful generally and probably help here. I think that'd be a viable solution.

Another way, which I think would keep both of our scenarios working, would be to reset the certname according to the current value of the UUID setting while performing the certificate revocation and setting up autosigning.

e.g. in app/models/host/managed.rb, handle_ca:

respond_to?(:initialize_puppetca,true) && initialize_puppetca && delCertificate && setAutosign

Between delCertificate (revocation) and setAutosign (adding autosign entry), we reset certname if it contains a UUID and the UUID setting's now off:

certname = name if certname =~ /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\Z/ && !Setting[:use_uuid_for_certificates]

I think this would solve the problem you're hitting, while keeping certname working for existing hosts?

GregSutcliffe · 2013-10-07T11:41:41Z

@domcleal that makes a lot of sense, apart from being able to edit the certname. Since that's closely tied to certificate generation/revocation, I'm not sure making it editable is a good idea. I definitely agree with displaying it if it's not == to the hostname (or maybe just all the time), and the reset code looks good too.

sodabrew · 2013-10-07T16:08:06Z

Seems complicated?

I like that my proposal has simple logic based on the one setting, because I don't want to keep the UUID cert name after toggling this option off anyhow.

GregSutcliffe · 2013-10-07T16:25:11Z

@sodabrew I agree, the issue is that your patch as written will mean that when you rebuild a host, the old cert won't get cleaned out from your puppetmaster.

This is because the clean operation happens as the host is being rebuilt (specifically when it retrieves it's provision template), which will be after you've altered the Setting and thus it tries to clean host.name rather than uuid from the puppetmaster.

What @domcleal is proposing is to fix that problem by resetting the certname after the clean has happened, rather than before - i.e by hooking in at the right time in the orchestration. Otherwise the logic is pretty much the same, the only extra bit is looking explicitly for old UUID certnames, rather than any certname. It's still based on the one setting, as you rightly want.

sodabrew · 2013-10-07T17:00:46Z

Oh, gotcha. Sounds good - will you be able to code this up for 1.3, or shall I jump in on it?

domcleal · 2013-10-07T17:50:06Z

If you're able to, that'd be great - though I'd ask for tests too please. test/unit/host_test.rb in the handle_ca area would be a good place to start I think, since there's a test that stubs out most of those calls already. Thanks!

sodabrew · 2013-10-08T06:48:31Z

If this works and looks mergeble then I'll rebase/squash first :) ... caveat that I haven't actually run the unit tests yet.

domcleal · 2013-10-08T08:04:19Z

[test]

domcleal · 2013-10-08T09:47:25Z

[test]

domcleal · 2013-10-08T12:16:49Z

test/unit/host_test.rb

+    h.expects(:delCertificate).returns(true)
+    h.expects(:setAutosign).returns(true)
+    assert h.handle_ca
+    assert h.certname.nil?


This is failing in the tests as h.certname will return the name if not defined. Perhaps use .read_attribute(:certname) or assert_equal to the name?

domcleal · 2013-10-08T18:07:54Z

Last one? [test]

sodabrew · 2013-10-08T18:11:25Z

I hope so, sorry for the dumb errors! I couldn't get tests running locally at all :/

domcleal · 2013-10-08T18:18:20Z

No worries, that's unusual. It's normally OK after copying the settings/database files in place, db:migrate and rake test.

sodabrew · 2013-10-08T18:22:59Z

Did the tests begin?

domcleal · 2013-10-08T18:23:50Z

Yep, on their way now. It checks every 15 mins, so there's some latency involved :)

sodabrew · 2013-10-08T18:32:37Z

Huh.

- expected exactly once, not yet invoked: #<Object:0x7f504d4606f0>.set_autosign('dhcp.mydomain.net')
- expected exactly once, not yet invoked: #<Object:0x7f504d4606f0>.del_certificate('b4340f6c-9ce7-4958-9dcf-de02d36461d5')

domcleal · 2013-10-08T18:43:57Z

Ah, because the methods on the host itself (which calls the PuppetCA object) are stubbed:

- expected exactly once, invoked once: #<Host::Managed:0x7ff7fe2532f0>.setAutosign(any_parameters)
- expected exactly once, invoked once: #<Host::Managed:0x7ff7fe2532f0>.delCertificate(any_parameters)

Removing these lines and making the puppetca stubs return true makes the test pass locally: domcleal@3a7f4dd

…based certificate and before enabling autosign for a hostname-based certificate

domcleal · 2013-10-08T19:15:09Z

[test]

ohadlevy · 2013-10-08T19:51:59Z

app/models/host/managed.rb

+    return false unless delCertificate
+
+    # If the user has changed use_uuid_for_certificates to false,
+    # then null out the certname. This means we may revoke the hostname


shouldnt we do this when the setting change instead?
this might be a bit unexpected?

That then raises the issue in my first two comments, we lose track of certificates of existing hosts. This is less surprising, as existing certs continue to work and Foreman can delete them (as we continue to track them), but then sign new certificates as per the current value of the setting.

shouldn't we have some way to warn the user? that this change is potentially impacting some of the hosts?

ideally, we should have a way to:

let the user know which systems might be impacted

let the user decide what to do on a per system base?

Perhaps ideally, but that's a lot of complexity and work for a single setting.

I see a few options forward then:

document well

open another follow up feature request and merge as is

extend this PR to include a warning to the user?

any other option? what would be your take?

I prefer (1) and (2). I think the behaviour this PR currently implements is good - it doesn't break existing hosts and automatically picks up the change when required. I can add a note to the setting in the manual and file a related ticket on merge.

sodabrew · 2013-10-08T20:17:57Z

Added logging?

domcleal · 2013-10-08T20:43:24Z

Nice, thanks @sodabrew. I'll give this a try and merge tomorrow.

sodabrew · 2013-10-08T20:53:14Z

Thank you too, I appreciated the pointers along the way.

domcleal · 2013-10-09T08:11:52Z

No problem. Thanks for the contribution @sodabrew, merged as 89e506f for Foreman 1.3.0 final.

domcleal reviewed Oct 8, 2013
View reviewed changes

sodabrew added 2 commits October 8, 2013 11:55

Add Foreman.is_uuid?

7053acc

Code and tests to nil the certname of a host after revoking its uuid-…

8a0f21c

…based certificate and before enabling autosign for a hostname-based certificate

ohadlevy reviewed Oct 8, 2013
View reviewed changes

Add logging when we remove the certname UUID value from a host

4bc825d

domcleal closed this Oct 9, 2013

sodabrew deleted the patch-1 branch October 9, 2013 19:05

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use a hidden value of certname if use_uuid_for_certificates has been toggled on/off #925

Don't use a hidden value of certname if use_uuid_for_certificates has been toggled on/off #925

sodabrew commented Oct 2, 2013

domcleal commented Oct 4, 2013

sodabrew commented Oct 4, 2013

domcleal commented Oct 7, 2013

GregSutcliffe commented Oct 7, 2013

sodabrew commented Oct 7, 2013

GregSutcliffe commented Oct 7, 2013

sodabrew commented Oct 7, 2013

domcleal commented Oct 7, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

domcleal commented Oct 8, 2013

domcleal Oct 8, 2013

domcleal commented Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

domcleal commented Oct 8, 2013

ohadlevy Oct 8, 2013

domcleal Oct 8, 2013

ohadlevy Oct 8, 2013

domcleal Oct 8, 2013

ohadlevy Oct 8, 2013

domcleal Oct 8, 2013

ohadlevy Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 9, 2013

Don't use a hidden value of certname if use_uuid_for_certificates has been toggled on/off #925

Don't use a hidden value of certname if use_uuid_for_certificates has been toggled on/off #925

Conversation

sodabrew commented Oct 2, 2013

domcleal commented Oct 4, 2013

sodabrew commented Oct 4, 2013

domcleal commented Oct 7, 2013

GregSutcliffe commented Oct 7, 2013

sodabrew commented Oct 7, 2013

GregSutcliffe commented Oct 7, 2013

sodabrew commented Oct 7, 2013

domcleal commented Oct 7, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

domcleal commented Oct 8, 2013

Choose a reason for hiding this comment

domcleal commented Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

domcleal commented Oct 8, 2013

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

sodabrew commented Oct 8, 2013

domcleal commented Oct 8, 2013

sodabrew commented Oct 8, 2013

domcleal commented Oct 9, 2013