The global PB service is going away. The existing interface just takes URIs; we may need to add specific interfaces to access private vs. public permissions.

Given that the only caller is nsHttpChannel, I think we can potentially modify the interface for ProcessStsHeader to accept an nsIPrivateBrowsingConsumer argument and use that to determine the privacy state. Honza, does that sound reasonable?

Rather a new method ProcessStsHeaderWithPrivateState (or so) ? The current one will assume PB=off.

Any particular reason we want two different methods? The single method could accept a null pointer if necessary.

(In reply to Josh Matthews [:jdm] from comment #3) > Any particular reason we want two different methods? The single method could > accept a null pointer if necessary. Just preserve compatibility for extensions. That's all. But making the arg just optional may be the solution here. Yep, let's do it.

(In reply to Honza Bambas (:mayhemer) from comment #4) > (In reply to Josh Matthews [:jdm] from comment #3) > > Any particular reason we want two different methods? The single method could > > accept a null pointer if necessary. > > Just preserve compatibility for extensions. That's all. But making the arg > just optional may be the solution here. Yep, let's do it. Note that it would only help with js add-ons, binary add-ons would still need to be recompiled.

Options: 1. preserve binary compatibility: new interface 2. preserve JS compatibility: new method or default-valued arg 3. no preservation at all: just modify the method's signature I don't know what the policy is now. I see patches doing (2) quit often these days.

Honestly in this case I'd really prefer if we break both binary and js compatibility, so that extensions using this interface start to be PB aware. I don't think there are too many extensions relying on this, which is heavily biasing my judgement towards breaking backwards compat.

Hmm, I wonder if more users of the service have appeared in the paste 1.5 months? It looks like we'll need to add arguments to a bunch of interface methods: * RemoveStsState * IsStsURI * IsStsHost * ProcessStsHeader We should probably make the extra argument nsILoadContext so we can easily update consumers like http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_websocket.html?force=1#1226.

Brian, is there some way to get to a logical docshell or channel in the cert verification code at http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#307?

According to the AMO mxr, Sid's Force-TLS addon is the only one that uses this interface. All in-tree users appear here: http://mxr.mozilla.org/mozilla-central/search?string=nsistricttransport

(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #9) > Brian, is there some way to get to a logical docshell or channel in the cert > verification code at > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ > SSLServerCertVerification.cpp#307? There should not be any uses of the docshell in the SSL or cert verification code. I am working on removing the last usage of docshell in that code now. Instead, you need to pass the information for strict transport security when you create the transport layer. Camilo has a very similar issue and he already is modifying the code to pass additional information at the time the socket transport is created. You and him should be able to use basically the exact same technique. We need the private browsing information earlier anyway, so that we can fix bugs about writing SSL-related data to disk in private browsing mode.

(In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment #10) > According to the AMO mxr, Sid's Force-TLS addon is the only one that uses > this interface. All in-tree users appear here: > http://mxr.mozilla.org/mozilla-central/search?string=nsistricttransport I'm sure Sid will happily change his add-on if we ask him nicely. :-)

(In reply to Honza Bambas (:mayhemer) from comment #6) > Options: > 1. preserve binary compatibility: new interface > 2. preserve JS compatibility: new method or default-valued arg > 3. no preservation at all: just modify the method's signature > > I don't know what the policy is now. I see patches doing (2) quit often > these days. In this case, I think it is OK to go with #3 and ignore addon compatibility. I am not sure it is OK to do #1 or #2. We want all addons to respect the private browsing mode settings for each tab, right? If so, then any time that cannot happen automatically, we must change the signatures of the methods/interfaces used by addons so that the addons will break until they implement the correct private browsing mode behavior.

I'd rather us break add-ons here.

Yeah, break compatibility with the add-on that uses it. I'll fix the add-on.

(In reply to Brian Smith (:bsmith) from comment #11) > (In reply to Josh Matthews [:jdm] (travelling until June 25th) from comment > #9) > > Brian, is there some way to get to a logical docshell or channel in the cert > > verification code at > > http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/ > > SSLServerCertVerification.cpp#307? > > There should not be any uses of the docshell in the SSL or cert verification > code. I am working on removing the last usage of docshell in that code now. > > Instead, you need to pass the information for strict transport security when > you create the transport layer. Camilo has a very similar issue and he > already is modifying the code to pass additional information at the time the > socket transport is created. You and him should be able to use basically the > exact same technique. > > We need the private browsing information earlier anyway, so that we can fix > bugs about writing SSL-related data to disk in private browsing mode. Brian, I haven't been in contact with Camilo about his changes yet, but I'm worried by this response. Isn't the socket transport creation a one-time event? We need to be able to determine the privacy status of the window that initiated the connection that had a cert error, an action which is necessarily dynamic.

(In reply to Josh Matthews [:jdm] from comment #16) > Brian, I haven't been in contact with Camilo about his changes yet, but I'm > worried by this response. Isn't the socket transport creation a one-time > event? We need to be able to determine the privacy status of the window that > initiated the connection that had a cert error, an action which is > necessarily dynamic. I think we just have a little miscommunication. We need to know whether the connection should be a "private" one at the time where we create and/or choose the connection to send the request on. If we need a "private" connection and there are no suitable private connections, we must create a new one; when we do so, we should pass all the information that private browsing and strict transport security needs at that time. If there's an existing "private" connection then we can just put the transaction on that connection without any problems. In particular, I am expecting that we will pass the private browsing information (just "bool isPrivate"?) in new parameters of nsSSLIOLayerNewSocket and nsSSLIOLayerAddToSocket. In fact, perhaps we should just change the nsISocketProvider API to add the "isPrivate" flag as a parameter newSocket and addToSocket, so that all providers and all users of the providers are required to explicitly understand private browsing mode.

Josh, please let us know if you want Chris to take this!

I'm pretty sure I can tackle this in the next couple days.

Try run for 5e54241800b4 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=5e54241800b4 Results (out of 132 total builds): success: 77 warnings: 54 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-5e54241800b4

Try run for 5a3f87205068 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=5a3f87205068 Results (out of 133 total builds): success: 99 warnings: 33 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-5a3f87205068

See the change to nsISocketProvider.idl. When you create sockets, you need to specify whether or not the socket should be considered to be in "private browsing." I guess mailnews doesn't have any concept of "private browsing" so you can just pass "false," if you are using these APIs directly.

Comment on attachment 663548 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. >- connector.speculativeConnect(searchURI, null, null); >+ let callbacks = window.getInterface(Ci.nsIWebNavigation) >+ .QueryInterface(Ci.nsIDocShell) >+ .currentDocumentChannel.notificationCallbacks; >+ connector.speculativeConnect(searchURI, callbacks, null); Is the window's own nsIInterfaceRequestor implementation incapable of handling the request? (Nit: you forgot to QueryInterface to nsIInterfaceRequestor but you were probably saved by some other browser code doing that for you.)

I'm not sure what you're asking. People like bz and gavin have always told me that this is the series of steps requred to obtain a docshell.

(In reply to Josh Matthews from comment #26) > I'm not sure what you're asking. People like bz and gavin have always told > me that this is the series of steps requred to obtain a docshell. Sorry, I misread the code, I overlooked that you wanted the channel's callbacks. (The window.QueryInterface(Ci.nsIInterfaceRequestor) is still technically missing though.)

Comment on attachment 676282 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. Boris, can you approve the interface changes?

Note to self: rev the UUIDs.

Comment on attachment 676282 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. Once bug 804495 lands you should be able to GetInterface an nsILoadContext directly from a window. Interface changes look fine.

Comment on attachment 676282 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. Review of attachment 676282 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/search/content/search.xml @@ +500,5 @@ > var searchURI = engine.getSubmission("dummy").uri; > + let callbacks = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor) > + .getInterface(Components.interfaces.nsIWebNavigation) > + .QueryInterface(Components.interfaces.nsIDocShell) > + .currentDocumentChannel.notificationCallbacks; This part I am not sure about, because I don't know if this is the right notificationCallbacks to be retrieving (or even what this will retrieve). Please ask somebody who knows such things to review this chunk. ::: docshell/base/nsDocShell.cpp @@ +4171,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > > bool isStsHost = false; > + uint32_t flags = > + mInPrivateBrowsing ? nsIStrictTransportSecurityService::IS_PRIVATE : 0; uint32_t flags = tsi->GetTransportFlags(); (See comments below; this assumes we add a [notxpcom] GetTransportFlags() method to nsITransportSecurityInfo.) ::: netwerk/base/src/nsSocketTransport2.cpp @@ +976,5 @@ > > if (mConnectionFlags & nsISocketTransport::ANONYMOUS_CONNECT) > proxyFlags |= nsISocketProvider::ANONYMOUS_CONNECT; > > + bool isPrivate = !!(mConnectionFlags & nsISocketTransport::IS_PRIVATE); proxyFlags |= (mConnectionlags & (nsISocketTransport::IS_PRIVATE); @@ +984,5 @@ > // if this is the first type, we'll want the > // service to allocate a new socket > rv = provider->NewSocket(mNetAddr.raw.family, > host, port, proxyHost, proxyPort, > + proxyFlags, isPrivate, &fd, isPrivate should just be a flag incorporated into proxyFlags. We must have had some misunderstanding about what boolean argument I wanted you to convert to a flag. I wasn't asking for you to change nsIStrictTransportSecurityService's boolean isPrivate argument to a flag; I was asking for *this* isPrivate argument to be incorporated into the flags argument (proxyFlags in this case) so that we don't have to change the nsISocket* interfaces at all (besides adding the new flag constant). ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1114,5 @@ > // If this wasn't an STS host, errors are allowed, but no more STS processing > // will happen during the session. > bool wasAlreadySTSHost; > + uint32_t flags = > + NS_UsePrivateBrowsing(this) ? nsIStrictTransportSecurityService::IS_PRIVATE : 0; It is good to pass flags to nsIStrictTransportSecurityService. But, why not just have nsIStrictTransportSecurityService use nsISocketTransport's flags, instead of having its own? Otherwise, we have to translate between the two, right? ::: security/manager/boot/src/nsStrictTransportSecurityService.cpp @@ +227,5 @@ > if (aIncludeSubdomains != nullptr) { > *aIncludeSubdomains = false; > } > > + bool isPrivate = aFlags & nsIStrictTransportSecurityService::IS_PRIVATE; It seems like you should just have ProcessStsHeaderMutating take the flags itself, instead of converting to boolean here. Notice in ProcessStsHeaderMutating -> SetStsStat you end up converting the boolean back into flags anyway. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +292,5 @@ > bool strictTransportSecurityEnabled = false; > nsCOMPtr<nsIStrictTransportSecurityService> stss > = do_GetService(NS_STSSERVICE_CONTRACTID, &nsrv); > if (NS_SUCCEEDED(nsrv)) { > + uint32_t flags = mInfoObject->IsPrivate() ? nsIStrictTransportSecurityService::IS_PRIVATE : 0; We should just store the transport flags in mInfoObject, and then have a GetTransportFlags() that we can then pass the result of to IsStsHost. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +1312,5 @@ > PRFileDesc **fd, > nsISupports** info, > bool forSTARTTLS, > + bool anonymousLoad, > + bool isPrivate) Again, instead of converting the transport flags into booleans, we should just pass them as a single flags parameter, and then extract out the anonymousLoad and/or isPrivate flags when necessary; e.g. bool IsPrivate() { return mLoadFags & nsISocketTransport::IS_PRIVATE; } @@ +2436,5 @@ > PRFileDesc* layer = nullptr; > nsresult rv; > PRStatus stat; > > nsNSSSocketInfo* infoObject = new nsNSSSocketInfo(); new nsNSSSocketInfo(flags);

I believe this addresses all of your points.

Comment on attachment 679486 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. Gavin, can you sign off on the browser/ change?

And yes, I need to change one stale reference to nsIStrictTransportService.IS_PRIVATE in test_websocket.html. I've done that locally.

Comment on attachment 679486 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. Review of attachment 679486 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with just one notable concern. Please ask biesi about moving the nsISocketProvider flags to nsISocketTransport. ::: netwerk/base/public/nsISocketTransport.idl @@ +151,5 @@ > const unsigned long DISABLE_IPV6 = (1 << 2); > > /** > + * If set, indicates that the connection was initiated from a source > + * defined as being private. We should expand on this to help future Necko peers understand what "private" means. Something like s/private./private in the sense of Private Browsing. Generally, this there should be no state shared between connections that are private and connections that are not private; it is OK for multiple private connections to share state with each other, and it is OK for multiple non-private connections to share state with each other." ::: netwerk/base/src/nsSocketTransport2.cpp @@ +977,5 @@ > if (mConnectionFlags & nsISocketTransport::ANONYMOUS_CONNECT) > proxyFlags |= nsISocketProvider::ANONYMOUS_CONNECT; > > + if (mConnectionFlags & nsISocketTransport::IS_PRIVATE) > + proxyFlags |= nsISocketTransport::IS_PRIVATE; Oops. I overlooked this in my previous comment. nsISocketTransport flags are slightly different than nsISocketProvider flags. In order to avoid confusion and duplicates, I think we should first move nsISocketProvider.PROXY_RESOLVES_HOST to nsISocketTransport, with a different value (since 1 << 0 is already used for BYPASS_CACHE), and then remove nsISocketProvider.ANONYMOUS_CONNECT (using nsISocketTransport.ANONYMOUS_CONNECT, which happens to have the same value). Then, for flags that only apply to transports or providers, we can simply document that fact in the IDL, while flags that apply to both (ANONYMOUS_CONNECT and IS_PRIVATE) are guaranteed to not collide when used interchangeably. ::: security/manager/ssl/src/TransportSecurityInfo.h @@ +89,5 @@ > mutable ::mozilla::Mutex mMutex; > > protected: > nsCOMPtr<nsIInterfaceRequestor> mCallbacks; > + uint32_t mTransportFlags; const uint32_t mTransportFlags; ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +76,5 @@ > mFirstServerHelloReceived(false), > mNPNCompleted(false), > mHandshakeCompleted(false), > mJoined(false), > mSentClientCert(false) mTransportFlags(transportFlags)

Christian, what do you think of bsmith's suggestion of moving flags around in comment 38?

>::: security/manager/ssl/src/TransportSecurityInfo.h >@@ +89,5 @@ >> mutable ::mozilla::Mutex mMutex; >> >> protected: >> nsCOMPtr<nsIInterfaceRequestor> mCallbacks; >> + uint32_t mTransportFlags; > >const uint32_t mTransportFlags; > >::: security/manager/ssl/src/nsNSSIOLayer.cpp >@@ +76,5 @@ >> mFirstServerHelloReceived(false), >> mNPNCompleted(false), >> mHandshakeCompleted(false), >> mJoined(false), >> mSentClientCert(false) > >mTransportFlags(transportFlags) I can't make these changes, since inherited members can't be used in initializer lists, and I can't add the flags to the constructor for TransportSecurityInfo since there's a generic factory function for the class.

Comment on attachment 679486 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. Review of attachment 679486 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +370,5 @@ > NS_ENSURE_TRUE(stss, NS_ERROR_OUT_OF_MEMORY); > > bool isStsHost = false; > + uint32_t flags = > + NS_UsePrivateBrowsing(this) ? nsISocketTransport::IS_PRIVATE : 0; Everywhere you are using NS_usePrivateBrowsing in nsHttpChanel, you can just use mPrivateBrowsing, right? It seems better to use mPrivateBrowsing since NS_UsePrivateBrowsing() is non-trivial.

(In reply to Josh Matthews [:jdm] from comment #40) > I can't make these changes, since inherited members can't be used in > initializer lists, and I can't add the flags to the constructor for > TransportSecurityInfo since there's a generic factory function for the class. OK, I forgot about the splitting of TransportSecurityInfo and nsNSSSocketinfo and what it means. We cannot put the flags in TransportSecurityInfo because we need to be able to serialize/deserialize TransportSecurityInfo from the cache, but we can't serialize/deserialize the flags. Either we can put the flags accessor in nsISSLSocketControl, or we can just use your original idea of checking if the current tab/window is a private browsing tab/window. Probably your original idea is better.

Comment on attachment 679486 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. I'm not really clear on what implications passing the chrome window's nsILoadContext to speculativeConnect() may have, beyond the obvious one being added in this patch (its use in nsHttpHandler::SpeculativeConnect). I traced its other use in this code path to NullHttpTransaction::GetSecurityCallbacks, but figuring out what callers of that might use it for isn't trivial. Someone who knows that that's "safe" should sign off on this - I don't know who that would be.

Comment on attachment 679486 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. Patrick, can you comment on the change in the caller of speculativeConnect in search.xml?

(In reply to Brian Smith (:bsmith) from comment #42) > (In reply to Josh Matthews [:jdm] from comment #40) > > I can't make these changes, since inherited members can't be used in > > initializer lists, and I can't add the flags to the constructor for > > TransportSecurityInfo since there's a generic factory function for the class. > > OK, I forgot about the splitting of TransportSecurityInfo and > nsNSSSocketinfo and what it means. We cannot put the flags in > TransportSecurityInfo because we need to be able to serialize/deserialize > TransportSecurityInfo from the cache, but we can't serialize/deserialize the > flags. Either we can put the flags accessor in nsISSLSocketControl, or we > can just use your original idea of checking if the current tab/window is a > private browsing tab/window. Probably your original idea is better. I went with nsISSLSocketControl, otherwise I needed to add another static_cast to nsNSSSocketInfo in SSLServerCertVerification, since we need the flags there.

Formalizing the request from comment 38 and comment 39. The flags issue is the only thing blocking this patch from landing now.

Comment on attachment 681597 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. Christian, if you could approve the API changes, I would appreciate it. In particular, I ended up following the existing convention and having an IS_PRIVATE flag in both nsISocketTransport and nsISocketProvider. I think that is the easier route to take at this point, rather than trying to set up a flag sharing scheme between them.

Comment on attachment 681597 [details] [diff] [review] Add privacy status argument to relevant nsIStrictTransportSecurityService methods. +++ b/netwerk/base/public/nsIStrictTransportSecurityService.idl this file seriously needs to document flags, since it doesn't have any flags in the IDL itself. where do the valid flags come from? I don't think sharing flags between the two interfaces is a good idea, I prefer the approach in this patch. Although I don't like the name IS_PRIVATE... maybe NO_PERSISTENT_STORAGE or something along those lines? +++ b/netwerk/socket/nsISSLSocketControl.idl need new IID

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2c9c988869 https://hg.mozilla.org/integration/mozilla-inbound/rev/326ed6f1dc9e

Followup to fix xpcshell failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/e123e0173185 However unfortunately there are still mochitest-a11y failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=326ed6f1dc9e&jobname=-other eg: https://tbpl.mozilla.org/php/getParsedLog.php?id=17102623&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=17103078&tree=Mozilla-Inbound Backed out three changesets: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f467d800bee

https://hg.mozilla.org/integration/mozilla-inbound/rev/d86aa72c7dbf should do it this time.