Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1823213002:
Don't override curve preferences in BoringSSL. (Closed)
|
Created:
4 years, 9 months ago by davidben_webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't override curve preferences in BoringSSL.
BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as either client or server.
R=torbjorng@webrtc.org
TBR=juberti@webrtc.org
BUG=webrtc:5674
Committed: https://crrev.com/60d5f3f4b7d6448ade12be703951816de2402ffd
Cr-Commit-Position: refs/heads/master@{#12120}
Patch Set 1 #Patch Set 2 : rebase #Messages
Total messages: 30 (14 generated)
davidben@webrtc.org changed reviewers: + torbjorng@webrtc.org
torbjorng: I've left the #ifdefs in for now in case you end up needing to temporarily revert the vanilla OpenSSL removal.
On 2016/03/22 18:57:32, davidben_webrtc wrote: > torbjorng: I've left the #ifdefs in for now in case you end up needing to > temporarily revert the vanilla OpenSSL removal. (Hrm. Apparently I can't run try jobs?)
The CQ bit was checked by torbjorng@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823213002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823213002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as a server. BUG=5674 ========== to ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as a server. BUG=webrtc:5674 ==========
torbjorng@webrtc.org changed reviewers: + perkj@webrtc.org
lgtm, assuming you're confident about the client side is not affected in a compatibility affecting way. Yay, allowing e.g. X25519 as a server is great! I lack API knowledge to be sure this does not cause client side problems; this code is shared for client and server side startup. I triggered the bots, and as you see they are green. I added an owner for rs lgtm.
Description was changed from ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as a server. BUG=webrtc:5674 ========== to ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as either client or server. BUG=webrtc:5674 ==========
On 2016/03/23 11:57:56, torbjorng (webrtc) wrote: > lgtm, assuming you're confident about the client side is not affected in a > compatibility affecting way. > > Yay, allowing e.g. X25519 as a server is great! > > I lack API knowledge to be sure this does not cause client side problems; this > code is shared for client and server side startup. Oh huh. Actually I think X25519 was getting disabled by both client and server. I borrowed an idea from OpenSSL at some point and made set_tmp_ecdh a compatibility wrapper over set1_curves, but it hadn't occurred to me that means it affected both. So I think X25519 was getting disabled in both directions, and with this CL we'll have it enabled both ways. Tweaked the commit message slightly accordingly.
lgtm
torbjorng@webrtc.org changed reviewers: + juberti@webrtc.org - perkj@webrtc.org
PTAL, juberti.
On 2016/03/23 14:55:02, torbjorng (webrtc) wrote: > PTAL, juberti. lgtm
The CQ bit was checked by torbjorng@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823213002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823213002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4407)
Description was changed from ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as either client or server. BUG=webrtc:5674 ========== to ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as either client or server. TBR=juberti@webrtc.org BUG=webrtc:5674 ==========
The CQ bit was checked by torbjorng@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823213002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823213002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
(Landing manually per torbjorng.)
Description was changed from ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as either client or server. TBR=juberti@webrtc.org BUG=webrtc:5674 ========== to ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as either client or server. R=torbjorng@webrtc.org TBR=juberti@webrtc.org BUG=webrtc:5674 Committed: https://chromium.googlesource.com/external/webrtc/+/60d5f3f4b7d6448ade12be703... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 60d5f3f4b7d6448ade12be703951816de2402ffd (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as either client or server. R=torbjorng@webrtc.org TBR=juberti@webrtc.org BUG=webrtc:5674 Committed: https://chromium.googlesource.com/external/webrtc/+/60d5f3f4b7d6448ade12be703... ========== to ========== Don't override curve preferences in BoringSSL. BoringSSL has since fixed OpenSSL's API wart and can do ECDHE by default as a server. Notably, removing this call means that X25519 may be used as either client or server. R=torbjorng@webrtc.org TBR=juberti@webrtc.org BUG=webrtc:5674 Committed: https://crrev.com/60d5f3f4b7d6448ade12be703951816de2402ffd Cr-Commit-Position: refs/heads/master@{#12120} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/60d5f3f4b7d6448ade12be703951816de2402ffd Cr-Commit-Position: refs/heads/master@{#12120} |
