-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fix config template issues and add some improvements #179
Conversation
The defaults of empty arrays are generating invalid openssl configs
This PR, also embracing #177, is fixing only the first part of #178, but leaves the second part open and still producing the issue
I'd suggest to address this in a separate PR. |
days => $days, | ||
password => $password, | ||
req_ext => $req_ext, | ||
req_ext => !empty($altnames) and !empty($extkeyusage), |
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.
@zilchms @rtib This condition seems to prevent altnames
from being used. The condition used to be !empty($altnames + $extkeyusage)
so either would trigger req_ext
to be set and therefore the openssl command to be given the necessary option for the certificate to have the extension (this is done in lib/puppet/provider/x509_cert/openssl.rb
line 103
). This became a and
condition which now requires both lists to be non-empty. I wasnt able to get certs with SANs to generate until I changed this locally to a or
condition. I'll look into making a contribution shortly but I am commenting here just to have you take a look if you agree and maybe do the change if it's easy on your end.
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.
You're right, my mistake. AFAIK, I tested extkeyusage, which should work, but that can't work either. Unfortunately, the module lacks appropriate tests, which is a shame considering the importance of its functionality. I can file a PR, I broke it I fix it.
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.
Thanks for the quick answer! This is the basic change with a test (that fails with main branch but passes with change), but I'll let you decide if there is more is needed. I didnt see a test for the actual function generating the openssl command for instance but only did a quick glance.
diff --git a/manifests/certificate/x509.pp b/manifests/certificate/x509.pp
index 7543d7e..2aae7c4 100644
--- a/manifests/certificate/x509.pp
+++ b/manifests/certificate/x509.pp
@@ -184,7 +184,7 @@ define openssl::certificate::x509 (
csr => $csr,
days => $days,
password => $password,
- req_ext => !empty($altnames) and !empty($extkeyusage),
+ req_ext => !empty($altnames) or !empty($extkeyusage),
force => $force,
ca => $ca,
cakey => $cakey,
diff --git a/spec/defines/openssl_certificate_x509_spec.rb b/spec/defines/openssl_certificate_x509_spec.rb
index e2df33d..57facfe 100644
--- a/spec/defines/openssl_certificate_x509_spec.rb
+++ b/spec/defines/openssl_certificate_x509_spec.rb
@@ -422,6 +422,26 @@ describe 'openssl::certificate::x509' do
}
end
+ context 'when passing altnames, extension is enabled' do
+ let(:params) do
+ {
+ country: 'com',
+ organization: 'bar',
+ commonname: 'foo.example.com',
+ altnames: ['bar.example.com'],
+ }
+ end
+
+ it {
+ is_expected.to contain_x509_cert('/etc/ssl/certs/foo.crt').with(
+ ensure: 'present',
+ template: '/etc/ssl/certs/foo.cnf',
+ csr: '/etc/ssl/certs/foo.csr',
+ req_ext: true
+ )
+ }
+ end
+
context 'when passing all parameters' do
let(:params) do
{
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.
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.
Pull Request (PR) description
Some bugfixes and improving refactoring.
This Pull Request (PR) fixes the following issues
Fixes config template issue which partly Fixes #178
Improves transparency of default values of openssl::certificate::x509
Makes DN attributes optional moving the API towards RFC5280