Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9415040:
Refactor TransportSecurityState. (Closed)
|
Created:
8 years, 10 months ago by palmer Modified:
8 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, wtc Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRefactor TransportSecurityState.
Do some minor "gcl lint" cleanup while here.
BUG=113280, 120373
TEST=net_unittests, browser_tests, unit_tests TransportSecurityPersisterTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134754
Patch Set 1 #
Total comments: 50
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 21
Patch Set 4 : #Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Patch Set 7 : #
Total comments: 68
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #
Total comments: 36
Patch Set 11 : #
Total comments: 4
Patch Set 12 : #Patch Set 13 : #Messages
Total messages: 25 (0 generated)
Obviously, this is not a complete CL — this is a piñata proposal for you all to comment on. Go nuts! :)
Nuts I shall go! I tried to avoid cheating and looking at either the existing implementation or the previous header, and instead approach it as-if this was a brand new implementation, and that I had familiarity with the specs. This was tremendously easy, as I'm not that familiar with the previous code, I have read both specs, and only reviewing the right side meant only have the code to read. ;-) At a high level, I still feel like there's a lot of functionality stuffed in here across various layers of responsibility. Perhaps this is the best approach, and that this truly is one "Responsibility" domain in terms of Single Responsibility, but I feel like there's still opportunity for some better abstraction. That said, if you feel like YAGNI (YAK-ni?), please let me know. https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... File net/base/transport_security_state.h (right): https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:26: // SHA1 and SHA256. TODO(palmer): Specify and implement that in Comment nit: Drop the "we" Comment nit: Place TODO's at the start of a line That said... These typedefs don't seem to really belong here. At best, it would seem better within either TransportSecurityState (as public typedefs) OR where SHA1Fingerprint is defined (x509_cert_types.h), rather than sticking it into the net {} namespace here. https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:49: // format. So, it feels very weird to have this interface directly interfacing with JSON and/or using a string for opaque reasons. I understand this matches the previous interface, but it feels like if you're trying to move to a more dynamic, in-memory representation, you should be able to have all the operations exposed that can help you mutate this class. If you don't want to expose the operations to mutate this class, perhaps some sort of visitor or interface that can handle JSON reading. It just feels like a lot of responsibility to chuck into this class. It's also weird that TransportSecurityState("") is needed, instead of just permitting TransportSecurityState() (if no built-in state) https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:57: enum UpgradeMode { What does this upgrade? Upgrade to HTTPS? Mode { MODE_DEFAULT, MODE_FORCE_HTTPS, } ? https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:101: bool IsMoreStrict(const DomainState& other) const; IsStricterThan? That said, both strictness and strength seem a bit like abstract concepts, so it might help to explain what they mean. Without having looked at the implementation yet, I would assume some hierarchy: 1) HSTS 2) HSTS w/ include subdomains 3) STS with 5 pins 3) STS with 3 pins 4) STS with 1 pin etc https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:136: // Optional; hashes of dynamically pinned SubjectPublicKeyInfos. (They nit: Mentioning the means at which it can be set seems like an implementation detail. Additionally, referencing chrome://net-internals seems like a 'layering' violation by mentioning the Chrome-implementation details (I don't think content/ layer for embedders exposes net-internals, for example) // Optional; hashes of dynamically pinned SPKIs. https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:157: class SerializationDelegate { Why the rename? Delegate is certainly the more common name for these things. When there are > 1 events, we add new virtual methods (that clients can then no-op), rather than add more delegate types. Plus I'm not fond of the added |Serialization| naming that limits the scope of what this can do. Is there any way that this can be done without locks? I mean, this class is base::NonThreadSafe, so why should locks even matter? https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:188: // |host|. (If there is no DomainState for |host|, |*result| is not You can remove the (). Also note http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... on the superflous comments. It feels like the implicit contract is that it does what it says it does on success (returns true, updates |*result), then the converse should also be true (returns false, doesn't update |*result|). I understand the (historic) reason why you spelled out the false behaviour, but I think it's ok to leave it implied/understood, as that's certainly the dominant pattern in our code. You could always change the if to iff if you have reservations about non-obviousness. https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:191: // If |sni_available| is true, searches the static pins defined for The whole |sni_available| doesn't feel like an aptly-named variable, in light of the recent refactoring, and that it hearkens back to SSLv3 vs TLSv1 versioning. |sni_enabled| perhaps? https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:202: // updated.) Same comment here as on 188 https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:238: bool Serialize(std::string* output) const; see comments re: serialization being part of this class interface https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:257: static bool IsGooglePinnedProperty(const std::string& host, This never quite felt like the right layer to stick this. Any ideas on moving this out, say into CFCR? https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:263: static void ReportUMAOnPinFailure(const std::string& host); This seems like it belongs somewhere else. Unfortunately, I don't know where might be appropriate. Either private to the implementation (automatically reporting UMAs) or up in the layer that calls this (and is UMA aware) https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:268: static bool GetPublicKeyHash(const X509Certificate& cert, It seems like this would be better suited in X509Certificate, not here. https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:282: static std::string CanonicalizeHost(const std::string& hostname); Does this really need to be part of the public interface? I feel like this should be a hidden implementation detail. https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:284: // Parses |side_info| as a side pin (TODO(agl): document the format). If TODOs on newlines at the end of the description. // Parses |side_info| as a side pin. If successful, returns // ... // A side pin ... // TODO(agl): Document the format of a side pin https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:290: static bool ParseSidePin(const base::StringPiece& leaf_spki, Another weird sort of utility function here that feels out of place for transport security, since it just deals with SPKI and fingerprints. https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:305: // history to an attacker reading the serialized state on disk. Since this class doesn't do any formal serialization to disk, it seems odd to mention. Does this mean that Serialize() stores in SHA256(CanonicalizeHost(domain))? Would it make more sense to document that there instead? https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:309: // from the command line). Since you mentioned |enabled_hosts_| keys, what is the format for these keys (my guess: CanonicalizeHost(domain), but that's non-obvious. https://chromiumcodereview.appspot.com/9415040/diff/1/net/base/transport_secu... net/base/transport_security_state.h:312: SerializationDelegate* serialization_delegate_; What's the ownership of this pointer? (I'm guessing no ownership at all). Which means that |delegate_| must remain valid for the lifetime of the class - something that should be documented in Set[Serialization]Delegate(). For some reason, I have this idea that the more Chromium-y pattern is to: a) Take the delegate in the constructor and mention the non-ownership b) Provide a base::ObserverList Having the function to set the delegate (at potentially any time during the lifetime of the class), and without formally documenting the way to unset it (I'm guessing |NULL| is valid input to unset, but it's not clear), is a bit odd.
http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:26: // SHA1 and SHA256. TODO(palmer): Specify and implement that in > Comment nit: Drop the "we" > Comment nit: Place TODO's at the start of a line Done. > These typedefs don't seem to really belong here. At best, it would seem better > within either TransportSecurityState (as public typedefs) OR where > SHA1Fingerprint is defined (x509_cert_types.h), rather than sticking it into the > net {} namespace here. I don't want to do too much work in one CL — this thing is going to be monstrous as it is. Eventually, yes, we'll do a real generic implementation in x509_cert_types; until then, this typedef gives callers some insulation against the change. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:49: // format. > So, it feels very weird to have this interface directly interfacing with JSON > and/or using a string for opaque reasons. > > I understand this matches the previous interface, but it feels like if you're > trying to move to a more dynamic, in-memory representation, you should be able > to have all the operations exposed that can help you mutate this class. Yeah, I know. I was trying to "refactor, but not too much". But you're right. I've added a utility function: bool ParseJSONSecurityState(const std::string& json, TransportSecurityState* state); Does that work? Also, I made the constructor take no arguments. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:57: enum UpgradeMode { On 2012/02/17 05:16:01, Ryan Sleevi wrote: > What does this upgrade? > > Upgrade to HTTPS? > > Mode { > MODE_DEFAULT, > MODE_FORCE_HTTPS, > } ? Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:101: bool IsMoreStrict(const DomainState& other) const; > Without having looked at the implementation yet, I would assume some hierarchy: > 1) HSTS > 2) HSTS w/ include subdomains > 3) STS with 5 pins > 3) STS with 3 pins > 4) STS with 1 pin > > etc The implementation is not even that well-defined, unfortunately. Fortunately, it is used in only one place with a "TODO(palmer): Reconsider this?" comment. So, let's reconsider this and just get rid of it. Maybe someday we'll want it again. The one place it is used is in EnableHost, to determine if a dynamic entry should override a static entry. So by getting rid of IsMoreStrict/IsStricterThan, we are changing policy (allowing weak dynamic entries to override static ones). However, we are hearing from site operators that they affirmatively want the ability to un-do pinning (and/or HSTS), so perhaps that is best. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:136: // Optional; hashes of dynamically pinned SubjectPublicKeyInfos. (They On 2012/02/17 05:16:01, Ryan Sleevi wrote: > nit: Mentioning the means at which it can be set seems like an implementation > detail. Additionally, referencing chrome://net-internals seems like a 'layering' > violation by mentioning the Chrome-implementation details (I don't think > content/ layer for embedders exposes net-internals, for example) > > // Optional; hashes of dynamically pinned SPKIs. Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:157: class SerializationDelegate { > Why the rename? Delegate is certainly the more common name for these things. > When there are > 1 events, we add new virtual methods (that clients can then > no-op), rather than add more delegate types. Wow, I bet that is confusing. But if that's the way we do it, OK. Renamed back to just Delegate. > Is there any way that this can be done without locks? I mean, this class is > base::NonThreadSafe, so why should locks even matter? I'm not sure; It Was Like That When I Got Here. svn blame says phajdan.jr wrote this: http://src.chromium.org/viewvc/chrome?view=rev&revision=103012 The safety guarantee/locking seems to be the calls to CalledOnValidThread in transport_security_state.cc. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:188: // |host|. (If there is no DomainState for |host|, |*result| is not On 2012/02/17 05:16:01, Ryan Sleevi wrote: > You can remove the (). > > Also note > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > on the superflous comments. > > It feels like the implicit contract is that it does what it says it does on > success (returns true, updates |*result), then the converse should also be true > (returns false, doesn't update |*result|). > > I understand the (historic) reason why you spelled out the false behaviour, but > I think it's ok to leave it implied/understood, as that's certainly the dominant > pattern in our code. > > You could always change the if to iff if you have reservations about > non-obviousness. Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:191: // If |sni_available| is true, searches the static pins defined for On 2012/02/17 05:16:01, Ryan Sleevi wrote: > The whole |sni_available| doesn't feel like an aptly-named variable, in light of > the recent refactoring, and that it hearkens back to SSLv3 vs TLSv1 versioning. > > |sni_enabled| perhaps? Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:202: // updated.) On 2012/02/17 05:16:01, Ryan Sleevi wrote: > Same comment here as on 188 Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:238: bool Serialize(std::string* output) const; > see comments re: serialization being part of this class interface Is it better to implement a const_iterator for TransportSecurityState (TSS:c_i::operator* returns a DomainState*)? In effect, TSS is a map of string => DomainState. It's just that the lookup is a bit weird due to CanonicalizeHost and include_subdomains. Otherwise it would be a straight-up map. Hence, expose an iterator, and let callers handle serialization in whatever way is appropriate for them, getting all JSON/et c. policy out of this class. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:257: static bool IsGooglePinnedProperty(const std::string& host, > This never quite felt like the right layer to stick this. Any ideas on moving > this out, say into CFCR? The implementation relies on private implementation details in transport_security_state.cc (such as comparison with kGoogleAcceptableCerts). So to move it to CFCR, we'd have to expose that currently-private stuff. And I'm not sure that's the right idea. But maybe it is. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:263: static void ReportUMAOnPinFailure(const std::string& host); > This seems like it belongs somewhere else. Unfortunately, I don't know where > might be appropriate. Either private to the implementation (automatically > reporting UMAs) or up in the layer that calls this (and is UMA aware) Unlike IsGooglePinnedProperty, it is fairly straightforward to move this up into the caller (which is net/socket/ssl_client_socket_nss.cc). http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:268: static bool GetPublicKeyHash(const X509Certificate& cert, On 2012/02/17 05:16:01, Ryan Sleevi wrote: > It seems like this would be better suited in X509Certificate, not here. Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:282: static std::string CanonicalizeHost(const std::string& hostname); > Does this really need to be part of the public interface? I feel like this > should be a hidden implementation detail. It looks like it's public for use in transport_security_state_unittest.cc. Does FRIEND_TEST allow us to keep this private? http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:284: // Parses |side_info| as a side pin (TODO(agl): document the format). If On 2012/02/17 05:16:01, Ryan Sleevi wrote: > TODOs on newlines at the end of the description. > > // Parses |side_info| as a side pin. If successful, returns > // ... > // A side pin ... > // TODO(agl): Document the format of a side pin Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:290: static bool ParseSidePin(const base::StringPiece& leaf_spki, > Another weird sort of utility function here that feels out of place for > transport security, since it just deals with SPKI and fingerprints. Agreed. Got a better candidate place? X509Certificate? http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:305: // history to an attacker reading the serialized state on disk. > Since this class doesn't do any formal serialization to disk, it seems odd to > mention. Agreed. > Does this mean that Serialize() stores in SHA256(CanonicalizeHost(domain))? Yes. > Would it make more sense to document that there instead? Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:309: // from the command line). > Since you mentioned |enabled_hosts_| keys, what is the format for these keys (my > guess: CanonicalizeHost(domain), but that's non-obvious. It's even less usable than that. It's base64encode(sha256(canonicalize_host?(domain))). Sob... http://scarybeastsecurity.blogspot.com/2011/04/fiddling-with-chromiums-new-ce... I sort of think that, instead, we should get rid of this feature and instead make chrome://net-internals/#hsts easier to use. Perhaps. agl, cevans, thoughts? http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:312: SerializationDelegate* serialization_delegate_; > What's the ownership of this pointer? (I'm guessing no ownership at all). Which > means that |delegate_| must remain valid for the lifetime of the class - > something that should be documented in Set[Serialization]Delegate(). > > For some reason, I have this idea that the more Chromium-y pattern is to: > a) Take the delegate in the constructor and mention the non-ownership > b) Provide a base::ObserverList > > Having the function to set the delegate (at potentially any time during the > lifetime of the class), and without formally documenting the way to unset it > (I'm guessing |NULL| is valid input to unset, but it's not clear), is a bit odd. I agree. I think there is no ownership at all, and so far everyone has simply been letting the delegate live as long as the class. I'm inclined to do: c) Take the delegate in the constructor and have TSS take ownership Thoughts?
Better! Some more feedback below. I'll take another pass tomorrow, this was just a cursory glance. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:49: // format. On 2012/03/05 23:47:51, Chris P. wrote: > > So, it feels very weird to have this interface directly interfacing with JSON > > and/or using a string for opaque reasons. > > > > I understand this matches the previous interface, but it feels like if you're > > trying to move to a more dynamic, in-memory representation, you should be able > > to have all the operations exposed that can help you mutate this class. > > Yeah, I know. I was trying to "refactor, but not too much". But you're right. > > I've added a utility function: > > bool ParseJSONSecurityState(const std::string& json, TransportSecurityState* > state); > > Does that work? > > Also, I made the constructor take no arguments. Works a hell of a lot better, thanks. Still not thrilled with the JSON portion being in this file, but I think the new design is much nicer. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:157: class SerializationDelegate { On 2012/03/05 23:47:51, Chris P. wrote: > > Why the rename? Delegate is certainly the more common name for these things. > > When there are > 1 events, we add new virtual methods (that clients can then > > no-op), rather than add more delegate types. > > Wow, I bet that is confusing. > > But if that's the way we do it, OK. Renamed back to just Delegate. Not really, since any inheritance from a nested class requires fully qualifying the name eg: class MySerializationDelegate : public TransportSecurityState::Delegate { // TransportSecurityState::Delegate implementation virtual void StateIsDirty(TransportSecurityState* state); }; http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:238: bool Serialize(std::string* output) const; On 2012/03/05 23:47:51, Chris P. wrote: > > see comments re: serialization being part of this class interface > > Is it better to implement a const_iterator for TransportSecurityState > (TSS:c_i::operator* returns a DomainState*)? > > In effect, TSS is a map of string => DomainState. It's just that the lookup is a > bit weird due to CanonicalizeHost and include_subdomains. Otherwise it would be > a straight-up map. Hence, expose an iterator, and let callers handle > serialization in whatever way is appropriate for them, getting all JSON/et c. > policy out of this class. +1. I recently did that for expiring_cache, and that's the policy taken by base::DictionaryValue and other types. It doesn't even need to derive from an STL iterator - we tend to be more verbose and use explicit CapitalizedMethods for custom iterators. eg: SomeType::Iterator it(my_some_type); for (; it.GetNext(); it.IsValid()) { std::string hostname = ...; ... } http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:257: static bool IsGooglePinnedProperty(const std::string& host, On 2012/03/05 23:47:51, Chris P. wrote: > > This never quite felt like the right layer to stick this. Any ideas on moving > > this out, say into CFCR? > > The implementation relies on private implementation details in > transport_security_state.cc (such as comparison with kGoogleAcceptableCerts). So > to move it to CFCR, we'd have to expose that currently-private stuff. And I'm > not sure that's the right idea. But maybe it is. Fundamentally, how does this differ from GetStaticDomainState() ? Simply checking if the static pins == kGoogleAcceptableCerts ? It would still seem like you could replace this with a lookup of the static domain state for a host and then compare the pin results, all at a higher layer. WDYT? http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:282: static std::string CanonicalizeHost(const std::string& hostname); On 2012/03/05 23:47:51, Chris P. wrote: > > Does this really need to be part of the public interface? I feel like this > > should be a hidden implementation detail. > > It looks like it's public for use in transport_security_state_unittest.cc. Does > FRIEND_TEST allow us to keep this private? Yup. You can use either FRIEND_TEST (declared between lines 274-275), if it's just one or two tests, or friend the test fixture and use TEST_F to have all unit tests derive from the test fixture. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:290: static bool ParseSidePin(const base::StringPiece& leaf_spki, On 2012/03/05 23:47:51, Chris P. wrote: > > Another weird sort of utility function here that feels out of place for > > transport security, since it just deals with SPKI and fingerprints. > > Agreed. Got a better candidate place? X509Certificate? Side pins always come in as an X.509 extension, right? If so, then it probably makes sense there, especially since we can support it on all the platforms without having to coerce into an NSS handle. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:312: SerializationDelegate* serialization_delegate_; On 2012/03/05 23:47:51, Chris P. wrote: > > What's the ownership of this pointer? (I'm guessing no ownership at all). > Which > > means that |delegate_| must remain valid for the lifetime of the class - > > something that should be documented in Set[Serialization]Delegate(). > > > > For some reason, I have this idea that the more Chromium-y pattern is to: > > a) Take the delegate in the constructor and mention the non-ownership > > b) Provide a base::ObserverList > > > > Having the function to set the delegate (at potentially any time during the > > lifetime of the class), and without formally documenting the way to unset it > > (I'm guessing |NULL| is valid input to unset, but it's not clear), is a bit > odd. > > I agree. I think there is no ownership at all, and so far everyone has simply > been letting the delegate live as long as the class. > > I'm inclined to do: > > c) Take the delegate in the constructor and have TSS take ownership > > Thoughts? That gets a bit messier - then objects which have-a TSS potentially have a loop or have to write detachable delegates. I think specifying non-ownership and must-outlive is fine, unless you can think of a reason that TSS might live a long time. http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:28: // TODO(palmer): Specify and implement that in net/base/x509_cert_types.h. File a bug, assign to you. Easier to track TODOs when they reference discrete bugs :) http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:55: struct NET_EXPORT DomainState { I think you've reached the point where this should be a class instead ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... ) http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:164: // The new state for |host| is persisted using the Delegate nit: Line wrapping http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:172: // The new state for |host| is persisted using the Delegate nit: Line wrapping http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:187: bool GetDomainState(DomainState* result, Per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param... This should be bool GetDomainState(const std::string& host, bool sni_enabled, DomainState* result) const; See also 203-205. http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:263: // successful, returns true and appends the hash of the public key that signed nit: Line wrapping.
http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:238: bool Serialize(std::string* output) const; On 2012/03/07 06:02:46, Ryan Sleevi wrote: > On 2012/03/05 23:47:51, Chris P. wrote: > > > see comments re: serialization being part of this class interface > > > > Is it better to implement a const_iterator for TransportSecurityState > > (TSS:c_i::operator* returns a DomainState*)? > > > > In effect, TSS is a map of string => DomainState. It's just that the lookup is > a > > bit weird due to CanonicalizeHost and include_subdomains. Otherwise it would > be > > a straight-up map. Hence, expose an iterator, and let callers handle > > serialization in whatever way is appropriate for them, getting all JSON/et c. > > policy out of this class. > > +1. I recently did that for expiring_cache, and that's the policy taken by > base::DictionaryValue and other types. It doesn't even need to derive from an > STL iterator - we tend to be more verbose and use explicit CapitalizedMethods > for custom iterators. > > eg: > SomeType::Iterator it(my_some_type); > for (; it.GetNext(); it.IsValid()) { > std::string hostname = ...; > ... > } Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:257: static bool IsGooglePinnedProperty(const std::string& host, > Fundamentally, how does this differ from GetStaticDomainState() ? Simply > checking if the static pins == kGoogleAcceptableCerts ? Correct. > It would still seem like you could replace this with a lookup of the static > domain state for a host and then compare the pin results, all at a higher layer. > WDYT? That would require making kGoogleAcceptableCerts public; currently, it is a private implementation detail in TSS. I'd like to keep it that way, because the existence of this function is already ugly enough as it is. Making its guts public increases the size and weirdness of our interface, which is the opposite of what this refactoring is about. So I think we should leave this wart alone. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:282: static std::string CanonicalizeHost(const std::string& hostname); Turns out CanonicalizeHost needs to be public anyway, so that chrome/browser/transport_security_persister.h can use it to obfuscate hostnames while persisting. obfuscated_hostname = sha256(CanonicalizeHost(hostname)) http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:290: static bool ParseSidePin(const base::StringPiece& leaf_spki, On 2012/03/07 06:02:46, Ryan Sleevi wrote: > On 2012/03/05 23:47:51, Chris P. wrote: > > > Another weird sort of utility function here that feels out of place for > > > transport security, since it just deals with SPKI and fingerprints. > > > > Agreed. Got a better candidate place? X509Certificate? > > Side pins always come in as an X.509 extension, right? If so, then it probably > makes sense there, especially since we can support it on all the platforms > without having to coerce into an NSS handle. Done. http://codereview.chromium.org/9415040/diff/1/net/base/transport_security_sta... net/base/transport_security_state.h:312: SerializationDelegate* serialization_delegate_; > I think specifying non-ownership and must-outlive is fine, unless you can think > of a reason that TSS might live a long time. TSS is per-profile, so they do live a long time. But in the existing code, the Delegate lives the same life as the TSS. So I think it is ok to pass the Delegate to the TSS constructor and document that the caller owns the Delegate. http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:28: // TODO(palmer): Specify and implement that in net/base/x509_cert_types.h. On 2012/03/07 06:02:46, Ryan Sleevi wrote: > File a bug, assign to you. Easier to track TODOs when they reference discrete > bugs :) Done. http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:55: struct NET_EXPORT DomainState { On 2012/03/07 06:02:46, Ryan Sleevi wrote: > I think you've reached the point where this should be a class instead ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... > ) Done. http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:164: // The new state for |host| is persisted using the Delegate On 2012/03/07 06:02:46, Ryan Sleevi wrote: > nit: Line wrapping Done. http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:172: // The new state for |host| is persisted using the Delegate On 2012/03/07 06:02:46, Ryan Sleevi wrote: > nit: Line wrapping Done. http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:187: bool GetDomainState(DomainState* result, On 2012/03/07 06:02:46, Ryan Sleevi wrote: > Per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param... > > This should be > > bool GetDomainState(const std::string& host, > bool sni_enabled, > DomainState* result) const; > > See also 203-205. Done. http://codereview.chromium.org/9415040/diff/4001/net/base/transport_security_... net/base/transport_security_state.h:263: // successful, returns true and appends the hash of the public key that signed On 2012/03/07 06:02:46, Ryan Sleevi wrote: > nit: Line wrapping. Done.
Still feels like there's some tight coupling here between the HTTP logic and the access/storage of pinned state. Comments below http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:47: : delegate_(delegate); This belongs in the .cc file. http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:53: class NET_EXPORT DomainState { Are you sure you need to NET_EXPORT this? My understanding was that the child types would automatically be included by way of their parents export. http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:54: enum UpgradeMode { nit: Missing public public: enum UpgradeMode { http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:71: bool ParseSTSHeader(const std::string& value); ParsePinsHeader and ParseSTSHeader both seem like they fit elsewhere (net/http?). I'm just thinking that most of our header-related code chills over there - net/http/http_response_headers.h, net/http/http_auth.h - heck, even net/http/http_mac_signature.h lives there. http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:100: UpgradeMode upgrade_mode; DomainState is meant to be immutable after creation, right? Should these all be private: instead? I'm guessing 'no' if you want to move the deserialization into another class - but something to consider in terms of general API exposure. http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:161: std::map<std::string, DomainState>::const_iterator iterator_; Can you cheat with a typedef here (more aptly, put the typedef at line 250, and use that typedef here) Since TSS isn't a class, you shouldn't have to worry about 2-pass evaluation, and can leave the typedef private (which this class can access by virtue of being a member class) http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:236: static bool ParsePin(const std::string& value, Fingerprint* out); See above comments about HTTP header parsing above. http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h... net/base/x509_certificate.h:248: const SHA1Fingerprint GetPublicKeyHash() const; No need to "const SHA1Fingerprint" since you're returning a copy/newly allocated structure (it would only be necessary if it was stored as a member variable) Although I think you mentioned you were staticizing this, which is good. http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h... net/base/x509_certificate.h:578: FingerprintVector* out_pub_key_hash); So I think this would be better in TSS (or something like it). My comment about adding an X509Certificate accessor was related to the net/socket/ssl_client_socket_nss.cc parsing (which can be a separate CL than this). There, I was suggesting something like static bool GetSidePin(OSCertHandle handle, std::string* side_pin); Where GetSidePin() handles the locating of the OID in the cert extensions array (1.3.6.1.4.1.11129.2.1.4) and grabbing that data. The interpretation of that data can remain in TSS.
http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:47: : delegate_(delegate); On 2012/03/15 03:51:15, Ryan Sleevi wrote: > This belongs in the .cc file. Done. http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:53: class NET_EXPORT DomainState { On 2012/03/15 03:51:15, Ryan Sleevi wrote: > Are you sure you need to NET_EXPORT this? My understanding was that the child > types would automatically be included by way of their parents export. Done. http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:54: enum UpgradeMode { On 2012/03/15 03:51:15, Ryan Sleevi wrote: > nit: Missing public > > public: > enum UpgradeMode { Done. http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:71: bool ParseSTSHeader(const std::string& value); > ParsePinsHeader and ParseSTSHeader both seem like they fit elsewhere > (net/http?). Well, the implementations of these two functions legitimately need to noodle around in the private fields of TSS::DS. To me, that's a sure sign of "should be a method" instead of "should be a utility function". Allowing net/http/foo to update TSS::DS would require exposing various getters and setters, which would increase the size of this interface. I don't currently think that's worth it, since one of my goals here is to keep this interface small-ish. Perhaps that will prove impossible... http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:100: UpgradeMode upgrade_mode; > DomainState is meant to be immutable after creation, right? Should these all be > private: instead? > > I'm guessing 'no' if you want to move the deserialization into another class - > but something to consider in terms of general API exposure. Making them private would require at least getters and possibly setters; furthermore, DomainState is not necessarily immutable after creation. Calling GetDomainState, updating it, and then calling EnableHost is perfectly legit. I suppose you could therefore argue that I should now go full-on with the accessors... http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:161: std::map<std::string, DomainState>::const_iterator iterator_; On 2012/03/15 03:51:15, Ryan Sleevi wrote: > Can you cheat with a typedef here (more aptly, put the typedef at line 250, and > use that typedef here) > > Since TSS isn't a class, you shouldn't have to worry about 2-pass evaluation, > and can leave the typedef private (which this class can access by virtue of > being a member class) I'm not sure what you mean. TSS is a class; what do I gain with a typedef? http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h... net/base/x509_certificate.h:248: const SHA1Fingerprint GetPublicKeyHash() const; On 2012/03/15 03:51:15, Ryan Sleevi wrote: > No need to "const SHA1Fingerprint" since you're returning a copy/newly allocated > structure (it would only be necessary if it was stored as a member variable) > > Although I think you mentioned you were staticizing this, which is good. Done. http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h... net/base/x509_certificate.h:578: FingerprintVector* out_pub_key_hash); On 2012/03/15 03:51:15, Ryan Sleevi wrote: > So I think this would be better in TSS (or something like it). > > My comment about adding an X509Certificate accessor was related to the > net/socket/ssl_client_socket_nss.cc parsing (which can be a separate CL than > this). > > There, I was suggesting something like > > static bool GetSidePin(OSCertHandle handle, > std::string* side_pin); > > Where GetSidePin() handles the locating of the OID in the cert extensions array > (1.3.6.1.4.1.11129.2.1.4) and grabbing that data. > > The interpretation of that data can remain in TSS. Done.
http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:46: TransportSecurityState(TransportSecurityState::Delegate* delegate) explicit http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h... net/base/x509_certificate.h:578: FingerprintVector* out_pub_key_hash); Note: this side pinning stuff was just a roadmap for Moxie to put TACK in. Since it doesn't look like he's going to be following it, it can just be removed.
I've marked a lot of things Done but haven't done a gcl upload yet. Don't worry, it's coming today. (It will come with an implementation of the refactored interface.) http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/10001/net/base/transport_security... net/base/transport_security_state.h:46: TransportSecurityState(TransportSecurityState::Delegate* delegate) On 2012/03/20 22:12:59, agl wrote: > explicit Done. http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/9415040/diff/10001/net/base/x509_certificate.h... net/base/x509_certificate.h:578: FingerprintVector* out_pub_key_hash); On 2012/03/20 22:12:59, agl wrote: > Note: this side pinning stuff was just a roadmap for Moxie to put TACK in. Since > it doesn't look like he's going to be following it, it can just be removed. Done.
Adding mirandac as OWNER for chrome/browser/profiles Adding eroman as OWNER for chrome/browser/ui/webui/net_internals Thanks!
lgtm for net_internals.cc http://codereview.chromium.org/9415040/diff/21024/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/9415040/diff/21024/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1009: // TODO(palmer): result->SetBoolean("preloaded", state.preloaded); Please add a bug association. http://codereview.chromium.org/9415040/diff/21024/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1017: result->SetString("preloaded_spki_hashes", hashes); Consider renaming it in the javascript too for consistency.
http://codereview.chromium.org/9415040/diff/21024/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/9415040/diff/21024/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1009: // TODO(palmer): result->SetBoolean("preloaded", state.preloaded); On 2012/03/23 21:34:29, eroman wrote: > Please add a bug association. Actually I got rid of the TODO, it was a previous note to myself that shouldn't have been gcl uploaded for review. http://codereview.chromium.org/9415040/diff/21024/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1017: result->SetString("preloaded_spki_hashes", hashes); On 2012/03/23 21:34:29, eroman wrote: > Consider renaming it in the javascript too for consistency. Done.
http://codereview.chromium.org/9415040/diff/20034/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/9415040/diff/20034/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1016: result->SetString("static_spki_hashes", hashes); You need to update the javascript too if you make this change. See src/chrome/browser/resources/net_internals/hsts_view.js
On 2012/03/23 22:59:24, eroman wrote: > http://codereview.chromium.org/9415040/diff/20034/chrome/browser/ui/webui/net... > File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): > > http://codereview.chromium.org/9415040/diff/20034/chrome/browser/ui/webui/net... > chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1016: > result->SetString("static_spki_hashes", hashes); > You need to update the javascript too if you make this change. > See src/chrome/browser/resources/net_internals/hsts_view.js LGTM for profiles/*.
nit party time. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (left): http://codereview.chromium.org/9415040/diff/20034/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:440: command_line.GetSwitchValueASCII(switches::kHstsHosts))); Is this no longer supported? Is there any security risks to users who assumed it was? Should we surface a butter-bar? http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... File chrome/browser/transport_security_persister.cc (right): http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:92: static ListValue* SPKIHashesToListValue(const FingerprintVector& hashes) { nit: The dominant pattern in Chromium code is to use unnamed namespaces for such helper classes, in part because of the general need due to enums/classes/constants/etc ( http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces ) This isn't a hard rule in the Chromium guide or the Google Open Source style guide that Chromium incorporates by reference, but it does encourage the use of unnamed namespaces ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... ) http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:108: const ListValue& pins) { nit: Parameter order: in -> out http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:124: CHECK(base::Base64Encode(hashed, &out)); Do you really want to crash the browser process here? Could you not just return an empty string and call it a day (like the next function) http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:147: if (!value.get() || !value->IsType(Value::TYPE_DICTIONARY)) nit: You can replace 147 - 150 with: DictionaryValue* dict_value; if (!value.get() || !value->GetAsDictionary(&dict_value)) return false; http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:173: (void) parsed->GetDouble("dynamic_spki_hashes_expiry", This method isn't WARN_UNUSED_RESULT, so is this (void) needed? http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:185: if (parsed->GetList("dynamic_spki_hashes", &pins_list)) nit: Since you re-use these strings in both serialization and deserialization, consider making them constants (in an unnamed namespace) to ensure any changes are kept in sync/reflected. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:195: << mode_string; What happened to spdy-only? Looks like you're dropping it to the floor. If it's deprecated, can you add a note about that? If it's not, and you set mode to something, should you also set dirtied to be safe? http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... File chrome/browser/transport_security_persister_unittest.cc (right): http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:75: "}"; I think you should add additional variants, such as the SPDY handling, if this was something previously surfaced to users via command-line or UI and may have been serialized. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:115: // This is a docs.google.com override. Could you expand this comment? It's not clear what it's an override of - an internal preload? http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... File net/base/transport_security_state.cc (right): http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:48: /* XXX Delete? http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:91: }*/ Delete? http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:95: if (GetDomainState(host, true, &existing_state) && nit: GetDomainState(host, true /*sni_enabled*/, &existing_state) ? It's not obvious why calling with |sni_enabled| should be true for EnableHost, so perhaps you could expand on the comment as well. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:100: // We don't store this value. nit: Drop the "we", expand on why this value isn't appropriate to be stored. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:342: // "pin-" algo "=" base64 [ ";" ... ] Update the ABNF for -01. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:400: // "max-age" "=" delta-seconds [ ";" "includeSubDomains" ] Definitely update this guy http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:765: static const uint32 kTagSPIN = 0x4e495053; This was related to side pinning - delete? http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:30: // with |SetDelegate| to persist the state to disk. Or with the constructor. Which is odd to have both methods. Could you explain more why this is? http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:35: // http://tools.ietf.org/html/draft-ietf-websec-key-pinning-01. http://tools.ietf.org/html/ietf-websec-strict-transport-sec to refer to whatever is the latest version (eg: currently -06 for STS) Same for ietf-websec-key-pinning Unless you're intentionally trying to indicate a specific draft revision. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:78: bool ParseSTSHeader(const base::Time& now, const std::string& value); nit: Could you file a TODO to see about moving these out? At some point I imagine we'll need to support either different versions and/or different extensions. I'd like to de-couple the HTTP header parsing from the logical representation. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:105: bool ShouldRedirectHTTPToHTTPS() const; It seems you're using this for other protocols. Should this be renamed? http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:171: void SetDelegate(Delegate* delegate); nit: Document http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:199: DomainState* result); nit: I originally wondered why these methods weren't const when just reading the header, and then it became clear when reading the implementation that you expire and remove entries. It would be good if you could clarify this in the comments (both here and at 213). http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:256: // If we have a Delegate, call its |StateIsDirty| function. nit: s/function/method Broader still: // If a Delegate is present, notify it that the internal // state has changed. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:16: #endif These do not belong in x509_certificate.cc If they were acceptable, they'd belong at line 42-44, as per http://dev.chromium.org/developers/coding-style#TOC-Code-formatting http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:37: #include "crypto/sha2.h" ?? http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:44: #endif Nor this http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:500: bool X509Certificate::GetPublicKeyHash(const OSCertHandle& cert, My gut is that because this builds directly on top of GetDEREncoded, it may not belong in X509Certificate at all. Do you anticipate further users of this beyond pinning, and could you explain where/why/how? http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:503: if (!GetDEREncoded(cert, &der_bytes)) This requires making a copy of the |cert| data. If you do think this is a re-usable method, then you should implement this within each of the _platform files, which can use the raw data when applicable (eg: NSS+Win, but not OS X, OpenSSL, or Android) http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.h... net/base/x509_certificate.h:55: typedef std::vector<Fingerprint> FingerprintVector; Both of these belong in x509_cert_types.h. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.h... net/base/x509_certificate.h:230: // Parses |cert|'s Subject Public Key Info structure, hashes it, nit: // Compute the SHA1Fingerprint of the Subject Public Key Info // for |cert|. On success, stores the fingerprint in // |fingerprint| and returns true. http://codereview.chromium.org/9415040/diff/20034/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/9415040/diff/20034/net/socket/ssl_client_socke... net/socket/ssl_client_socket_nss.cc:138: SECItem* encodedResponse, void *pwArg); This were typed such to match the underlying NSS style, so they are probably fine as is. If you are going to change them, change all four variables :) http://codereview.chromium.org/9415040/diff/20034/net/socket_stream/socket_st... File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/9415040/diff/20034/net/socket_stream/socket_st... net/socket_stream/socket_stream_job.cc:32: domain_state.ShouldRedirectHTTPToHTTPS()) { Does STS apply to non-HTTPS? I didn't think the WG had reached a consensus.
http://codereview.chromium.org/9415040/diff/20034/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (left): http://codereview.chromium.org/9415040/diff/20034/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:440: command_line.GetSwitchValueASCII(switches::kHstsHosts))); On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Is this no longer supported? > > Is there any security risks to users who assumed it was? > > Should we surface a butter-bar? Putting support back in, per cevans, but in a new way (now that de/serialization policy is in transport_security_persister.{h,cc}). http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... File chrome/browser/transport_security_persister.cc (right): http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:92: static ListValue* SPKIHashesToListValue(const FingerprintVector& hashes) { On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: The dominant pattern in Chromium code is to use unnamed namespaces for such > helper classes, in part because of the general need due to > enums/classes/constants/etc ( > http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces ) > > This isn't a hard rule in the Chromium guide or the Google Open Source style > guide that Chromium incorporates by reference, but it does encourage the use of > unnamed namespaces ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp... > ) Done. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:108: const ListValue& pins) { On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: Parameter order: in -> out > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... Done. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:124: CHECK(base::Base64Encode(hashed, &out)); On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Do you really want to crash the browser process here? > > Could you not just return an empty string and call it a day (like the next > function) Done. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:147: if (!value.get() || !value->IsType(Value::TYPE_DICTIONARY)) On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: You can replace 147 - 150 with: > > DictionaryValue* dict_value; > if (!value.get() || !value->GetAsDictionary(&dict_value)) > return false; Done. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:173: (void) parsed->GetDouble("dynamic_spki_hashes_expiry", > This method isn't WARN_UNUSED_RESULT, so is this (void) needed? No, it's not needed. Removed. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:185: if (parsed->GetList("dynamic_spki_hashes", &pins_list)) On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: Since you re-use these strings in both serialization and deserialization, > consider making them constants (in an unnamed namespace) to ensure any changes > are kept in sync/reflected. Done. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:195: << mode_string; On 2012/03/28 00:50:32, Ryan Sleevi wrote: > What happened to spdy-only? > > Looks like you're dropping it to the floor. If it's deprecated, can you add a > note about that? > > If it's not, and you set mode to something, should you also set dirtied to be > safe? It had been deprecated for a long time, and a search of the code showed it was not used. So yeah I'm dropping it. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... File chrome/browser/transport_security_persister_unittest.cc (right): http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:75: "}"; > I think you should add additional variants, such as the SPDY handling, if this > was something previously surfaced to users via command-line or UI and may have > been serialized. It was never used. From a source tree that does not have this refactoring CL: ~/chromium/src $ search -v -n '\.(h|cpp)$' -C SPDY_ONLY ./net/base/transport_security_state.h:50: // SPDY_ONLY (aka X-Bodge-Transport-Security) is a hopefully temporary ./net/base/transport_security_state.h:54: MODE_SPDY_ONLY = 2, ./net/base/transport_security_state.h ./net/base/transport_security_state_static.h:2095: case DomainState::MODE_SPDY_ONLY: ./net/base/transport_security_state_static.h:2202: mode = DomainState::MODE_SPDY_ONLY; ./net/base/transport_security_state_static.h So it's defined, and then handled in new code by agl that does not take this refactoring into account. Nobody else uses it; we'll remove the case from agl's code too. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:115: // This is a docs.google.com override. On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Could you expand this comment? > > It's not clear what it's an override of - an internal preload? Done. http://codereview.chromium.org/9415040/diff/20034/chrome/browser/ui/webui/net... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): http://codereview.chromium.org/9415040/diff/20034/chrome/browser/ui/webui/net... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1016: result->SetString("static_spki_hashes", hashes); On 2012/03/23 22:59:24, eroman wrote: > You need to update the javascript too if you make this change. > See src/chrome/browser/resources/net_internals/hsts_view.js Thank you. And, unfortunately, chrome/test/data/webui/net_internals/hsts_view.js. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... File net/base/transport_security_state.cc (right): http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:48: /* XXX On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Delete? Done. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:91: }*/ On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Delete? Done. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:95: if (GetDomainState(host, true, &existing_state) && > nit: GetDomainState(host, true /*sni_enabled*/, &existing_state) ? > > It's not obvious why calling with |sni_enabled| should be true for EnableHost, > so perhaps you could expand on the comment as well. It doesn't really matter, since statically-defined states will have null created dates. But for that reason, we might as well not search the SNI-only static states. Added a comment to that effect. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:100: // We don't store this value. On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: Drop the "we", expand on why this value isn't appropriate to be stored. Done. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:342: // "pin-" algo "=" base64 [ ";" ... ] On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Update the ABNF for -01. Done. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:400: // "max-age" "=" delta-seconds [ ";" "includeSubDomains" ] > Definitely update this guy What is wrong about this comment, other than being merely one example? http://tools.ietf.org/html/draft-ietf-websec-strict-transport-sec-06#section-6.1 I'll leave it to abarth to update the comment if he updates the parser. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.cc:765: static const uint32 kTagSPIN = 0x4e495053; On 2012/03/28 00:50:32, Ryan Sleevi wrote: > This was related to side pinning - delete? Done. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:30: // with |SetDelegate| to persist the state to disk. > Or with the constructor. > > Which is odd to have both methods. Could you explain more why this is? Turns out only SetDelegate is necessary, so I got rid of the constructor that took a Delegate. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:35: // http://tools.ietf.org/html/draft-ietf-websec-key-pinning-01. On 2012/03/28 00:50:32, Ryan Sleevi wrote: > http://tools.ietf.org/html/ietf-websec-strict-transport-sec to refer to whatever > is the latest version (eg: currently -06 for STS) > > Same for ietf-websec-key-pinning > > Unless you're intentionally trying to indicate a specific draft revision. Done. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:78: bool ParseSTSHeader(const base::Time& now, const std::string& value); > nit: Could you file a TODO to see about moving these out? http://code.google.com/p/chromium/issues/detail?id=122866 http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:105: bool ShouldRedirectHTTPToHTTPS() const; > It seems you're using this for other protocols. Should this be renamed? No, not that I know of. (MODE_SPDY_ONLY is gone.) http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:171: void SetDelegate(Delegate* delegate); On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: Document Done. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:199: DomainState* result); On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: I originally wondered why these methods weren't const when just reading the > header, and then it became clear when reading the implementation that you expire > and remove entries. > > It would be good if you could clarify this in the comments (both here and at > 213). Done. http://codereview.chromium.org/9415040/diff/20034/net/base/transport_security... net/base/transport_security_state.h:256: // If we have a Delegate, call its |StateIsDirty| function. On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: s/function/method > > Broader still: > // If a Delegate is present, notify it that the internal > // state has changed. Done. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:16: #endif > These do not belong in x509_certificate.cc This and the other things removed; some kind of mishap. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:37: #include "crypto/sha2.h" On 2012/03/28 00:50:32, Ryan Sleevi wrote: > ?? Done. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:44: #endif On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Nor this Done. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:500: bool X509Certificate::GetPublicKeyHash(const OSCertHandle& cert, On 2012/03/28 00:50:32, Ryan Sleevi wrote: > My gut is that because this builds directly on top of GetDEREncoded, it may not > belong in X509Certificate at all. > > Do you anticipate further users of this beyond pinning, and could you explain > where/why/how? Turns out we only need this for a unit test. Moved it there. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.c... net/base/x509_certificate.cc:503: if (!GetDEREncoded(cert, &der_bytes)) On 2012/03/28 00:50:32, Ryan Sleevi wrote: > This requires making a copy of the |cert| data. > > If you do think this is a re-usable method, then you should implement this > within each of the _platform files, which can use the raw data when applicable > (eg: NSS+Win, but not OS X, OpenSSL, or Android) Done. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.h... net/base/x509_certificate.h:55: typedef std::vector<Fingerprint> FingerprintVector; On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Both of these belong in x509_cert_types.h. Done. http://codereview.chromium.org/9415040/diff/20034/net/base/x509_certificate.h... net/base/x509_certificate.h:230: // Parses |cert|'s Subject Public Key Info structure, hashes it, On 2012/03/28 00:50:32, Ryan Sleevi wrote: > nit: > // Compute the SHA1Fingerprint of the Subject Public Key Info > // for |cert|. On success, stores the fingerprint in > // |fingerprint| and returns true. Done. http://codereview.chromium.org/9415040/diff/20034/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_nss.cc (right): http://codereview.chromium.org/9415040/diff/20034/net/socket/ssl_client_socke... net/socket/ssl_client_socket_nss.cc:138: SECItem* encodedResponse, void *pwArg); On 2012/03/28 00:50:32, Ryan Sleevi wrote: > This were typed such to match the underlying NSS style, so they are probably > fine as is. > > If you are going to change them, change all four variables :) Done. http://codereview.chromium.org/9415040/diff/20034/net/socket_stream/socket_st... File net/socket_stream/socket_stream_job.cc (right): http://codereview.chromium.org/9415040/diff/20034/net/socket_stream/socket_st... net/socket_stream/socket_stream_job.cc:32: domain_state.ShouldRedirectHTTPToHTTPS()) { On 2012/03/28 00:50:32, Ryan Sleevi wrote: > Does STS apply to non-HTTPS? > > I didn't think the WG had reached a consensus. I don't know (but I'd vote yes). But, all I'm doing here is re-arranging the arguments to suit the new function prototype. We can revisit this problem in a later CL.
Fun times! Mostly nits, but several questions below. Note that while I request that the ordering of the implementation be made to match that of the header, please defer until the final LGT..M, to make it easier to diff. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... File chrome/browser/transport_security_persister.cc (right): http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:92: namespace { Place unnamed namespaces at the beginning of the file, as per http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces (which is [almost] entirely consistent with existing Chromium code) Specifically between lines 25-27 http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:156: bool TransportSecurityPersister::DeserializeFromCommandLine( nit: Fix the ordering of this file so that definition order matches the declaration order from the header. You should save this until right before committing, to make reviewing diffs easier, but please make sure to address this. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:158: // We purposefully ignore |dirty| because we do not want to persist nit: Drop the we http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:191: !parsed->GetDouble(kExpiry, &expiry)) { So input data failures aren't fatal? Should there be some sort of log? Or do you really want to ignore them? I'd rather have some clear and obvious error if I tried to HSTS something and it failed. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... File chrome/browser/transport_security_persister.h (right): http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:8: // loading it at startup. I can't remember if it's been mentioned before, but is there a risk of incognito state leaking into the main profile due to it being a singleton? If so, perhaps a TODO with the bug here would be nice :) http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:58: // Implements the interface of ImportantFileWriter::DataSerializer. The original form was consistent with http://dev.chromium.org/developers/coding-style#TOC-Code-formatting "When you implement an interface" ... http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:67: // Should you include a version with the serialization form, especially as things are still a bit draft-y? http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:83: // SHA256(net::TransportSecurityState::CanonicalizeHost(domain)). The There are two dictionaries (the first is host -> data, the second is key -> value). So it's not clear here what "keys" you're referring to, especially since you s/host/domain/ (from line 64). Might need some word smithing. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:113: bool forced, nit: Function parameter ordering http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... |forced| should be before |dirty| http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... File chrome/browser/transport_security_persister_unittest.cc (right): http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:27: { } nit: the opening brace should be on the end of line 26, the closing brace should be on 27 http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:30: } nit: Move this up to line 29, in keeping with http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:33: content::TestBrowserThread test_io_thread; I don't see any tests covering serializing from actual file data. Can you add one? Note that you will need to explicitly initialize message_loop prior to line 25 so that it's an IO thread (and can thus read files) http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... File net/base/transport_security_state.cc (right): http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.cc:54: : delegate_(NULL) { } nit: move } to a new line http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.cc:492: } nit: move } to line 491 http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.h:156: end_(state.enabled_hosts_.end()) { } nit: } on a new line, as per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.h:157: ~Iterator() { } nit: { } -> {} http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.h:232: std::map<std::string, DomainState>& GetEnabledHosts() { I would much rather you expose an API that allows callers to mutate it than just exposing the data members directly. This seems 'dangerous' (from maintenance) to have just floating around publicly. http://codereview.chromium.org/9415040/diff/43003/net/url_request/url_request... File net/url_request/url_request_http_job.cc (right): http://codereview.chromium.org/9415040/diff/43003/net/url_request/url_request... net/url_request/url_request_http_job.cc:604: TransportSecurityState::DomainState::MODE_FORCE_HTTPS; I found the original comment was very helpful in explaining why, when searching fails, you force HTTPS. Could you add it (or something equivalent) back. Same with line 638 (new)
http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... File chrome/browser/transport_security_persister.cc (right): http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:92: namespace { On 2012/04/26 19:21:12, Ryan Sleevi wrote: > Place unnamed namespaces at the beginning of the file, as per > http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces > > (which is [almost] entirely consistent with existing Chromium code) > > Specifically between lines 25-27 Done. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:156: bool TransportSecurityPersister::DeserializeFromCommandLine( > nit: Fix the ordering of this file so that definition order matches the > declaration order from the header. > > You should save this until right before committing, to make reviewing diffs > easier, but please make sure to address this. Will do. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:158: // We purposefully ignore |dirty| because we do not want to persist On 2012/04/26 19:21:12, Ryan Sleevi wrote: > nit: Drop the we Done. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.cc:191: !parsed->GetDouble(kExpiry, &expiry)) { On 2012/04/26 19:21:12, Ryan Sleevi wrote: > So input data failures aren't fatal? > > Should there be some sort of log? Or do you really want to ignore them? > > I'd rather have some clear and obvious error if I tried to HSTS something and it > failed. The code as it is here is merely moved, not changed; this policy is the original policy. That said, we do LOG before continue-ing below, so I'll add LOG statements in this parsing loop when continue-ing (i.e. when failing). http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... File chrome/browser/transport_security_persister.h (right): http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:8: // loading it at startup. > I can't remember if it's been mentioned before, but is there a risk of incognito > state leaking into the main profile due to it being a singleton? It's a singleton per-profile. See chrome/browser/profiles/profile_io_data.{h,cc}. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:58: // Implements the interface of ImportantFileWriter::DataSerializer. On 2012/04/26 19:21:12, Ryan Sleevi wrote: > The original form was consistent with > http://dev.chromium.org/developers/coding-style#TOC-Code-formatting > > "When you implement an interface" ... Done. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:67: // > Should you include a version with the serialization form, especially as things > are still a bit draft-y? The appearance of draftiness is largely due to backwards compat; I actually don't anticipate changing this much in the future. If we change it it will be to add things, which JSON allows easily and for which we can fall back gracefully. One of the things we'd need to fall back gracefully for would be the version field... http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:83: // SHA256(net::TransportSecurityState::CanonicalizeHost(domain)). The On 2012/04/26 19:21:12, Ryan Sleevi wrote: > There are two dictionaries (the first is host -> data, the second is key -> > value). So it's not clear here what "keys" you're referring to, especially since > you s/host/domain/ (from line 64). > > Might need some word smithing. Done. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister.h:113: bool forced, On 2012/04/26 19:21:12, Ryan Sleevi wrote: > nit: Function parameter ordering > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > > |forced| should be before |dirty| Done. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... File chrome/browser/transport_security_persister_unittest.cc (right): http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:27: { } On 2012/04/26 19:21:12, Ryan Sleevi wrote: > nit: the opening brace should be on the end of line 26, the closing brace should > be on 27 > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... Done. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:30: } On 2012/04/26 19:21:12, Ryan Sleevi wrote: > nit: Move this up to line 29, in keeping with > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... Done. http://codereview.chromium.org/9415040/diff/43003/chrome/browser/transport_se... chrome/browser/transport_security_persister_unittest.cc:33: content::TestBrowserThread test_io_thread; On 2012/04/26 19:21:12, Ryan Sleevi wrote: > I don't see any tests covering serializing from actual file data. Can you add > one? > > Note that you will need to explicitly initialize message_loop prior to line 25 > so that it's an IO thread (and can thus read files) Done. http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... File net/base/transport_security_state.cc (right): http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.cc:54: : delegate_(NULL) { } On 2012/04/26 19:21:12, Ryan Sleevi wrote: > nit: move } to a new line Done. http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.cc:492: } On 2012/04/26 19:21:12, Ryan Sleevi wrote: > nit: move } to line 491 Done. http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.h:156: end_(state.enabled_hosts_.end()) { } On 2012/04/26 19:21:12, Ryan Sleevi wrote: > nit: } on a new line, as per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... Done. http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.h:157: ~Iterator() { } On 2012/04/26 19:21:12, Ryan Sleevi wrote: > nit: { } -> {} Done. http://codereview.chromium.org/9415040/diff/43003/net/base/transport_security... net/base/transport_security_state.h:232: std::map<std::string, DomainState>& GetEnabledHosts() { On 2012/04/26 19:21:12, Ryan Sleevi wrote: > I would much rather you expose an API that allows callers to mutate it than just > exposing the data members directly. This seems 'dangerous' (from maintenance) to > have just floating around publicly. Done. http://codereview.chromium.org/9415040/diff/43003/net/url_request/url_request... File net/url_request/url_request_http_job.cc (right): http://codereview.chromium.org/9415040/diff/43003/net/url_request/url_request... net/url_request/url_request_http_job.cc:604: TransportSecurityState::DomainState::MODE_FORCE_HTTPS; On 2012/04/26 19:21:12, Ryan Sleevi wrote: > I found the original comment was very helpful in explaining why, when searching > fails, you force HTTPS. Could you add it (or something equivalent) back. > > Same with line 638 (new) Done.
Last two nits, but I'll give you the LGTM you've been waiting for :) Feel free to commit if/once addressed (also: don't forget the ordering fixes) http://codereview.chromium.org/9415040/diff/53001/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/53001/net/base/transport_security... net/base/transport_security_state.h:234: void EnabledHostsInsert(std::string hashed_host, const DomainState& state); nit: AddOrUpdateEnabledHost ? http://codereview.chromium.org/9415040/diff/53001/net/base/transport_security... net/base/transport_security_state.h:240: void ForcedHostsInsert(std::string hashed_host, const DomainState& state); nit: AddOrUpdateForcedHost?
> Last two nits, but I'll give you the LGTM you've been waiting for :) Thanks. :) > Feel free to commit if/once addressed (also: don't forget the ordering fixes) Did the nits and the ordering fixes.
http://codereview.chromium.org/9415040/diff/53001/net/base/transport_security... File net/base/transport_security_state.h (right): http://codereview.chromium.org/9415040/diff/53001/net/base/transport_security... net/base/transport_security_state.h:234: void EnabledHostsInsert(std::string hashed_host, const DomainState& state); On 2012/04/28 00:10:59, Ryan Sleevi wrote: > nit: AddOrUpdateEnabledHost ? Done. http://codereview.chromium.org/9415040/diff/53001/net/base/transport_security... net/base/transport_security_state.h:240: void ForcedHostsInsert(std::string hashed_host, const DomainState& state); On 2012/04/28 00:10:59, Ryan Sleevi wrote: > nit: AddOrUpdateForcedHost? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/palmer@chromium.org/9415040/65002
Try job failure for 9415040-65002 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/palmer@chromium.org/9415040/67010
Change committed as 134754 |
