Improvements to openssl_crs_new. Need advice

Hey,

I'd prefer a proper OO API in case we add new features in that area.

Regards, Niklas
Am Do., 19. Juli 2018 um 01:05 Uhr schrieb Jakub Zelenka bukka@php.net:

Hi,

On Wed, 18 Jul 2018, 00:15 Dominic Luechinger, dol+php@snowgarden.ch
wrote:

I'd like to improve the openssl_csr_new function to add any X509
"Requested Extensions" [1] to a CSR.
My motivation to improve this functionality is to avoid workarounds like
altering a openssl.cnf file and pass some ENV variable to it [2].

I already implemented the following new functionality:

Old:
mixed openssl_csr_new ( array $dn , resource &$privkey [, array
$configargs [, array $extraattribs ]] )

New (I can provide a patch, needs cleanup and testing):
mixed openssl_csr_new ( array $dn , resource &$privkey [, array
$configargs [, array $extraattribs[, array $extraexts ]]] )

E.g:

$privkey = `openssl_pkey_new()`;
$csr = openssl_csr_new([], $privkey, null, [], [
    'subjectAltName' => 'DNS:example.tld',
]);

While implementing the new functionality I realized that the 'Requested
Extensions' are represented as a CSR attribute and it contains the ASN1
structure of multiple extensions and their definitions. With the
following example the declaration of the extension should be possible
without the new argument $extraexts in openssl_csr_new.

$privkey = `openssl_pkey_new()`;
// Use OID of ExtensionRequest
$csr = openssl_csr_new([], $privkey, null, ['1.2.840.113549.1.9.14' =>
0xDEADBEEF]);

This won't work because the argument $extraattribs only applies to
subject attributes. The argument name is kind of misleading. See the
following bug report [3] from 2008 that describes the issue in a good
manor. IMHO this bug report is valid and the bug should be fixed in a
way that the attributes are added to the certificationRequestInfo [4]
instead being merged into the subject. This might break some existing
usage of this argument. With this bug fixed 'Requested Extensions' can
be added in a programmatic way. To generate the DER encoded content of
'Requested Extensions' a ASN1 library should be used.

Now comes to tricky part about supporting my initial goal to add
additional'Requested Extensions' to an CSR.

Should I summit my patch with the extra argument as a PR or should I fix
the bug 45076 or should I do both?

extraexts VS bug fix:

• No BC break VS BC break
• No need for a ASN1 library VS working with ASN1 DER encoded data
• Default extensions from openssl.cnf are preserved and can be
  overwritten VS definition of 'Requested Extensions' in DER overwrites
  default extensions from openssl.cnf

Looking at the pros and cons my guts tells my to do both. Patch and bug
fix.
Any other suggestions/thoughts?

I haven't had time to properly look into it but from the quick read, I
would prefer not to break BC even though the behaviour seems incorrect. I
think that changing that can cause some issues potentially. But I might
need more time to think about. Anyway the proposed patch seems reasonable
so it would be great if you can create a PR that.

Cheers

Jakub