search

voxpupuli / puppet-openssl

Puppet OpenSSL module
Apache License 2.0
38 stars 84 forks source link

Use private_key parameter when creating certificate #186

Closed vasilevalex closed 3 months ago

vasilevalex commented 5 months ago

Pull Request (PR) description

Use private key created before, not the default value.

This Pull Request (PR) fixes the following issues

Fixes #185

zilchms commented 5 months ago

I am still not sure if we want to allow direct private key passing into the certificate. CSRs exist for exactly this reason. The regression for no longer allowing directly passing keys into certs and using CSRs instead was made specifically for this reason.

To be clear: I am not against allowing passing the private key directly, I just would like to get a good reason to do so

I may also be completely wrong here, been a while since I made those changes

vasilevalex commented 5 months ago

In the simplest case without CA we need some private key to sign the certificate. So openssl is invokeg with -signkey: https://github.com/voxpupuli/puppet-openssl/blob/dcd89b5b529a2ab1f74c8b4869d8ba2a155131ca/lib/puppet/provider/x509_cert/openssl.rb#L84 with the private key as parameter. And if we don't pass the private key generated earlier, default value would be used.

fmichea commented 4 months ago

From @zilchms : I am still not sure if we want to allow direct private key passing into the certificate. CSRs exist for exactly this reason. The regression for no longer allowing directly passing keys into certs and using CSRs instead was made specifically for this reason.

Just hit this issue as well, I'm a bit of two minds on this. On one hand this bug currently prevents using the key/csr/cert parameters in order to control their naming convention. Plus if these parameters are not used, then the key is indeed passed to the certificate for signing anyway, with the correct name this time so no failure happens. So in the current state of things it doesn't appear to me that this fix should necessarily be blocked short term.

On the other hand I take your point about CSR, and for those that use a CA to then sign the certificates, maybe the right solution would be to allow creating the CNF + KEY + CSR without the certificate, and let that be generated by the CA?

At least in my setup I am working on, I get the certificate because its generated by default, but I dont use it and instead create signed versions from the CSR and my CA to then be uploaded. I could totally use an option to turn of cert generation off since those files are not necessary to me.

Any thoughts?

ashish1099 commented 3 months ago

any update on how we go ahead, I just hit the same issue

yakatz commented 3 months ago

It looks like the PR needs a rebase. Running through the comments very quickly, I don't see any objections, so once it can be cleanly merged, we should be able to do so.

yakatz commented 3 months ago

Actually, @zilchms might be opposed. Can you clarify?

zilchms commented 3 months ago

No further objections, feel free to merge after rebasing and passing CI :) At some point I will write some tests and refactor a bit of this module, so bugs/regressions like this dont sneak in unnoticed/without discussion beforehand

vasilevalex commented 3 months ago

Rebased and passed CI

ekohl commented 3 months ago

@vasilevalex sorry, but we've been working on this module today and that introduced another merge conflict. Good news is that we should have acceptance tests now so it should be easier to add an acceptance test to it.

ekohl commented 3 months ago

I took the liberty to rebase your PR.