Make x509 altnames and extkeyusage optional

[treydock]

The defaults of empty arrays are generating invalid openssl configs

Example of what's generated:

[ v3_req ]
extendedKeyUsage  =
subjectAltName    = @alt_names

This throws all sorts of errors:

[root@rocky-8 /]# /opt/puppetlabs/puppet/bin/openssl req -new -key /etc/xdmod/simplesamlphp/cert/xdmod.key -config /etc/xdmod/simplesamlphp/cert/xdmod.cnf -out /etc/xdmod/simplesamlphp/cert/xdmod.csr
Error Loading request extension section v3_req
46912522284800:error:2206D06C:X509 V3 routines:X509V3_parse_list:invalid null name:crypto/x509v3/v3_utl.c:386:
46912522284800:error:22097069:X509 V3 routines:do_ext_nconf:invalid extension string:crypto/x509v3/v3_conf.c:93:name=extendedKeyUsage,section=
46912522284800:error:22098080:X509 V3 routines:X509V3_EXT_nconf:error in extension:crypto/x509v3/v3_conf.c:47:name=extendedKeyUsage, value=

[rtib]

Sorry for the fly by comment, but you may have a look on https://github.com/voxpupuli/puppet-openssl/commit/50317c20da3eabce73436996cd233ac853061cec which is fixing the config template too, I hope.

[treydock]

@rtib What about the updated change to use Optional[Array[String[1]]] ? That way it's either Undef or a non-empty Array so no need to check if empty as Puppet will validate the parameters first.

[rtib]

The semantics of the empty() function is covering all you need. It is built-in with Puppet, so there is no additional dependency.

Though notions like Optional[Array[String[1]]] seem pretty popular these days, IMO, they are kinda crabbed. This particular one still allow undef which you need to check for later and it is deprecated to use empty() for that. It seems more simple and easier to understand to have an Array or Array[String] with an empty array as default.

[rtib]

The root cause of the problem is lying in the template used to generate the config, not in type signature of the parameters. This is part of what rendered release 3.0.0 unusable (see #178). I'd prefer to not change the module API.

[zilchms]

I agree partly with both: we have to fix the template to only set the values (and corresponding keys) in the config, if they are present. Checking for undef is easy enough in a template, default boolean behaviour will take care of that (even without empty). I also agree with using Optional[Array[String[1]]]. That way only non empty Arrays with non-empty Strings can be passed in. The undef for the default value we can deal with easily in the config template.

[rtib]

... using Optional[Array[String[1]]]. That way only non empty Arrays with non-empty Strings can be passed in. The undef for the default value we can deal with easily in the config template.

Look at it: Optional[Array[String[1]]] $altnames = undef, or Array $altnames = [],

Now imagine you are new to this module, probably new to Puppet, you want to use the module trying to understand its parameters. Which one you'd prefer?

[rtib]

From the software engineering point of view, the real difference between

if $somearray {

and

unless $somearray.empty {

is that the first one is using a hidden, implicit behaviour of the language, while the semantics of the later is fully explicit.

[treydock]

I look at data types as a way to represent what's valid. If an empty array is not valid, it should not be allowed as a parameter irregardless of how it might look to someone new to Puppet. Data types should validate the inputs, if empty array is not a valid input, it shouldn't be allowed. Similar for empty strings. If empty strings are not a valid input, data types need to prevent them.

I'd imagine a new Puppet user would rather be prevented from invalid inputs or unexpected behavior rather than having an easier time looking at the code. If they go to look at the code cause they passed empty array and it broke X509 cert creation, it will be a lot harder to debug (as I recently found out) than if they are prevented from that issue to begin with.

[treydock]

But the real issue is the template, so fixing that via https://github.com/voxpupuli/puppet-openssl/commit/50317c20da3eabce73436996cd233ac853061cec might be better or be good to do at same time.

[rtib]

I look at data types as a way to represent what's valid.

Which is completely fine. However, there are two perspectives: one is the public API of the module, intended to be understood and used by others; and two, all the internal variables defined as they are used.

On the public API, a parameter called altnames, intended to hold a list of subjectAltName entries might be of type Array. There is nothing wrong having this array empty, even by default. There is no reason to tie the empty case to a special value, i.e. undef.

[treydock]

Correct, empty array is a valid default in these cases. @rtib Do you want to open PR with template fixes or do you want me to handle with this PR?

[rtib]

@rtib Do you want to open PR with template fixes or do you want me to handle with this PR?

I think this PR is the right place for that. But, we need to test that thoroughly, that commit is just a first take.

[rtib]

I found some time to look deeper into this and did some refactoring:

• e1367b92dce56efe6a1508c6b2d3247e31a47fa7 did some code cleanup of x509.pp
• 798f6746eaf43e98ef39ee1f73e165bf9f9b93fd move the handling of distinguished name attributes a bit towards specification as described in section-4.1.2.4 of RFC5280

[zilchms]

Those look pretty good, would you like to provide them in a PR? :)

[rtib]

Opened #179

[zilchms]

I think with the merge of #179 and #180, this is no longer needed? @treydock,. can you check that?