Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1888913005:
Add a hook to inject trusted Cast roots for testing purposes. (Closed)
|
Created:
4 years, 8 months ago by ryanchung Modified:
4 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a hook to inject trusted Cast roots for testing purposes.
BUG= None
Committed: https://crrev.com/6d2e4b2d33365b1b459be8ff139fbefa27265fcb
Cr-Commit-Position: refs/heads/master@{#389539}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addresses comments #Patch Set 3 : Addresses comments #Patch Set 4 : Rebased #Patch Set 5 : Renamed function #
Total comments: 4
Patch Set 6 : Added note to comment #
Total comments: 2
Patch Set 7 : Rebased #Patch Set 8 : Fixed nit #
Messages
Total messages: 25 (8 generated)
ryanchung@chromium.org changed reviewers: + sheretov@chromium.org
ryanchung@chromium.org changed reviewers: + eroman@chromium.org
The general API change to make net::TrustStore a parameter sgtm. Some nits on the implementation. https://codereview.chromium.org/1888913005/diff/1/extensions/common/cast/cast... File extensions/common/cast/cast_cert_validator.h (right): https://codereview.chromium.org/1888913005/diff/1/extensions/common/cast/cast... extensions/common/cast/cast_cert_validator.h:58: net::TrustAnchor CreateTrustAnchor(const uint8_t (&subject)[SubjectSize], A more useful API is probably: void AddTrustAnchor(____, net::TrustStore*); I suggest changing to something like that before exposing. Also as a side-note I am in the process of refactoring this to be: trust_store->AddTrustedCertificate(trusted_cert_der); As part of https://codereview.chromium.org/1890193003/ (that CL is not ready for your review yet, just mentioning it as an FYI). https://codereview.chromium.org/1888913005/diff/1/extensions/common/cast/cast... extensions/common/cast/cast_cert_validator.h:96: net::TrustStore trust_store) WARN_UNUSED_RESULT; const net::TrustStore& https://codereview.chromium.org/1888913005/diff/1/extensions/common/cast/cast... extensions/common/cast/cast_cert_validator.h:98: // Overloads VerifyDeviceCert(const std::vector<std::string>& certs, This comment could be simplified. // This is an overloaded version of VerifyDeviceCert() that uses the default Cast trust store.
Thanks. https://codereview.chromium.org/1888913005/diff/1/extensions/common/cast/cast... File extensions/common/cast/cast_cert_validator.h (right): https://codereview.chromium.org/1888913005/diff/1/extensions/common/cast/cast... extensions/common/cast/cast_cert_validator.h:58: net::TrustAnchor CreateTrustAnchor(const uint8_t (&subject)[SubjectSize], On 2016/04/18 18:28:40, eroman wrote: > A more useful API is probably: > > void AddTrustAnchor(____, net::TrustStore*); > > I suggest changing to something like that before exposing. > > Also as a side-note I am in the process of refactoring this to be: > > trust_store->AddTrustedCertificate(trusted_cert_der); > > As part of https://codereview.chromium.org/1890193003/ (that CL is not ready > for your review yet, just mentioning it as an FYI). I'm thinking of undoing this API change and waiting for your change to land first. Then we won't have to introduce this new API and remove it in a few days. What do you think? https://codereview.chromium.org/1888913005/diff/1/extensions/common/cast/cast... extensions/common/cast/cast_cert_validator.h:96: net::TrustStore trust_store) WARN_UNUSED_RESULT; On 2016/04/18 18:28:40, eroman wrote: > const net::TrustStore& Done. https://codereview.chromium.org/1888913005/diff/1/extensions/common/cast/cast... extensions/common/cast/cast_cert_validator.h:98: // Overloads VerifyDeviceCert(const std::vector<std::string>& certs, On 2016/04/18 18:28:40, eroman wrote: > This comment could be simplified. > > // This is an overloaded version of VerifyDeviceCert() that uses the default > Cast trust store. Done.
sure sgtm
BTW https://codereview.chromium.org/1890193003/ has landed, so you can rebase on top of that now.
Description was changed from ========== Overload VerifyDeviceCert so that the TrustStore can be passed as a parameter. BUG= None ========== to ========== Add a hook to inject trusted Cast roots for testing purposes. BUG= None ==========
Thanks. I've updated the CL. PTAL
https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/... File extensions/common/cast/cast_cert_validator.cc (right): https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/... extensions/common/cast/cast_cert_validator.cc:293: return CastTrustStore::Get().AddTrustedCertificateWithoutCopying(data, If you are using the "*WithoutCopying()" flavor, please indicate so in the documentation that |data| must remain valid for the duration of the program as it is not copied. Or alternately use AddTrustedCertificate() instead and it will make a copy of the data (which would be appropriate if the data came from a std::string rather than say an array from rodata). https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/... File extensions/common/cast/cast_cert_validator.h (right): https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/... extensions/common/cast/cast_cert_validator.h:94: bool AddTrustAnchorForTest(const uint8_t* data, Andrei suggested the name "AddDevTrustAnchorsForTest()" over email, what do you think about that? Assuming the dev trust anchors are fixed, this could internalize the certificate data into the implementation rather than exposing a fully generic interface. I am fine with either approach, so will defer to you/sheretov. (My only concern with internalizing the data is ensuring the compilation is smart enough to strip the unused data.)
https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/... File extensions/common/cast/cast_cert_validator.cc (right): https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/... extensions/common/cast/cast_cert_validator.cc:293: return CastTrustStore::Get().AddTrustedCertificateWithoutCopying(data, On 2016/04/22 05:58:54, eroman wrote: > If you are using the "*WithoutCopying()" flavor, please indicate so in the > documentation that |data| must remain valid for the duration of the program as > it is not copied. > > Or alternately use AddTrustedCertificate() instead and it will make a copy of > the data (which would be appropriate if the data came from a std::string rather > than say an array from rodata). Done, I added the warning. The certificate data will be hard coded so there's no need to copy the data. https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/... File extensions/common/cast/cast_cert_validator.h (right): https://codereview.chromium.org/1888913005/diff/80001/extensions/common/cast/... extensions/common/cast/cast_cert_validator.h:94: bool AddTrustAnchorForTest(const uint8_t* data, On 2016/04/22 05:58:54, eroman wrote: > Andrei suggested the name "AddDevTrustAnchorsForTest()" over email, what do you > think about that? > > Assuming the dev trust anchors are fixed, this could internalize the certificate > data into the implementation rather than exposing a fully generic interface. > > I am fine with either approach, so will defer to you/sheretov. > > (My only concern with internalizing the data is ensuring the compilation is > smart enough to strip the unused data.) We've decided to not bundle the test anchors for now. Also, Cast can ensure that the test root is not compiled into production code.
lgtm https://codereview.chromium.org/1888913005/diff/100001/extensions/common/cast... File extensions/common/cast/cast_cert_validator.h (right): https://codereview.chromium.org/1888913005/diff/100001/extensions/common/cast... extensions/common/cast/cast_cert_validator.h:92: // |data| must remain valid throughout the lifetime of the program. nit: "and not be mutated"
lgtm
ryanchung@chromium.org changed reviewers: + asargent@chromium.org
+asargent for //extensions approval
https://codereview.chromium.org/1888913005/diff/100001/extensions/common/cast... File extensions/common/cast/cast_cert_validator.h (right): https://codereview.chromium.org/1888913005/diff/100001/extensions/common/cast... extensions/common/cast/cast_cert_validator.h:92: // |data| must remain valid throughout the lifetime of the program. On 2016/04/22 20:03:19, eroman wrote: > nit: "and not be mutated" Done.
lgtm
The CQ bit was checked by ryanchung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org, sheretov@chromium.org Link to the patchset: https://codereview.chromium.org/1888913005/#ps140001 (title: "Fixed nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1888913005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1888913005/140001
Message was sent while issue was closed.
Description was changed from ========== Add a hook to inject trusted Cast roots for testing purposes. BUG= None ========== to ========== Add a hook to inject trusted Cast roots for testing purposes. BUG= None ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add a hook to inject trusted Cast roots for testing purposes. BUG= None ========== to ========== Add a hook to inject trusted Cast roots for testing purposes. BUG= None Committed: https://crrev.com/6d2e4b2d33365b1b459be8ff139fbefa27265fcb Cr-Commit-Position: refs/heads/master@{#389539} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6d2e4b2d33365b1b459be8ff139fbefa27265fcb Cr-Commit-Position: refs/heads/master@{#389539} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
