Eventually OCSP-must-staple will exist, and as I understand it, it will look a whole lot like Strict-Transport-Security. So, nsStrictTransportSecurityService should be refactored into a more general site security info service. My plan is to put the handling of private mode / not private mode, parent domains that set includeSubdomains, expiration, and preload lists into a utility class that nsStrictTransportSecurityService and the eventual nsOCSPMustStapleService (or whatever) can make use of. I wrote this on top of my patch for bug 846825. It isn't entirely necessary, but we will want to complete that before OCSP-must-staple (because the header parsing is still specific to what type of security info we're talking about, and the refactoring in that bug helps with this).

Comment on attachment 767494 [details] [diff] [review] patch Review of attachment 767494 [details] [diff] [review]: ----------------------------------------------------------------- David, I wished that I could use something like this for the Pinning information, however the call to get the pinning data happens not in the main thread. What do you think we could do to make the getsecurityinfo call fast off the main thread? (I think the latency would be of around 100 ms)

To really make that work, we would probably need a thread-safe data store for this security information we're collecting. I think there's a plan to stop using the permission manager anyway, right?

Comment on attachment 767494 [details] [diff] [review] patch We talked about how we're going to do this a little differently, so clearing feedback for now.

This renames nsIStrictTransportSecurityService to nsISiteSecurityService in preparation of refactoring it to handle multiple types of security header.

This patch refactors nsISiteSecurityService to be able to handle multiple types of security headers. Currently, only HSTS is supported.

Comment on attachment 769747 [details] [diff] [review] patch 1/2 Review to Camilo instead.

Comment on attachment 769747 [details] [diff] [review] patch 1/2 Review of attachment 769747 [details] [diff] [review]: ----------------------------------------------------------------- Looks good (did not comlie it)

Comment on attachment 769755 [details] [diff] [review] patch 2/2 Review of attachment 769755 [details] [diff] [review]: ----------------------------------------------------------------- The interface you have might not be the right one for HKPK but lets fix that later

Comment on attachment 769747 [details] [diff] [review] patch 1/2 Hi, Patrick - I was wondering if you could do a sr on these patches. The purpose of this bug is to refactor nsIStrictTransportSecurityService into a more generalized nsISiteSecurityService that can handle multiple types of security property (where we currently support HSTS and will eventually support key pinning and OCSP-must-staple). This patch (1 of 2) consists of a number of renames like nsIStrictTransportSecurityService to nsISiteSecurityService.

Comment on attachment 769755 [details] [diff] [review] patch 2/2 This patch (2 of 2) generalizes the functions in nsISiteSecurityService (previously nsIStrictTransportSecurityService) to take a parameter that specifies which type of security property to operate on. Currently only HSTS is supported.

Comment on attachment 769755 [details] [diff] [review] patch 2/2 Review of attachment 769755 [details] [diff] [review]: ----------------------------------------------------------------- I'll give you an r+ for the netwerk/* bits but I'm not a sr'r, so I can't do that. ::: netwerk/base/public/nsISiteSecurityService.idl @@ +55,5 @@ > /** > * Checks if the given security info is for an STS host with a broken > * transport layer (certificate errors like invalid CN). > */ > + boolean shouldIgnoreHeader(in nsISupports aSecurityInfo); why does this one not need a type param?

Comment on attachment 769747 [details] [diff] [review] patch 1/2 Review of attachment 769747 [details] [diff] [review]: ----------------------------------------------------------------- I'll give you an r+ for the netwerk/* bits but I'm not a sr'r, so I can't do that.

Thank you for the quick review. My understanding was that the meaning of a super-review changed to be that interface changes require sr+ from the module owner (hence, every module owner is the super-reviewer for that module). (In reply to Patrick McManus [:mcmanus] [afk until Aug 09] from comment #11) > Comment on attachment 769755 [details] [diff] [review] > ::: netwerk/base/public/nsISiteSecurityService.idl > @@ +55,5 @@ > > /** > > * Checks if the given security info is for an STS host with a broken > > * transport layer (certificate errors like invalid CN). > > */ > > + boolean shouldIgnoreHeader(in nsISupports aSecurityInfo); > > why does this one not need a type param? Basically, this is a utility function that, given the security information of a connection, says whether or not any information from it is trustworthy. I'm actually not sure this belongs in nsISiteSecurityService, but that can be a follow-up bug.

Comment on attachment 769747 [details] [diff] [review] patch 1/2 bz - we spoke on IRC about reviewing these patches (this is 1 of 2). The comments asking mcmanus for sr should explain the purpose of these changes, but let me know if you need more context. Thanks!

Comment on attachment 769755 [details] [diff] [review] patch 2/2 (2 of 2)

Er, wait. I thought you wanted sr on these, not detailed code review. If you want detailed review, I'm probably the wrong choice; it would be better to get module owners/peers for the affected modules instead...

Sorry - my intention was to ask for a code review. I can get code reviews from owners/peers of the relevant modules instead. I thought mcmanus would be a good choice for sr, since he's the necko module owner, but there seems to be some confusion on who is able to give sr+ for these changes.

Comment on attachment 769747 [details] [diff] [review] patch 1/2 David, please ask the people who own the relevant code, at least if review is desired before about two weeks from now. :(

Comment on attachment 769747 [details] [diff] [review] patch 1/2 bz - sorry about the confusion. If you have time for a sr of the idl changes, that would be great. jst - I can't seem to raise you on irc, so maybe you're reading bugmail. If you could review the changes in content and the docshell, I would appreciate it. Previous comments should explain the rationale for these changes, but let me know if you would like more context. Thanks!

Comment on attachment 769747 [details] [diff] [review] patch 1/2 - In netwerk/base/public/nsISiteSecurityService.idl: -[scriptable, uuid(c6138514-f212-4747-98c2-7abfce3be293)] -interface nsIStrictTransportSecurityService : nsISupports +[scriptable, uuid(1ca9de3d-26b8-4e0c-9641-62c380bdd9c7)] +interface nsISiteSecurityService : nsISupports Since there's no binary incompatible changes here (just a rename), I don't see a reason to have to update the uuid here. Given how mechanical this change is, and that it's really just a rename, I'm going to r+sr this whole patch.

Comment on attachment 769755 [details] [diff] [review] patch 2/2 netwerk/base/public/nsISiteSecurityService.idl: + boolean shouldIgnoreHeader(in nsISupports aSecurityInfo); Given that this method really answers the question whether all security headers should be ignored, I think we should make header in the name be plural, i.e. shouldIgnoreHeaders(). r+sr=jst on this patch too, given that necko and security folks have already looked at this as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/97fcb5a154d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8a88c6d0c8

Had to deal with merge conflicts. Here's patch 1/2 as checked in. Carrying over all the reviews.

This will need some documentation. The main points are: 1. nsIStrictTransportSecurityService is now nsISiteSecurityService 2. The functionality is (currently) the same, but nsISiteSecurityService::HEADER_HSTS must be specified as an additional parameter to each function in the interface (in the future, different headers will be supported)

Backed out for OSX mochitest-other orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/8102ebf36ce9 https://tbpl.mozilla.org/php/getParsedLog.php?id=26173750&tree=Mozilla-Inbound

Somehow I pushed an old copy of the patches. This should work: https://hg.mozilla.org/integration/mozilla-inbound/rev/b44c631a881a https://hg.mozilla.org/integration/mozilla-inbound/rev/25593587283a

https://hg.mozilla.org/mozilla-central/rev/b44c631a881a https://hg.mozilla.org/mozilla-central/rev/25593587283a

Ok - now that this landed on central, here's the actual patches.

In your commit http://hg.mozilla.org/mozilla-central/rev/25593587283a you probably landed broken test code. http://hg.mozilla.org/mozilla-central/rev/25593587283a#l10.14 10.14 + gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HSTA, uri, I think that should have been HEADER_HSTS, because I cannot find symbol HEADER_HSTA anywhere else.

If you agree to the fix, please feel free to go ahead and check it in.

Comment on attachment 796285 [details] [diff] [review] fix HSTA Review of attachment 796285 [details] [diff] [review]: ----------------------------------------------------------------- Good catch.