Set OPENSSL_EC_NAMED_CURVE explicitly on EC key so that certificate has ASN1 OID and NIST curve inf…

Set OPENSSL_EC_NAMED_CURVE explicitly on EC key so that certificate has ASN1 OID and NIST curve info. Without this openSSL handshake negotiation fails throwing NO_SHARED_CIPHER error. the change made is along the lines of openssl behavior documented here: https://wiki.openssl.org/index.php/Elliptic_Curve_Diffie_Hellman#ECDH_and_Named_Curves

tested with openssl 1.0.2j

BUG=webrtc:6763
Committed: https://crrev.com/bbfed52cf2912d5088abeb017049241e4d37d21f
Cr-Commit-Position: refs/heads/master@{#15536}

[ssaroha, 2016-11-28 07:06:34]

Description was changed from

==========
Set OPENSSL_EC_NAMED_CURVE explicitly on EC key so that certificate has ASN1 OID
and NIST curve info. Without this openSSL handshake negotiation fails throwing
NO_SHARED_CIPHER error. the change made is along the lines of openssl behavior
documented here:
https://wiki.openssl.org/index.php/Elliptic_Curve_Diffie_Hellman#ECDH_and_Nam...

tested with openssl 1.0.2j

BUG=webrtc:6763
==========

to

==========
Set OPENSSL_EC_NAMED_CURVE explicitly on EC key so that certificate has ASN1 OID
and NIST curve info. Without this openSSL handshake negotiation fails throwing
NO_SHARED_CIPHER error. the change made is along the lines of openssl behavior
documented here:
https://wiki.openssl.org/index.php/Elliptic_Curve_Diffie_Hellman#ECDH_and_Nam...

tested with openssl 1.0.2j

BUG=webrtc:6763
==========

[mattdr-at-webrtc.org, 2016-11-29 23:52:10]

mattdr@webrtc.org changed reviewers:
+ mattdr@webrtc.org

[mattdr-at-webrtc.org, 2016-11-29 23:52:11]

https://codereview.webrtc.org/2534773002/diff/1/webrtc/base/opensslidentity.cc
File webrtc/base/opensslidentity.cc (right):

https://codereview.webrtc.org/2534773002/diff/1/webrtc/base/opensslidentity.c...
webrtc/base/opensslidentity.cc:68: EC_KEY_set_asn1_flag(ec_key,
OPENSSL_EC_NAMED_CURVE);
Let's change this comment to offer a bit more context:

Ensure the curve name is included when this key is serialized. Without this
call, OpenSSL versions before 1.1.0 will create certificates that don't work for
TLS. This is a no-op for BoringSSL and OpenSSL 1.1.0+.

[ssaroha, 2016-12-02 16:16:35]

On 2016/11/29 23:52:11, mattdr-at-webrtc.org wrote:
> https://codereview.webrtc.org/2534773002/diff/1/webrtc/base/opensslidentity.cc
> File webrtc/base/opensslidentity.cc (right):
> 
>
https://codereview.webrtc.org/2534773002/diff/1/webrtc/base/opensslidentity.c...
> webrtc/base/opensslidentity.cc:68: EC_KEY_set_asn1_flag(ec_key,
> OPENSSL_EC_NAMED_CURVE);
> Let's change this comment to offer a bit more context:
> 
> Ensure the curve name is included when this key is serialized. Without this
> call, OpenSSL versions before 1.1.0 will create certificates that don't work
for
> TLS. This is a no-op for BoringSSL and OpenSSL 1.1.0+.

submitted patch 2.

[mattdr-at-webrtc.org, 2016-12-02 19:10:29]

Thanks for the patch. Just some cosmetic stuff and it will be ok to submit.

Great investigation, by the way!

https://codereview.webrtc.org/2534773002/diff/20001/webrtc/base/opensslidenti...
File webrtc/base/opensslidentity.cc (right):

https://codereview.webrtc.org/2534773002/diff/20001/webrtc/base/opensslidenti...
webrtc/base/opensslidentity.cc:64: // Ensure curve name is included when EC key
is
The whitespace is a bit weird here -- it seems to associate the comment with the
previous call.

Let's have:

EC_KEY* ec_key = ...
<blank line>
//
//
//
EC_KEY_set_asn1_flag...
<blank line>
if (!pkey ...

that way it's obvious which call the comment refers to.

https://codereview.webrtc.org/2534773002/diff/20001/webrtc/base/opensslstream...
File webrtc/base/opensslstreamadapter.cc (right):

https://codereview.webrtc.org/2534773002/diff/20001/webrtc/base/opensslstream...
webrtc/base/opensslstreamadapter.cc:22: #include <openssl/ssl.h>
Please keep headers in alphabetical order.

[ssaroha, 2016-12-03 01:38:47]

On 2016/12/02 19:10:29, mattdr-at-webrtc.org wrote:
> Thanks for the patch. Just some cosmetic stuff and it will be ok to submit.
> 
> Great investigation, by the way!
> 
>
https://codereview.webrtc.org/2534773002/diff/20001/webrtc/base/opensslidenti...
> File webrtc/base/opensslidentity.cc (right):
> 
>
https://codereview.webrtc.org/2534773002/diff/20001/webrtc/base/opensslidenti...
> webrtc/base/opensslidentity.cc:64: // Ensure curve name is included when EC
key
> is
> The whitespace is a bit weird here -- it seems to associate the comment with
the
> previous call.
> 
> Let's have:
> 
> EC_KEY* ec_key = ...
> <blank line>
> //
> //
> //
> EC_KEY_set_asn1_flag...
> <blank line>
> if (!pkey ...
> 
> that way it's obvious which call the comment refers to.
> 
>
https://codereview.webrtc.org/2534773002/diff/20001/webrtc/base/opensslstream...
> File webrtc/base/opensslstreamadapter.cc (right):
> 
>
https://codereview.webrtc.org/2534773002/diff/20001/webrtc/base/opensslstream...
> webrtc/base/opensslstreamadapter.cc:22: #include <openssl/ssl.h>
> Please keep headers in alphabetical order.

thanks, submitted patch 4 which has all the review comments addressed.

[mattdr-at-webrtc.org, 2016-12-07 21:56:41]

lgtm

[mattdr-at-webrtc.org, 2016-12-07 21:57:58]

The CQ bit was checked by mattdr@webrtc.org

[commit-bot: I haz the power, 2016-12-07 21:58:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-07 22:05:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 22:05:47]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11172)

[mattdr-at-webrtc.org, 2016-12-07 23:08:26]

mattdr@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[mattdr-at-webrtc.org, 2016-12-07 23:08:52]

added pthatcher for OWNERS

[pthatcher1, 2016-12-08 02:24:55]

Looks good to me.  I just want to make sure we have the correct contributor
process done for a new contributor.

https://codereview.webrtc.org/2534773002/diff/60001/AUTHORS
File AUTHORS (right):

https://codereview.webrtc.org/2534773002/diff/60001/AUTHORS#newcode33
AUTHORS:33: Satender Saroha <ssaroha@yahoo.com>
Did you read and sign one of the contributor agreements at
https://webrtc.org/contributing/?

[ssaroha, 2016-12-08 03:03:33]

On 2016/12/08 02:24:55, pthatcher1 wrote:
> Looks good to me.  I just want to make sure we have the correct contributor
> process done for a new contributor.
> 
> https://codereview.webrtc.org/2534773002/diff/60001/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.webrtc.org/2534773002/diff/60001/AUTHORS#newcode33
> AUTHORS:33: Satender Saroha <mailto:ssaroha@yahoo.com>
> Did you read and sign one of the contributor agreements at
> https://webrtc.org/contributing/

Yes, i signed the individual contributor agreement on Nov 27th.

[pthatcher1, 2016-12-10 02:01:01]

lgtm

[mattdr-at-webrtc.org, 2016-12-12 02:07:49]

The CQ bit was checked by mattdr@webrtc.org

[commit-bot: I haz the power, 2016-12-12 02:07:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-12 02:42:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481508469090030,
"parent_rev": "8fe291f518e8538f03eda34eb648088b7f22705b", "commit_rev":
"296080756f1af3482569f8acf7c4a05de3cd8d56"}

[commit-bot: I haz the power, 2016-12-12 02:42:10]

Description was changed from

==========
Set OPENSSL_EC_NAMED_CURVE explicitly on EC key so that certificate has ASN1 OID
and NIST curve info. Without this openSSL handshake negotiation fails throwing
NO_SHARED_CIPHER error. the change made is along the lines of openssl behavior
documented here:
https://wiki.openssl.org/index.php/Elliptic_Curve_Diffie_Hellman#ECDH_and_Nam...

tested with openssl 1.0.2j

BUG=webrtc:6763
==========

to

==========
Set OPENSSL_EC_NAMED_CURVE explicitly on EC key so that certificate has ASN1 OID
and NIST curve info. Without this openSSL handshake negotiation fails throwing
NO_SHARED_CIPHER error. the change made is along the lines of openssl behavior
documented here:
https://wiki.openssl.org/index.php/Elliptic_Curve_Diffie_Hellman#ECDH_and_Nam...

tested with openssl 1.0.2j

BUG=webrtc:6763

Review-Url: https://codereview.webrtc.org/2534773002
==========

[commit-bot: I haz the power, 2016-12-12 02:42:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-12 02:42:18]

Description was changed from

==========
Set OPENSSL_EC_NAMED_CURVE explicitly on EC key so that certificate has ASN1 OID
and NIST curve info. Without this openSSL handshake negotiation fails throwing
NO_SHARED_CIPHER error. the change made is along the lines of openssl behavior
documented here:
https://wiki.openssl.org/index.php/Elliptic_Curve_Diffie_Hellman#ECDH_and_Nam...

tested with openssl 1.0.2j

BUG=webrtc:6763

Review-Url: https://codereview.webrtc.org/2534773002
==========

to

==========
Set OPENSSL_EC_NAMED_CURVE explicitly on EC key so that certificate has ASN1 OID
and NIST curve info. Without this openSSL handshake negotiation fails throwing
NO_SHARED_CIPHER error. the change made is along the lines of openssl behavior
documented here:
https://wiki.openssl.org/index.php/Elliptic_Curve_Diffie_Hellman#ECDH_and_Nam...

tested with openssl 1.0.2j

BUG=webrtc:6763

Committed: https://crrev.com/bbfed52cf2912d5088abeb017049241e4d37d21f
Cr-Commit-Position: refs/heads/master@{#15536}
==========

[commit-bot: I haz the power, 2016-12-12 02:42:19]

Patchset 4 (id:??) landed as
https://crrev.com/bbfed52cf2912d5088abeb017049241e4d37d21f
Cr-Commit-Position: refs/heads/master@{#15536}