Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1965723002:
FakeDtlsIdentityStore supporting both RSA and ECDSA. (Closed)
DescriptionFakeDtlsIdentityStore supporting both RSA and ECDSA.
Previously it only supported RSA-1024/0x10001, now it also supports ECDSA-P256.
This will be necessary for when KT_DEFAULT changes from KT_RSA to KT_ECDSA
since FakeDtlsIdentityStore is used by many tests.
BUG=webrtc:5795
R=mattdr@google.com, tommi@webrtc.org
Committed: https://crrev.com/db7bd3a586be5629012f2edd9095c530d743efcf
Cr-Commit-Position: refs/heads/master@{#12680}
Patch Set 1 : Run bots with KT_DEFAULT = KT_ECDSA to see what other problems exist with switching default #Patch Set 2 : KT_DEFAULT = KT_RSA again #
Total comments: 4
Patch Set 3 : Added comment #Messages
Total messages: 16 (7 generated)
Description was changed from ========== FakeDtlsIdentityStore to support both RSA and ECDSA. This will be necessary before chaning the default to ECDSA. BUG=webrtc:5795 ========== to ========== FakeDtlsIdentityStore supporting both RSA and ECDSA. Previously it only supported RSA-1024/0x10001, now it also supports ECDSA-P256. This will be necessary for when KT_DEFAULT changes from KT_RSA to KT_ECDSA since FakeDtlsIdentityStore is used by many tests. BUG=webrtc:5795 ==========
In patch set 1, the bots fail because various peerconnection_unittests complain about the fingerprint in a string constant not being the same as the identity's fingerprint. This is not a problem with the FakeDtlsIdentityStore, creating a different identity than before will yield a different fingerprint, which happens if the fake store is told to generate ECDSA instead of RSA. I don't believe there should be an assumption in peerconnection_unittests about what the resulting fingerprint will be unless that test explicitly specifies which certificate (identity) is used? Unless perhaps we make that constant part of fakedtlsidentitystore.h. This problem needs to be addressed, but we can address it separately since KT_DEFAULT is not changing in this CL.
On 2016/05/10 11:44:39, hbos wrote: > In patch set 1, the bots fail because various peerconnection_unittests complain > about the fingerprint in a string constant not being the same as the identity's > fingerprint. This is not a problem with the FakeDtlsIdentityStore, creating a > different identity than before will yield a different fingerprint, which happens > if the fake store is told to generate ECDSA instead of RSA. The same thing happens if I update the PEM strings so that a different RSA certificate is used and KT_DEFAULT remains KT_RSA. The fingerprint in peerconnectioninterface_unittest.cc results in a hidden assumption that a specific certificate is used.
hbos@webrtc.org changed reviewers: + mattdr@google.com, mattdr@webrtc.org, tommi@webrtc.org
Please take a look tommi and mattdr.
lgtm % 1 rfc https://codereview.webrtc.org/1965723002/diff/20001/webrtc/api/test/fakedtlsi... File webrtc/api/test/fakedtlsidentitystore.h (right): https://codereview.webrtc.org/1965723002/diff/20001/webrtc/api/test/fakedtlsi... webrtc/api/test/fakedtlsidentitystore.h:29: "5aTSMsbbkZ+C/OzTnbiMqLL/vg6jAgMBAAECgYAvgOs4FJcgvp+TuREx7YtiYVsH\n" don't you mean "5aTSMsbbkZ+C/OzTnbiMqLL/vg6jAgMBAAECgYAvgOs4FJcgvp+TuREx7YtiYVsh\n"? https://codereview.webrtc.org/1965723002/diff/20001/webrtc/api/test/fakedtlsi... webrtc/api/test/fakedtlsidentitystore.h:141: RTC_DCHECK(!expires_ms); can you add a comment for why this requirement is here?
lgtm lgtm
https://codereview.webrtc.org/1965723002/diff/20001/webrtc/api/test/fakedtlsi... File webrtc/api/test/fakedtlsidentitystore.h (right): https://codereview.webrtc.org/1965723002/diff/20001/webrtc/api/test/fakedtlsi... webrtc/api/test/fakedtlsidentitystore.h:29: "5aTSMsbbkZ+C/OzTnbiMqLL/vg6jAgMBAAECgYAvgOs4FJcgvp+TuREx7YtiYVsH\n" On 2016/05/10 16:09:01, tommi-webrtc wrote: > don't you mean > "5aTSMsbbkZ+C/OzTnbiMqLL/vg6jAgMBAAECgYAvgOs4FJcgvp+TuREx7YtiYVsh\n"? Haha, ofc, rookie mistake https://codereview.webrtc.org/1965723002/diff/20001/webrtc/api/test/fakedtlsi... webrtc/api/test/fakedtlsidentitystore.h:141: RTC_DCHECK(!expires_ms); On 2016/05/10 16:09:01, tommi-webrtc wrote: > can you add a comment for why this requirement is here? Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, mattdr@google.com Link to the patchset: https://codereview.webrtc.org/1965723002/#ps40001 (title: "Added comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1965723002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1965723002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
Description was changed from ========== FakeDtlsIdentityStore supporting both RSA and ECDSA. Previously it only supported RSA-1024/0x10001, now it also supports ECDSA-P256. This will be necessary for when KT_DEFAULT changes from KT_RSA to KT_ECDSA since FakeDtlsIdentityStore is used by many tests. BUG=webrtc:5795 ========== to ========== FakeDtlsIdentityStore supporting both RSA and ECDSA. Previously it only supported RSA-1024/0x10001, now it also supports ECDSA-P256. This will be necessary for when KT_DEFAULT changes from KT_RSA to KT_ECDSA since FakeDtlsIdentityStore is used by many tests. BUG=webrtc:5795 R=mattdr@google.com, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/db7bd3a586be5629012f2edd9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as db7bd3a586be5629012f2edd9095c530d743efcf (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== FakeDtlsIdentityStore supporting both RSA and ECDSA. Previously it only supported RSA-1024/0x10001, now it also supports ECDSA-P256. This will be necessary for when KT_DEFAULT changes from KT_RSA to KT_ECDSA since FakeDtlsIdentityStore is used by many tests. BUG=webrtc:5795 R=mattdr@google.com, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/db7bd3a586be5629012f2edd9... ========== to ========== FakeDtlsIdentityStore supporting both RSA and ECDSA. Previously it only supported RSA-1024/0x10001, now it also supports ECDSA-P256. This will be necessary for when KT_DEFAULT changes from KT_RSA to KT_ECDSA since FakeDtlsIdentityStore is used by many tests. BUG=webrtc:5795 R=mattdr@google.com, tommi@webrtc.org Committed: https://crrev.com/db7bd3a586be5629012f2edd9095c530d743efcf Cr-Commit-Position: refs/heads/master@{#12680} ========== |
