Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(651)

Issue 1106103003: Don't use RSAPrivateKey in NSS integration code. (Closed)

Created:
5 years, 8 months ago by davidben
Modified:
5 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, dzhioev+watch_chromium.org, stevenjb+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@ocsp-refactor
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't use RSAPrivateKey in NSS integration code. Currently some NSS platform integration logic transits private keys through RSAPrivateKey on CrOS. This prevents incrementally switching RSAPrivateKey to BoringSSL while keeping platform integrations on NSS. The intent of this change is to clarify RSAPrivateKey as a BoringSSL vs NSS internal crypto library (use_openssl=0 vs use_openssl=1) abstraction. It's primarily to be used with SignatureCreator. Code which uses NSS based on use_nss_certs rather than use_openssl because the underlying platform is NSS should call NSS routines directly, or introduce different abstractions. Remove the problematic RSAPrivateKey methods and instead add crypto/nss_key_util.h which contains some helper functions for manipulating NSS keys. This is sufficient to allow consumers of the removed methods to use NSS directly with about as much code. (This should not set back migrating that logic to NSS as that code was already very NSS-specific; those APIs assumed PK11SlotInfo.) nss_key_util.h, like nss_util.h, is built whenever NSS is used either internally or for platform integrations. This is so rsa_private_key_nss.cc can continue to use the helper functions to implement the NSS-agnostic interface. With this, the chimera CrOS configuration should build. The RSAPrivateKey logic is functional with the exception of some logic in components/ownership. That will be resolved in a future CL. BUG=478777 Committed: https://crrev.com/a46a990b2ccae2b66e87b5f76d2866044dc3182e Cr-Commit-Position: refs/heads/master@{#327909}

Patch Set 1 #

Patch Set 2 : missing include #

Patch Set 3 : build dependency #

Patch Set 4 : avoid exposing NSS through net headers (unnecessary) #

Total comments: 20

Patch Set 5 : sleevi comments #

Total comments: 45

Patch Set 6 : pneubeck comments #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+452 lines, -430 lines) Patch
M chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc View 1 2 3 4 5 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc View 1 2 3 4 8 chunks +20 lines, -18 lines 0 comments Download
M chrome/browser/chromeos/ownership/owner_settings_service_chromeos.cc View 3 chunks +6 lines, -5 lines 0 comments Download
M chrome/browser/chromeos/platform_keys/platform_keys_nss.cc View 1 2 3 4 6 chunks +20 lines, -16 lines 0 comments Download
M components/ownership.gypi View 1 2 1 chunk +5 lines, -0 lines 2 comments Download
M components/ownership/BUILD.gn View 1 2 2 chunks +5 lines, -0 lines 0 comments Download
M components/ownership/owner_key_util_impl.cc View 2 chunks +17 lines, -1 line 0 comments Download
M crypto/BUILD.gn View 1 2 3 4 4 chunks +14 lines, -9 lines 0 comments Download
M crypto/crypto.gyp View 1 2 3 4 4 chunks +8 lines, -8 lines 0 comments Download
M crypto/crypto.gypi View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A crypto/nss_key_util.h View 1 2 3 4 1 chunk +58 lines, -0 lines 0 comments Download
A crypto/nss_key_util.cc View 1 2 3 4 5 1 chunk +161 lines, -0 lines 0 comments Download
A crypto/nss_key_util_unittest.cc View 1 2 3 4 5 1 chunk +87 lines, -0 lines 0 comments Download
M crypto/rsa_private_key.h View 2 chunks +1 line, -68 lines 0 comments Download
M crypto/rsa_private_key_nss.cc View 1 7 chunks +26 lines, -207 lines 0 comments Download
D crypto/rsa_private_key_nss_unittest.cc View 1 chunk +0 lines, -66 lines 0 comments Download
M net/net.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M net/test/cert_test_util.h View 1 2 3 2 chunks +6 lines, -9 lines 0 comments Download
M net/test/cert_test_util_nss.cc View 1 2 3 4 5 2 chunks +10 lines, -19 lines 0 comments Download

Messages

Total messages: 31 (6 generated)
davidben
rsleevi and pneubeck for primary reviewers, since you're the people I associate with crypto and ...
5 years, 8 months ago (2015-04-27 18:07:22 UTC) #2
Ryan Sleevi
https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc (right): https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc#newcode127 chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:127: crypto::ScopedSECKEYPrivateKey private_key; Why the different naming? public_key_obj vs private_key ...
5 years, 8 months ago (2015-04-27 19:11:27 UTC) #3
davidben
https://codereview.chromium.org/1106103003/diff/60001/components/ownership/owner_key_util_impl.cc File components/ownership/owner_key_util_impl.cc (right): https://codereview.chromium.org/1106103003/diff/60001/components/ownership/owner_key_util_impl.cc#newcode70 components/ownership/owner_key_util_impl.cc:70: return nullptr; On 2015/04/27 19:11:27, Ryan Sleevi wrote: > ...
5 years, 8 months ago (2015-04-27 19:17:10 UTC) #4
davidben
https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc (right): https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc#newcode127 chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:127: crypto::ScopedSECKEYPrivateKey private_key; On 2015/04/27 19:11:26, Ryan Sleevi wrote: > ...
5 years, 8 months ago (2015-04-27 19:53:44 UTC) #5
pneubeck (no reviews)
several comments apply to the original code already, so feel free to punt these parts. ...
5 years, 8 months ago (2015-04-28 09:56:27 UTC) #6
davidben
https://codereview.chromium.org/1106103003/diff/80001/chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc (right): https://codereview.chromium.org/1106103003/diff/80001/chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc#newcode122 chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:122: crypto::ScopedSECKEYPrivateKey CreateOwnerKeyInSlot(PK11SlotInfo* slot) { On 2015/04/28 09:56:26, pneubeck wrote: ...
5 years, 7 months ago (2015-04-28 16:27:46 UTC) #7
pneubeck (no reviews)
I don't know about the components/ownership/ change, but the rest lgtm. https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc File crypto/nss_key_util.cc (right): ...
5 years, 7 months ago (2015-04-28 16:40:28 UTC) #8
davidben
+dpolukhin for chrome/browser/chromeos/ and components/ownership/ OWNERS. Also for questions re what do to here about ...
5 years, 7 months ago (2015-04-29 21:51:32 UTC) #10
pneubeck (no reviews)
https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc File crypto/nss_key_util.cc (right): https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#newcode140 crypto/nss_key_util.cc:140: int slot_count = item->module->loaded ? item->module->slotCount : 0; On ...
5 years, 7 months ago (2015-04-30 07:45:56 UTC) #11
Dmitry Polukhin
In general things looks good but I don't know how we build real Chrome for ...
5 years, 7 months ago (2015-04-30 08:44:45 UTC) #12
davidben
On 2015/04/30 08:44:45, Dmitry Polukhin wrote: > In general things looks good but I don't ...
5 years, 7 months ago (2015-04-30 13:35:38 UTC) #13
Dmitry Polukhin
lgtm
5 years, 7 months ago (2015-04-30 13:38:17 UTC) #14
Ryan Sleevi
lgtm
5 years, 7 months ago (2015-04-30 18:34:23 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106103003/100001
5 years, 7 months ago (2015-04-30 19:41:29 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/60324)
5 years, 7 months ago (2015-04-30 19:52:10 UTC) #19
davidben
+caitkp for components/ownership.gypi (hrm, perhaps that should be OWNERSd to the components/ownership folks)
5 years, 7 months ago (2015-04-30 22:37:30 UTC) #21
Cait (Slow)
I'm not too familiar with the ownership/ component. Please get a components/ownership OWNER (alemate@ or ...
5 years, 7 months ago (2015-05-01 14:04:02 UTC) #22
Ryan Sleevi
On 2015/05/01 14:04:02, Cait wrote: > I'm not too familiar with the ownership/ component. Please ...
5 years, 7 months ago (2015-05-01 14:05:21 UTC) #23
Ryan Sleevi
https://codereview.chromium.org/1106103003/diff/100001/components/ownership.gypi File components/ownership.gypi (right): https://codereview.chromium.org/1106103003/diff/100001/components/ownership.gypi#newcode40 components/ownership.gypi:40: '../build/linux/system.gyp:ssl', On 2015/05/01 14:04:01, Cait wrote: > Why are ...
5 years, 7 months ago (2015-05-01 14:07:41 UTC) #24
Cait (Slow)
Ah -- I see a components/ownership OWNER has already approved this CL, so please disregard ...
5 years, 7 months ago (2015-05-01 14:09:37 UTC) #25
Cait (Slow)
codereview needs a "new message" notification :\ sorry for the noise and thanks for clarification. ...
5 years, 7 months ago (2015-05-01 14:13:05 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1106103003/100001
5 years, 7 months ago (2015-05-01 15:31:49 UTC) #28
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 7 months ago (2015-05-01 15:36:06 UTC) #29
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/a46a990b2ccae2b66e87b5f76d2866044dc3182e Cr-Commit-Position: refs/heads/master@{#327909}
5 years, 7 months ago (2015-05-01 15:36:59 UTC) #30
spang
5 years, 7 months ago (2015-05-01 21:00:23 UTC) #31
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1118263003/ by spang@chromium.org.

The reason for reverting is: Causes SEGV during login on Chrome OS

BUG=483606.

Powered by Google App Engine
This is Rietveld 408576698