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]

This is not solving the problem. It still allows to set an empty array to those parameters which will lead to the same result.

[rtib]

What's about $req_ext = !empty($altnames) or !empty($extkeyusage) instead of that if-else-spaghetti

[zilchms]

well, you have to detect if we actually have values in extkexusage and altnames in order to set the req_ext. You cant set it always as that would error out when generating a cert, when both arrays are empty (and not set in the config, assuming its fixed). I am working on memory here though, when i last worked on it. I could be wrong.

If we only allow non-empty arrays and undef the 'if $altnames or $extkeyusage' works just fine, no need for the empty check

[rtib]

First: My suggesting is essentially to use

$boolean_variable = (expression)

over something like

if (expression) {
  $boolean_variable = true
} else {
  $boolean_variable = false
}

Second, the v3_req section of the config depends on a lot more parameters. The $req_ext is not passed to the template at all, it is referenced only at a single point, to pass it to x509_cert. Thus, you can drop this variable and replace the expression into the x509_cert to be passed to its req_ext parameter.

[rtib]

Sorry for the fly by comment, but you may have a look on 50317c2 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 50317c2 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:

• e1367b9 did some code cleanup of x509.pp
• 798f674 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?