Adds support for PKCS12 containers to SecurityContext

Adds support for PKCS12 containers to SecurityContext

Looking forward to using platform specific APIs, this container type is
more common on MacOS and Windows, and is the only container type
supported by the native iOS API.

R=whesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/89f414dc3776c0ed3dd8ba0b398d3bf214daef70

[zra, 2016-02-09 19:23:18]

zra@google.com changed reviewers:
+ whesse@google.com

[zra, 2016-02-09 19:23:40]

Description was changed from

==========
Adds support for PKCS12 containers to SecurityContext

Looking forward to using platform specific APIs, this container type is
more common on MacOS and Windows, and is the only container type
supported on iOS.
==========

to

==========
Adds support for PKCS12 containers to SecurityContext

Looking forward to using platform specific APIs, this container type is
more common on MacOS and Windows, and is the only container type
supported by the native iOS API.
==========

[Bill Hesse, 2016-02-10 18:08:39]

LGTM with comments addressed.

https://codereview.chromium.org/1687533002/diff/20001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1687533002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:658: status =
SetTrustedCertificatesBytesPKCS12(context, bio);
If someone submits a flawed PEM file, they will never get a good error about
what is actually wrong.  The only way I can see around this is to have a
heuristic that looks for the "=== BEGIN" string, or some other indication of
PEM, and returns the PEM error in that case.

But if this isn't easy, it isn't necessary.

https://codereview.chromium.org/1687533002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:754: // Key/certificate mismatch doesn't imply
status is 0.
Is this implying that we need to add the key to the context before we add the
server certificate chain?  I didn't know this was required before.  If this is
true, is it documented?  Or is this only with a non-null key that it fires?

https://codereview.chromium.org/1687533002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:887: if (x509.get() == NULL) {
Isn't this the result of an error reading the file, like if it isn't a PEM file.
 This gives a non-null return from the function, which means we don't try the
PKCS12 case.

I think you need to check the error here, to see if it is the
ERR_GET_REASON(err) == PEM_R_NO_START_LINE indicating EOF, like we do above.

Maybe we should try the PKCS12 read first.

[zra, 2016-02-10 22:17:14]

Thanks!

https://codereview.chromium.org/1687533002/diff/20001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1687533002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:658: status =
SetTrustedCertificatesBytesPKCS12(context, bio);
On 2016/02/10 18:08:39, Bill Hesse wrote:
> If someone submits a flawed PEM file, they will never get a good error about
> what is actually wrong.  The only way I can see around this is to have a
> heuristic that looks for the "=== BEGIN" string, or some other indication of
> PEM, and returns the PEM error in that case.
> 
> But if this isn't easy, it isn't necessary.

Rearranged the logic a bit to avoid this problem.

https://codereview.chromium.org/1687533002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:754: // Key/certificate mismatch doesn't imply
status is 0.
On 2016/02/10 18:08:39, Bill Hesse wrote:
> Is this implying that we need to add the key to the context before we add the
> server certificate chain?  I didn't know this was required before.  If this is
> true, is it documented?  Or is this only with a non-null key that it fires?

This only fires with a non-null key. This is extra error checking in the case
that the key has been added first. It is still possible to add the key second.

https://codereview.chromium.org/1687533002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:887: if (x509.get() == NULL) {
On 2016/02/10 18:08:39, Bill Hesse wrote:
> Isn't this the result of an error reading the file, like if it isn't a PEM
file.
>  This gives a non-null return from the function, which means we don't try the
> PKCS12 case.
> 
> I think you need to check the error here, to see if it is the
> ERR_GET_REASON(err) == PEM_R_NO_START_LINE indicating EOF, like we do above.
> 
> Maybe we should try the PKCS12 read first.

Added error checking as done in the other calls.

[zra, 2016-02-10 22:21:02]

Description was changed from

==========
Adds support for PKCS12 containers to SecurityContext

Looking forward to using platform specific APIs, this container type is
more common on MacOS and Windows, and is the only container type
supported by the native iOS API.
==========

to

==========
Adds support for PKCS12 containers to SecurityContext

Looking forward to using platform specific APIs, this container type is
more common on MacOS and Windows, and is the only container type
supported by the native iOS API.

R=whesse@google.com

Committed:
https://github.com/dart-lang/sdk/commit/89f414dc3776c0ed3dd8ba0b398d3bf214daef70
==========

[zra, 2016-02-10 22:21:03]

Committed patchset #3 (id:40001) manually as
89f414dc3776c0ed3dd8ba0b398d3bf214daef70 (presubmit successful).