Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:99424
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain vdwaa.nl from 74.125.82.52 cause and error)
Date: Wed, 7 Jun 2017 14:54:51 +0200
To: Jakub Zelenka <bukka@php.net>
Cc: php-internals <internals@lists.php.net>
Message-ID: <20170607125449.GA9868@gmail.com>
References: <20170531101952.GB26690@gmail.com>
 <CAEKnhAF-NnxvwmgVzZuKitKZJu1cUTK+9dKzuido22s373m73w@mail.gmail.com>
 <20170604182219.GA6723@gmail.com>
 <CAEKnhAG=i0DKfa8E9cfFqE2DoO8p08WcYkm00+Z=ib5fEfGsig@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEKnhAG=i0DKfa8E9cfFqE2DoO8p08WcYkm00+Z=ib5fEfGsig@mail.gmail.com>
User-Agent: Mutt/1.8.3 (2017-05-23)
Subject: Re: [PHP-DEV] [RFC] Add openssl_pkcs7_read and extend
 openssl_pkcs7_verify
From: jelle@vdwaa.nl (Jelle van der Waa)

On 06/04/17 at 08:02pm, Jakub Zelenka wrote:
> On Sun, Jun 4, 2017 at 7:22 PM, Jelle van der Waa <jelle@vdwaa.nl> wrote:
> 
> > On 06/04/17 at 07:00pm, Jakub Zelenka wrote:
> > > On Wed, May 31, 2017 at 11:19 AM, Jelle van der Waa <jelle@vdwaa.nl>
> > wrote:
> > >
> > > > I would like to propose the addition of openssl_pkcs7_read and
> > extending
> > > > openssl_pkcs7_verify to also return a PKCS7 structure. The reasoning
> > for
> > > > the addition of these functions is the requirement at work to obtain
> > the
> > > > CA certificates usually send along with a signed email. The CA
> > > > certificates are required for OCSP verification (which is currently
> > done
> > > > in pure PHP, I also would like to see this added in PHP in the future).
> > > >
> > > > It is currently impossible to acquire the CA certificates with the
> > > > openssl functions which PHP provides, I've also found a bug report
> > > > requesting the ability to read a PKCS7 blob. [1]
> > > >
> > > > To summarize, I would propose to add an optional parameter to
> > > > openssl_pkcs7_verify which takes a string that defines the location
> > > > where the PKCS7 blob should be stored.
> > > >
> > > > $pkcs7 = "chain.pk7";
> > > > openssl_pkcs7_verify($file, PKCS7_NOVERIFY, $outfile, [], $outfile,
> > > > $content, $pkcs7);
> > > >
> > > > To be able to read the blob, I would propose a new function
> > > > openssl_pkcs7_read which returns an array of strings containing the PEM
> > > > certificates in the PKCS7 blob. I've based the naming and behaviour on
> > > > openssl_pkcs12_read.
> > > >
> > > > openssl_pkcs7_read($pkcs7, $data);
> > > > var_dump($data);
> > > >
> > > > I've implemented the above mentioned changes in my fork of PHP, mind
> > > > that the code isn't ready for a PR yet since there are some styling
> > > > issues, possible memory leaks and of course missing tests. The code
> > > > however works as a proof of concept. [2]
> > > >
> > > > For further background information, obtaining the pk7 output can be
> > done
> > > > with the 'openssl' tool:
> > > >
> > > > openssl smime -verify -pk7out  -in signed_email.eml  > foo.pkcs7
> > > > openssl pkcs7 -print_certs -in foo.pkcs7
> > > >
> > > >
> > > It seems reasonable from the quick look.
> > >
> > > I don't think we need RFC unless there are some objections. Once it's
> > > ready, PR should be enough IMHO.
> >
> > Thanks, this is my first contributing a feature to PHP and from reading
> > the wiki an RFC was the method to introduce new features.
> >
> > Offtopic, I also want to later add string based instead of file based
> > openssl_pkcs7_* functions. Do I need to create an RFC for it?
> >
> >
> First of all, it's really great that you contribute to this so thanks!

No problem, we've so far build our application with S/MIME support on
PHP and it worked so far :-)

> I think that for changes like this, it's best to first open PR. When the PR
> is ready and all issues resolved, then it should be announced on this
> mailing list and then see if there are no objections. If there are not, it
> should be fine to merge it IMHO. Otherwise if there is no agreement, it
> probably needs RFC. :) I think that RFC makes sense either when the feature
> is complex or when people are not in agreement about it.

Ok, I'm open to feedback! I've opened the PR https://github.com/php/php-src/pull/2563

Another thing I would want to work on is making the openssl_pkcs7_*
functions in memory instead of file based. This would increase the
security concerns since that means it won't save plaintext temporarily to
file. 

An attempt for that has been made in a PR, but was closed. Is the PR on
the right 'track' or can the current functions change to accept either a
string or a filename? (I'm not 100% if PHP internals allow that) or if
it's really wanted. [1]

[1] https://github.com/php/php-src/pull/560

-- 
Jelle van der Waa