|
|
Created:
3 years, 7 months ago by davidben Modified:
3 years, 7 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUnit-test CAPI and CNG SSLPrivateKey implementations.
This does not cover the CryptAcquireCertificatePrivateKey call (from my
manual testing, it didn't appear that the imported keys showed up
via CryptAcquireCertificatePrivateKey at all), but it tests that we are calling
CAPI and CNG correctly for the algorithms we are trying to implement. This
will be useful when we revise the interface for RSA-PSS and add support for
it. Also, it means we actually have some test for this stuff!
BUG=673058
Review-Url: https://codereview.chromium.org/2865883002
Cr-Commit-Position: refs/heads/master@{#470612}
Committed: https://chromium.googlesource.com/chromium/src/+/d6d1e4d17563f475169ea8564be5e581759eb8fd
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : reorder headers #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : . #Patch Set 8 : . #Patch Set 9 : . #
Total comments: 9
Patch Set 10 : rsleevi comments #Patch Set 11 : rebase #
Messages
Total messages: 61 (45 generated)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Work in progress SSLPlatformKeyWin tests. BUG=??? ========== to ========== Work in progress SSLPlatformKeyWin tests. BUG=673058 ==========
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by davidben@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Work in progress SSLPlatformKeyWin tests. BUG=673058 ========== to ========== Unit-test CAPI and CNG SSLPrivateKey implementations. This does not cover the CryptAcquireCertificatePrivateKey call (from my manual testing, it didn't appear that the imported keys showed up via CryptAcquireCertificatePrivateKey at all), but it tests that we are calling CAPI and CNG correctly for the algorithms we are trying to implement. This will be useful when we revise the interface for RSA-PSS and add support for it. Also, it means we actually have some test for this stuff! BUG=673058 ==========
davidben@chromium.org changed reviewers: + rsleevi@chromium.org
Amazingly, this seems to actually work! Ryan, does this seem sound based on your understanding of CAPI and CNG?
https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_win_unittest.cc (right): https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win_unittest.cc:183: header.dwMagic = BCRYPT_ECDSA_PRIVATE_P521_MAGIC; The MSDN docs call this field 'Magic', but it seems it is actually called 'dwMagic'.
LGTM https://codereview.chromium.org/2865883002/diff/160001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2865883002/diff/160001/net/BUILD.gn#newcode5278 net/BUILD.gn:5278: "ncrypt.lib", Why this versus the #pragma? https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_win_unittest.cc (right): https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win_unittest.cc:10: #include <NCrypt.h> These are in _win.h, but you removed them from _win.cc - why add them here? https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win_unittest.cc:63: bool AddBIGNUMLittleEndian(CBB* cbb, const BIGNUM* bn, size_t len) { Document? https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win_unittest.cc:68: bool PKCS8ToBLOBForCAPI(const std::string& pkcs8, std::vector<uint8_t>* blob) { Document?
https://codereview.chromium.org/2865883002/diff/160001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2865883002/diff/160001/net/BUILD.gn#newcode5278 net/BUILD.gn:5278: "ncrypt.lib", On 2017/05/08 21:01:41, Ryan Sleevi wrote: > Why this versus the #pragma? https://cs.chromium.org/chromium/src/PRESUBMIT.py?type=cs&q=pragma.*comment&s... https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... File net/ssl/ssl_platform_key_win_unittest.cc (right): https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win_unittest.cc:10: #include <NCrypt.h> On 2017/05/08 21:01:41, Ryan Sleevi wrote: > These are in _win.h, but you removed them from _win.cc - why add them here? Removed. https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win_unittest.cc:63: bool AddBIGNUMLittleEndian(CBB* cbb, const BIGNUM* bn, size_t len) { On 2017/05/08 21:01:41, Ryan Sleevi wrote: > Document? Done. https://codereview.chromium.org/2865883002/diff/160001/net/ssl/ssl_platform_k... net/ssl/ssl_platform_key_win_unittest.cc:68: bool PKCS8ToBLOBForCAPI(const std::string& pkcs8, std::vector<uint8_t>* blob) { On 2017/05/08 21:01:41, Ryan Sleevi wrote: > Document? Done.
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2865883002/#ps180001 (title: "rsleevi comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494348285901380, "parent_rev": "c52544194f2bf97e72cf9215f2c3d40928929036", "commit_rev": "370e7da99d7582d214199a1699fa03d4738c5b7a"}
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494348285901380, "parent_rev": "c52544194f2bf97e72cf9215f2c3d40928929036", "commit_rev": "370e7da99d7582d214199a1699fa03d4738c5b7a"}
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494348285901380, "parent_rev": "c52544194f2bf97e72cf9215f2c3d40928929036", "commit_rev": "370e7da99d7582d214199a1699fa03d4738c5b7a"}
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by davidben@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2865883002/#ps200001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494428343837980, "parent_rev": "ca53b45abadbea9b9fd422296ce4a973947666ff", "commit_rev": "d6d1e4d17563f475169ea8564be5e581759eb8fd"}
Message was sent while issue was closed.
Description was changed from ========== Unit-test CAPI and CNG SSLPrivateKey implementations. This does not cover the CryptAcquireCertificatePrivateKey call (from my manual testing, it didn't appear that the imported keys showed up via CryptAcquireCertificatePrivateKey at all), but it tests that we are calling CAPI and CNG correctly for the algorithms we are trying to implement. This will be useful when we revise the interface for RSA-PSS and add support for it. Also, it means we actually have some test for this stuff! BUG=673058 ========== to ========== Unit-test CAPI and CNG SSLPrivateKey implementations. This does not cover the CryptAcquireCertificatePrivateKey call (from my manual testing, it didn't appear that the imported keys showed up via CryptAcquireCertificatePrivateKey at all), but it tests that we are calling CAPI and CNG correctly for the algorithms we are trying to implement. This will be useful when we revise the interface for RSA-PSS and add support for it. Also, it means we actually have some test for this stuff! BUG=673058 Review-Url: https://codereview.chromium.org/2865883002 Cr-Commit-Position: refs/heads/master@{#470612} Committed: https://chromium.googlesource.com/chromium/src/+/d6d1e4d17563f475169ea8564be5... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/d6d1e4d17563f475169ea8564be5... |