Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:102920
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain kelunik.com from 85.215.255.25 cause and error)
MIME-Version: 1.0
References: <8bb21db9-1f64-6520-68af-23840bba77ea@snowgarden.ch> <CAEKnhAHWhZ+EghPv5BGhSLpk5FDme3aHB9-w5=AO+bY1WUJhtg@mail.gmail.com>
In-Reply-To: <CAEKnhAHWhZ+EghPv5BGhSLpk5FDme3aHB9-w5=AO+bY1WUJhtg@mail.gmail.com>
Date: Thu, 19 Jul 2018 22:36:35 +0200
Message-ID: <CANUQDCh7NWMNniByMj=poc1KtChjH9KABZ8qw7KxRxjZx4NwDA@mail.gmail.com>
To: Jakub Zelenka <bukka@php.net>
Cc: dol+php@snowgarden.ch, PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] Improvements to openssl_crs_new. Need advice
From: me@kelunik.com (Niklas Keller)

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
>
> >