Skip to content
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 #6566 - If the certname does not match the hostname, nil the certname #1604

Closed
wants to merge 1 commit into from

Conversation

sodabrew
Copy link
Member

@domcleal domcleal changed the title Fixes 6566 - If the certname does not match the hostname, nil the certname Fixes #6566 - If the certname does not match the hostname, nil the certname Jul 22, 2014
@domcleal
Copy link
Contributor

Could you amend the commit message to "#6566" so Redmine picks up the ticket number? Thanks.

@sodabrew
Copy link
Member Author

[test]

@GregSutcliffe
Copy link
Member

How does the new cert get to the host? I can see a problem with duplicate hosts showing up in foreman if we're not careful. Consider this:

  1. Host is renamed, certname changes along with hostname
  2. Host runs puppet, sending old hostname and certname (via the certname param in puppet.conf)
  3. Foreman cannot match the cert to an existing host, so creates a new one.

Are we saying it's the users problem to create a new cert matching the new certname? With both values altered at once, Foreman has no way to identify the new host, whereas until now the certname remains the common identifier.

@GregSutcliffe
Copy link
Member

Oh I'm being a fool, aren't I? This is only during orchestration?

@sodabrew
Copy link
Member Author

Yes, this is only at orchestration time. When you put the host into Build mode, Foreman sends a command to the Puppet smart-proxy to invalidate the certificate for certname. The question is whether the certname should change for the next rebuild - a valid UUID certname is retained, but a hostname-based certname may need to change to match a new hostname.

if !Setting[:use_uuid_for_certificates] && Foreman.is_uuid?(certname)
# If use_uuid_for_certificates is true, reuse the certname UUID value.
# If false, then reset the certname if it does not match the hostname.
if (Setting[:use_uuid_for_certificates] ? !Foreman.is_uuid?(certname) : certname != hostname)
logger.info "Removing UUID certificate value #{certname} for host #{name}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, we should probably change this log message to say something like: "Removing old certificate value #{certname} for host #{name} as it's out of date"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed 'UUID' from the comment and rebased.

@domcleal
Copy link
Contributor

Looks good to me - are you happy with the change @GregSutcliffe?

If use_uuid_for_certificates is true, reuse the certname UUID value.
If false, then reset the certname if it does not match the hostname.
@GregSutcliffe
Copy link
Member

I guess my only query, and it's not a bad one, is this: with this in place, is there any value to uuid_certnames? It seems like this is a nicer fix to the problem that uuid certnames was originally implemented for (where hosts want to change hostname because the CR created them with something random).

@domcleal
Copy link
Contributor

I think so, because this change is only when doing orchestration or reprovisioning. uuid_certnames are still useful for hostname changes while a host is running, e.g. a roaming laptop or multi-homed host with an identity crisis.

@GregSutcliffe
Copy link
Member

That's a very good point. In any case, this is good to go 👍

@domcleal
Copy link
Contributor

Merged as d2823e3 for Foreman 1.6.0. Thanks @sodabrew!

@domcleal domcleal closed this Jul 31, 2014
@sodabrew sodabrew deleted the fixes_6566_3222 branch July 31, 2014 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants