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

Add Puppet 5 support to the certificates class #23

Merged
merged 2 commits into from
Oct 27, 2017
Merged

Add Puppet 5 support to the certificates class #23

merged 2 commits into from
Oct 27, 2017

Conversation

binford2k
Copy link
Contributor

This will do its best to manage the certificate access on Puppet 5.
Unfortunately, it's not reasonably possible to do if the machine is
using legacy auth.conf, and there's not a good way to tell whether that
is the case on OSS. Therefore, we fail out and require the user to
either indicate that they're using HOCON, or to not manage the
whitelist. We fail by default because requiring a minor change in
classification is better than a false sense of security.

Tangent: since they're public certificates, I'm not even convinced that
it's worth trying to protect them. Perhaps it's time to just drop the
whitelist idea completely?

Fixes #18

This will do its best to manage the certificate access on Puppet 5.
Unfortunately, it's not reasonably possible to do if the machine is
using legacy auth.conf, and there's not a good way to tell whether that
is the case on OSS. Therefore, we fail out and require the user to
either indicate that they're using HOCON, or to not manage the
whitelist. We fail by default because requiring a minor change in
classification is better than a false sense of security.

Tangent: since they're public certificates, I'm not even convinced that
it's worth trying to protect them. Perhaps it's time to just drop the
whitelist idea completely?

Fixes #18
@binford2k
Copy link
Contributor Author

@olifre and @sammcj can you review and/or provide feedback on this solution? I'm not super happy with it because of the forced classification change, but I cannot figure out how to make it a drop-in upgrade unless we drop whitelist management completely (which I'm ok with).

Sorry for the delay, I actually found and fixed a PE bug while working through this.

@olifre
Copy link
Contributor

olifre commented Oct 26, 2017

[...] but I cannot figure out how to make it a drop-in upgrade unless we drop whitelist management completely (which I'm ok with).

I also don't see another way, and I see you spent quite some effort to explain the changes in the README, very much appreciated!
To me, dropping the whitelist management would also be fine. In any case, with Open Source Puppet 5.x when using the legacy auth.conf, my observation was that the whitelist was completely ignored and things only worked when I explicitly used *.
So what you have implemented in this PR is perfectly fine with me, and if nobody raises concerns, dropping the whitelist would strongly simplify things (and make it a drop-in upgrade). I'd be fine with either of these solutions ;-).

@sammcj
Copy link

sammcj commented Oct 26, 2017

Morning Gents,

Discussing with @ross-w (we're in the same team), we both feel that the effort / trouble of managing the public certificates far outweighs the value of protecting public certs (as you've said), so we tend to agree with you that dropping the whitelist idea completely.


FYI - We only run Puppet 5 with Puppet Enterprise, not open source (Shout out to Puppet for supporting non-profits / charitable organisations) so we can't test this in Puppet Open Source for you.

@binford2k binford2k merged commit a9d1508 into master Oct 27, 2017
@binford2k binford2k deleted the puppet5 branch December 20, 2018 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants