Speculative connections should prevent Client Certificate Selection dialog

VERIFIED FIXED in Firefox 57

Status

()

Core
Networking
P1
normal
VERIFIED FIXED
4 years ago
15 days ago

People

(Reporter: mayhemer, Assigned: valentin)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56+ disabled, firefox57+ verified, firefox58 verified)

Details

(Whiteboard: [necko-active][fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 6 obsolete attachments)

18.62 KB, patch
keeler
: review+
valentin
: review+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
mak
: review+
Details | Review
11.53 KB, patch
mak
: review+
Details | Diff | Splinter Review
18.71 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
keeler
: review+
Details | Review
59 bytes, text/x-review-board-request
mak
: review+
Details | Review
(Reporter)

Description

4 years ago
This gets more attention when we are going to preconnect more in the near future with seer.

Speculative connections may pop up a client cert selection dialog at a relatively random time.

There are two scenarios:
- null transaction that is responsible to complete the ssl handshake
- nsHttpConnectionMgr::OnMsgPruneDeadConnections calling to nsHttpConnection::IsAlive() calling to PR_Read(PR_MSG_PEEK)


First thought for a fix: 
- we should simply prevent client cert auth when precoonecting and rather close the connection with NET_RESET when demanded ; we can add a new flag at [1] at the PSM code and convert the ssl handshake error to NET_RESET in the null transaction or somewhere
- as an optimization maybe also remember the site wants a client cert and don't preconnect

More precise way to fix this:
- just leave the ssl connection in the middle of the handshake when the client cert decision is needed from user ; means to return early at [2] and tell the ssl socket the client cert decision will be made later (might involve nss changes..)


[1] http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp?rev=051e287b802f#2572
[2] http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp?rev=051e287b802f#2458

Updated

4 years ago
Blocks: 902439
I don't think we don't need to disable all preconnects with client certs - a fair amount of the time the preconnect will be hooked up to a real transaction by the time the cert phase comes around because DNS and TCP can be pretty laggy.

I think we can probably figure out a way to have a null transaction just declare itself "done" when it runs into this, rather than prompting like it is doing now, and go into the reuse pool.. when it is "resued" the right things should still happen. It would also be OK to not form the null transaction at all (just go right to the reuse pool) if we can tell a client cert would be used - but I'm not clear on how feasible that is. Either way would work ok for necko.

This seems natural for nick to work on. nick, are you game?
(Reporter)

Comment 2

4 years ago
(In reply to Patrick McManus [:mcmanus] from comment #1)
> I don't think we don't need to disable all preconnects with client certs - a
> fair amount of the time the preconnect will be hooked up to a real
> transaction by the time the cert phase comes around because DNS and TCP can
> be pretty laggy.
> 
> I think we can probably figure out a way to have a null transaction just
> declare itself "done" when it runs into this, 
> rather than prompting like it
> is doing now, and go into the reuse pool.. when it is "resued" the right
> things should still happen. 

That is what I propose as "More precise way to fix this".  You need to make the client cert decision callback be asynchronous.  AFAIK, NSS expects sync result now.

> It would also be OK to not form the null
> transaction at all (just go right to the reuse pool) if we can tell a client
> cert would be used - but I'm not clear on how feasible that is. 

It's not, we can just remember the site asked for a client cert and we was not able to do any kind of automatic selection.

> Either way
> would work ok for necko.
> 
> This seems natural for nick to work on. nick, are you game?

I'm not sure what your plan is.  Can you be more detailed?
I'm just asking if nick can add this to his plate, as he's taking on a lot of the responsibility around speculative things.

I think we both presume that NSS will need to either be made async with respect to a client cert - or will need a new method do determine beforehand if a client cert might be sent to this host which we can use to just not form the null transaction. But nick can propose what he thinks is best after investigation.

I agree its not a top priority.
Flags: needinfo?(hurley)
Yeah, I can take a look at this in the near-ish future.
Assignee: nobody → hurley
Flags: needinfo?(hurley)
(In reply to Nicholas Hurley [:hurley] from comment #4)
> Yeah, I can take a look at this in the near-ish future.

Any updates here?  Thumbnails are riding the 27 train, so if this doesn't look like hitting even 28 we might need to look into mitigating this some other way...
My regular 18-month ping :)  I expect it's quite a jarring experience for users who hit this.

Comment 7

2 years ago
In Nightly 42.0a1 (2015-08-06), I’m seeing this dialog as soon as I type ‘f’ into the address bar.  One of my frequently visited sites does indeed require certificate authentication, but obviously I shouldn’t get a modal popup until I actually go there.

It took me a while to find the right about:config flag to disable this; I tried network.prefetch-next=false, network.dns.disablePrefetch=true, and network.predictor.enabled=false without any change.  For anyone else who runs into this, setting network.http.speculative-parallel-limit=0 made it stop.

Comment 8

2 years ago
(In reply to Anders Kaseorg from comment #7)
> It took me a while to find the right about:config flag to disable this; I
> tried network.prefetch-next=false, network.dns.disablePrefetch=true, and
> network.predictor.enabled=false without any change.  For anyone else who
> runs into this, setting network.http.speculative-parallel-limit=0 made it
> stop.

Never mind, that’s no longer helping, even with all four flags (or maybe it never did, and I was somehow getting really lucky in my tests).  Is there really no way to entirely stop Firefox from making speculative connections?
This blocks bug 902439 which has been assigned to me for about 2 years - I guess it is time I gave up hoping, so I'm unassigning myself from that bug.

Updated

2 years ago
See Also: → bug 1240030
Whiteboard: [necko-backlog]
Duplicate of this bug: 1189739
Assignee: hurley → nobody
Duplicate of this bug: 1384335
Blocks: 1348275
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → affected
[Tracking Requested - why for this release]: The location bar uses speculative connect in 56, and this may be quite annoying for users (see bug 1348275 for example).
This is either a P1, or we may need to disable speculative connection in the urlbar, that is part of Photon Performance.
tracking-firefox56: --- → ?
I am changing this to necko-next. I think this will start happening much more often as comment 12 suggest.

Jason, I think we will need an owner here?
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [necko-backlog] → [necko-next]
(Reporter)

Comment 14

4 months ago
(In reply to Honza Bambas (:mayhemer) from comment #0)
> [1]
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> nsNSSIOLayer.cpp?rev=051e287b802f#2572
> [2]
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> nsNSSIOLayer.cpp?rev=051e287b802f#2458

https://hg.mozilla.org/mozilla-central/file/051e287b802f/security/manager/ssl/src/nsNSSIOLayer.cpp#l2458

today:

https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/security/manager/ssl/nsNSSIOLayer.cpp#2284
an this should be fix in 56...

Updated

4 months ago
Priority: -- → P1
Whiteboard: [necko-next] → [necko-next][fx-search]

Updated

4 months ago
Whiteboard: [necko-next][fx-search] → [necko-next][fxsearch][
Track 56+ as this might annoy user's experience.
tracking-firefox56: ? → +
(Reporter)

Comment 17

4 months ago
I'll take a look, maybe we just disable preconnects for those having private certs in the NSS db (=certs+keys eligible for client cert auth)
Assignee: nobody → honzab.moz
Flags: needinfo?(jduell.mcbugs)
Whiteboard: [necko-next][fxsearch][ → [necko-active][fxsearch]
(Reporter)

Comment 18

3 months ago
Created attachment 8900831 [details] [diff] [review]
v1 for consideration

Return a failure from nsNSS_SSLGetClientAuthData doesn't obviously kill the connection.  I've added a mKill flag on nsNSSSocketInfo that is:
- set when a speculative https connection asks for client cert
- converted in PSMRecv and PSMSend to a failure

I don't know how else to SAFELY kill the connection from SSLGetClientAuthData, David if you do, please let me know.  I'm not happy with testing IsKilled() before and after each recv/send on the socket...


Note that is something weird happening with preconnected ssl sockets.  I was not able to locally confirm this patch works when doing a preconnect to a local server requiring a client cert (added few hacks to run this code path, at least).

That is probably a different bug, I'll file it after some more investigation not involving client certs and this patch.
Attachment #8900831 - Flags: feedback?(dkeeler)
How do I make Firefox speculatively connect in this way? I'm running a server that requires a client cert, I have a client cert installed, etc., but I can't get the client cert selection dialog to come up without actually visiting the server with a normal navigation (I'm on linux by the way).
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 20

3 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #19)
> How do I make Firefox speculatively connect in this way? I'm running a
> server that requires a client cert, I have a client cert installed, etc.,
> but I can't get the client cert selection dialog to come up without actually
> visiting the server with a normal navigation (I'm on linux by the way).

I know about the only way: make a page on a different origin (likely file:/// will do too) that has a link to what ever page on your client-cert-requiring server.  Then click-and-hold the link.  That will trigger an ssl preconnect.  On release we deploy the actual request.  Then it depends if the preconnected socket or a new one is used.
Flags: needinfo?(honzab.moz)
Comment on attachment 8900831 [details] [diff] [review]
v1 for consideration

Review of attachment 8900831 [details] [diff] [review]:
-----------------------------------------------------------------

Cool - thanks for the pointer.
Looking at the nss code that actually uses the callback, it appears to support the callback returning SECWouldBlock: https://dxr.mozilla.org/mozilla-central/rev/f0abd25e1f4acced652d180c34b7c9eda638deb1/security/nss/lib/ssl/ssl3con.c#7542
Can we just return that if the connection is speculative and restart the handshake when it's not longer speculative?
Attachment #8900831 - Flags: feedback?(dkeeler) → feedback+
(Reporter)

Comment 22

3 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #21)
> Comment on attachment 8900831 [details] [diff] [review]
> v1 for consideration
> 
> Review of attachment 8900831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool - thanks for the pointer.

Were you able to verify the patch then?

> Looking at the nss code that actually uses the callback, it appears to
> support the callback returning SECWouldBlock:
> https://dxr.mozilla.org/mozilla-central/rev/
> f0abd25e1f4acced652d180c34b7c9eda638deb1/security/nss/lib/ssl/ssl3con.c#7542
> Can we just return that if the connection is speculative and restart the
> handshake when it's not longer speculative?

Not a bad idea!  I can give it a try.  Thanks.
(Reporter)

Comment 23

3 months ago
(In reply to Honza Bambas (:mayhemer) from comment #22)
> Not a bad idea!  I can give it a try.  Thanks.

I only have a concern that after a longer time of no activity on the socket the server or some middle box can close the connection, but I have to verify if we restart or not with a new connection then.  Don't want to spit "net reset" error pages on users.
(Reporter)

Comment 24

3 months ago
This bug is locally a WFM for me.  I've spent this whole evening trying to verify with IIS 10/h2=off and a client cert.  The speculative connection we use doesn't popup a client cert dialog until actually used.  Only change to reproduce for me was to allow speculative connections to localhost/localnet (the server is on localhost.)

Note that when the connection is reset we restart the transaction.  The server from some reason was resetting the h2 stream with HTTP_1_1_REQUIRED error so that we opened a new connection with h2 disabled and restarted the transaction.

David, can you get the dialog from a speculative connection on your server?  Is it public?  I have a wip patch but I can't verify it with my resources.
Flags: needinfo?(dkeeler)
(Reporter)

Comment 25

3 months ago
I think we are not properly driving the tls handshake on speculative connections.  I can see the null transaction being closed with 80470002 (BASE_STREAM_CLOSED) very soon and there is no reading performed on the socket...
(Reporter)

Comment 26

3 months ago
Created attachment 8902406 [details] [diff] [review]
v2 wip
Check also the duplicates, maybe they contain some interesting facts to reproduce. For example https://bugzilla.mozilla.org/show_bug.cgi?id=1384335#c7 and https://bugzilla.mozilla.org/show_bug.cgi?id=1384335#c8 may have some useful points?
The server I'm running is just `openssl s_server -Verify [root CA.pem] -www -cert [server cert.pem] -key [server key.pem]`. I imported that root and a client cert/key issued by that root as well. Good news/bad news is this patch prevents the selection dialog from coming up on a speculative connect, but then it just hangs.
Flags: needinfo?(dkeeler)
(Reporter)

Comment 29

3 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #28)
> The server I'm running is just `openssl s_server -Verify [root CA.pem] -www
> -cert [server cert.pem] -key [server key.pem]`. 

running on localhost?

> I imported that root and a
> client cert/key issued by that root as well. Good news/bad news is this
> patch prevents the selection dialog from coming up on a speculative connect,
> but then it just hangs.

yeah, because the async question for a client cert has never been actually implemented (bug 696976) - there is no API to my knowledge that would retrigger the client cert question.
(Reporter)

Comment 30

3 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #28)
> The server I'm running is just `openssl s_server -Verify [root CA.pem] -www
> -cert [server cert.pem] -key [server key.pem]`. I imported that root and a
> client cert/key issued by that root as well. Good news/bad news is this
> patch prevents the selection dialog from coming up on a speculative connect,
> but then it just hangs.

OK, I'm now running (on localhost) `openssl s_server -Verify ca.pem -CAfile ca.pem -www -cert device.pem -key device.key -accept 8000` and I can repro (client cert pops up on mouse down).  Note I had to hack the code to allow local speculative connections.  IIS on local host requiring ssl and requiring a client cert apparently behaves from some reason differently...
(Reporter)

Comment 31

3 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #28)
> but then it just hangs.

I know why now: we create a specul conn, we say WouldBlock in the client-cert callback.  Then the channel is actually created (on mouse up) we create another connection (because the first speculative is not ready, not yet in the idle pool to be used).  It then tries to connect the server with a newly opened socket, but the server is just single threaded, hence it will hang - the specul conn blocks it, it's in the middle of the handshake and we have no way to restart it.

This is not a good solution.  Only way is to kill the connection from the client cert callback when it's speculative.  

But - why should things be easy, yeah? ;) - problem is, that the second connection is actually also speculative :))  we do an early speculative conn from withing the channel before we async-ask http cache to get an entry.  Hence, it will have the same ending - it will block the client cert dialog or kill itself.

I think we could have a pool of client cert blocked ssl sockets and reset handshake on them when a channel is actually open for the origin.  But this would lead to two client cert dialogs being popped up - tho, it might not be that bad, since by default we have the "remember the decision" check box on, so the second client cert nego would use the first decision, it could work!  David, what do you think?


(In reply to Honza Bambas (:mayhemer) from comment #25)
> I think we are not properly driving the tls handshake on speculative
> connections.  I can see the null transaction being closed with 80470002
> (BASE_STREAM_CLOSED) very soon and there is no reading performed on the
> socket...

Not true.  I don't see read/write on the socket but the handshake is finished.  IIS had some weird issue with not asking a client cert...
Flags: needinfo?(dkeeler)
(Reporter)

Comment 32

3 months ago
Created attachment 8902863 [details] [diff] [review]
v3

- speculative on client cert are blocked (WouldBlock the cert callback)
- socket info for a blocked is kept in an array
- when a non-null (not only specul tls driving) transaction is created, we unblock all sockets for the host/port which should restart the handshake

But apparently calling DriveHandshake on a blocked socket doesn't move us forward but just goes on w/o asking for the client cert again and closes the connection.  also, from a reason I'm too tired to investigate this late the second specul connection still hangs (the reason may be different with this patch)

I'm afraid this won't work w/o bug 696976 properly implemented.
Attachment #8900831 - Attachment is obsolete: true
Attachment #8902406 - Attachment is obsolete: true
Can necko take note of when a speculative connection requests a client cert and just disable speculative connections in general for that host? That might be simplest if we want to fix this in time to ship this feature. Short of that, I don't think we should do anything hacky to get this to work. I'm investigating getting bug 696976 fixed.
Flags: needinfo?(dkeeler)
(Reporter)

Comment 34

3 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #33)
> Can necko take note of when a speculative connection requests a client cert
> and just disable speculative connections in general for that host? 

Yes!  That could work.

Note that when the TCP RTT is slow and we open both speculative connections (the mouse down one and then also the one started by the channel early) we will open the dialog AFTER the navigation or search has been started, so it's not an issue.

Note that I may not find time to finish this this week.  If not, someone else will have to take this bug.
Status: NEW → ASSIGNED
(Reporter)

Comment 35

3 months ago
Created attachment 8903224 [details] [diff] [review]
v4

should be selfexp.  a necko peer has to review this too...  giving to valentin.
Attachment #8902863 - Attachment is obsolete: true
Attachment #8903224 - Flags: review?(valentin.gosu)
Attachment #8903224 - Flags: review?(dkeeler)
Comment on attachment 8903224 [details] [diff] [review]
v4

Review of attachment 8903224 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a reasonable approach for now. We can hopefully make this work in the future. We should see about adding a test for this, though. Also, this doesn't actually compile for me, so I can't test it manually.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +958,5 @@
> +    if (!mExperienced) {
> +        uint32_t flags = 0;
> +        mSocketTransport->GetConnectionFlags(&flags);
> +        if (flags & nsISocketTransport::SPECULATIVE) {
> +            if (gHttpHandler->ConnMgr()->IsSpeculativeConnectDisabled(mConnInfo)) {

IsSpeculativeConnectDisabled isn't implemented (or declared).

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +299,5 @@
> +nsNSSSocketInfo::GetSpeculative(bool *aSpeculative)
> +{
> +  *aSpeculative = mSpeculative;
> +  return NS_OK;
> +}

nit: add a blank line after this one

@@ +2067,5 @@
> +    nsCOMPtr<nsIHttpProtocolHandler> handler(
> +      do_GetService(NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX "http"));
> +
> +    if (handler) {
> +      handler->DontPreconnect(info->GetHostName(), info->GetPort());

Does this do the right thing with respect to proxies and whatnot?

@@ +2070,5 @@
> +    if (handler) {
> +      handler->DontPreconnect(info->GetHostName(), info->GetPort());
> +
> +      // Bail, this socket will not be used anyway.
> +      return SECFailure;

We should PR_SetError(<some error>, 0); here just to be safe. I guess SSL_ERROR_NO_CERTIFICATE would at least be consistent.
Attachment #8903224 - Flags: review?(dkeeler) → review-
(Reporter)

Comment 37

3 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #36)
> We should see about adding a test for this, though.

Absolutely no time to do it, sorry.

> Also, this doesn't actually compile for me, so I can't test it manually.

I'm really getting tired.. will update so that it builds momentarily.  Sorry!

> > +    if (handler) {
> > +      handler->DontPreconnect(info->GetHostName(), info->GetPort());
> 
> Does this do the right thing with respect to proxies and whatnot?

I think yes.  info keeps the server we connect to (the ssl end point), if it's a proxy this keeps the proxy host/port.  But I may double check.
(Reporter)

Comment 38

3 months ago
(In reply to Honza Bambas (:mayhemer) from comment #37)
> (In reply to David Keeler [:keeler] (use needinfo?) from comment #36)
> > > +    if (handler) {
> > > +      handler->DontPreconnect(info->GetHostName(), info->GetPort());
> > 
> > Does this do the right thing with respect to proxies and whatnot?
> 
> I think yes.  info keeps the server we connect to (the ssl end point), if
> it's a proxy this keeps the proxy host/port.  But I may double check.

To be honest, I think you should know this better than me :)
(Reporter)

Comment 39

3 months ago
Created attachment 8903248 [details] [diff] [review]
v4

here it is.
Attachment #8903224 - Attachment is obsolete: true
Attachment #8903224 - Flags: review?(valentin.gosu)
Attachment #8903248 - Flags: review?(valentin.gosu)
Attachment #8903248 - Flags: review?(dkeeler)
Comment on attachment 8903248 [details] [diff] [review]
v4

Review of attachment 8903248 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like it works for the simple case. I'm not sure about the proxy case, though (e.g. authenticating to the proxy vs authenticating to a server through a proxy?)
I'm also looking at how we can test this.

::: security/manager/ssl/nsNSSIOLayer.cpp
@@ +2068,5 @@
> +    nsCOMPtr<nsIHttpProtocolHandler> handler(
> +      do_GetService(NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX "http"));
> +
> +    if (handler) {
> +      handler->DontPreconnect(info->GetHostName(), info->GetPort());

I have to think this one through. I'm not sure what the right answer is.
Duplicate of this bug: 1389618
(Reporter)

Comment 42

3 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #40)
> Comment on attachment 8903248 [details] [diff] [review]
> v4
> 
> Review of attachment 8903248 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like it works for the simple case. I'm not sure about the proxy
> case, though (e.g. authenticating to the proxy vs authenticating to a server
> through a proxy?)

I think what the psm info objects hold in hostname and port is the end node and it's more or less transparent what they are talking to.

More I think of it, I believe the code in nsHttpConnectionMgr::DontPreconnect may be wrong, we should test for both proxy OR origin match and don't preconnect if any of it (origin or proxy) matches the host asking a client cert.

Note that we only support https proxies and we do support ssl through ssl.  Hence, a conn entry that has proxy asking a client cert OR a server (behind that proxy) asking a client cert should both not be preconnected.

> I'm also looking at how we can test this.
> 
> ::: security/manager/ssl/nsNSSIOLayer.cpp
> @@ +2068,5 @@
> > +    nsCOMPtr<nsIHttpProtocolHandler> handler(
> > +      do_GetService(NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX "http"));
> > +
> > +    if (handler) {
> > +      handler->DontPreconnect(info->GetHostName(), info->GetPort());
> 
> I have to think this one through. I'm not sure what the right answer is.


I will try to find time to update the patch according the comment above yet today, but this is the last day I have.  Then I'm ooo till 09-18.
(Reporter)

Comment 43

3 months ago
As this should be done for 57, releasing, so that during the next two weeks someone else can finish this patch.
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Whiteboard: [necko-active][fxsearch] → [fxsearch]
(Reporter)

Comment 44

3 months ago
Created attachment 8903678 [details] [diff] [review]
v4.1

This is v4 updated according comment 42.  When an end point at host:port asks for a client cert during speculative connecting, all entries having that host:port as either origin or proxy (when set) are disabled preconnecting.  I think it's correct this way...

Pick the one :)
Attachment #8903678 - Flags: review?(valentin.gosu)
Attachment #8903678 - Flags: review?(dkeeler)
(Reporter)

Comment 45

3 months ago
(Both patches have a flaw)
(Reporter)

Comment 46

3 months ago
Created attachment 8903695 [details] [diff] [review]
v4.1

missed initiator of mDisallowPreconnects
Attachment #8903248 - Attachment is obsolete: true
Attachment #8903678 - Attachment is obsolete: true
Attachment #8903248 - Flags: review?(valentin.gosu)
Attachment #8903248 - Flags: review?(dkeeler)
Attachment #8903678 - Flags: review?(valentin.gosu)
Attachment #8903678 - Flags: review?(dkeeler)
Attachment #8903695 - Flags: review?(valentin.gosu)
Attachment #8903695 - Flags: review?(dkeeler)
Comment hidden (mozreview-request)
Comment on attachment 8903695 [details] [diff] [review]
v4.1

Review of attachment 8903695 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the PSM bits given the merge conflicts and the test being incorporated.
(On an unrelated note, I thought I had already submitted this review, but now bugzilla's telling me I haven't done it, so... there might be two copies of this comment?)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3240,5 @@
> +        RefPtr<nsConnectionEntry> ent = iter.Data();
> +        nsHttpConnectionInfo* info = ent->mConnInfo;
> +
> +        if ((info->GetOrigin() == host && info->OriginPort() == port) ||
> +            (info->ProxyInfo() && info->GetProxyHost() == host && info->ProxyPort() == port)) {

This seems safe, at least. We might be able to be a bit smarter in the future, but perhaps it's not worth it right now.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +241,5 @@
>      // belonging to the same conn info to free up a connection ASAP.
>      // NOTE: relatively expensive to call, there are two hashtable lookups.
>      bool IsConnEntryUnderPressure(nsHttpConnectionInfo*);
>  
> +    // This disables preconnecting for all existing connection entries matching

This doesn't actually apply cleanly for me on a recent mozilla-central checkout, but the fixup is fairly straightforward.
Attachment #8903695 - Flags: review?(dkeeler) → review+
(Assignee)

Comment 49

3 months ago
Comment on attachment 8903695 [details] [diff] [review]
v4.1

Review of attachment 8903695 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: netwerk/base/nsSocketTransport2.cpp
@@ +1260,5 @@
>                      mSecInfo = secinfo;
>                      callbacks = mCallbacks;
>                      SOCKET_LOG(("  [secinfo=%p callbacks=%p]\n", mSecInfo.get(), mCallbacks.get()));
>                  }
> +

nit: whitespace only change
Attachment #8903695 - Flags: review?(valentin.gosu) → review+
Assignee: nobody → honzab.moz
Whiteboard: [fxsearch] → [necko-active][fxsearch]

Comment 50

2 months ago
mozreview-review
Comment on attachment 8903786 [details]
bug 910207 - test that we don't show the client cert selection dialog on speculative connect

https://reviewboard.mozilla.org/r/175530/#review181228

I don't see a Try run, so please run the full mochitest-browser harness and check the test doesn't cause any new intermittent or unexpected failures before pushing.

Is the patch safe to uplift to 56, since the feature is already enabled there and we need to either uplift this or disable the feature?

::: browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:115
(Diff revision 1)
> +                           clientAuthDialogs);
> +
> +  let cert = await new Promise((resolve, reject) => {
> +    certService.getOrCreateCert("speculative-connect", {
> +      handleCert: function(c, rv) {
> +        if (rv) {

nit: It may be more "correct" to use:
if (!Components.isSuccessCode(rv))

::: browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:127
(Diff revision 1)
> +  });
> +  let server = startServer(cert);
> +  uri = `https://${host}:${server.port}/`;
> +  info(`running tls server at ${uri}`);
> +  await PlacesTestUtils.addVisits([{
> +    uri: uri,

nit: just "uri," (ES6 shorthand)
Attachment #8903786 - Flags: review?(mak77) → review+

Updated

2 months ago
Status: NEW → ASSIGNED
Hi Jason,
Honza seems to be in PTO until 9/18 and de-assign himself on comment 43, will this still be on his plate?
We also need to know :mak's question in comment 56 - is this patch safe to uplift to 56?
Flags: needinfo?(jduell.mcbugs)
Comment hidden (mozreview-request)
Thanks for the review! Here's try after I fixed an issue on windows:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e40b266a07b76e620a0f1817704fe522c132430
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1889fcb5fcd69d78044e2736d36521d84f9a3745
Valentin: can you take this and get it landed?  Please figure out if it's upliftable to 56 too.
Assignee: honzab.moz → valentin.gosu
Flags: needinfo?(jduell.mcbugs) → needinfo?(valentin.gosu)
(Assignee)

Comment 55

2 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c00078ce6abb450423c3b3424d22530cf138511
Bug 910207 - Prevent client certificate pop-up coming from a speculative connection, r=dkeeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/21ffcb521cd5fa6da6c7167eb371fec3787a4a14
Bug 910207 - Test that we don't show the client cert selection dialog on speculative connect r=mak
(Assignee)

Comment 56

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6f10d421ca15b520ab67bffa1c5b6bfb739e74d
Backed out for eslint failure at browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:48: Expected method shorthand:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc2c04a09fa95ed41f5e4a8a4aa3327452add24
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd1f98d46616f476960a55f40a2aa13d125272c4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=21ffcb521cd5fa6da6c7167eb371fec3787a4a14&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129638011&repo=mozilla-inbound

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:48:5 | Expected method shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:57:5 | Expected method shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:62:9 | Expected method shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:62:38 | 'input' is already declared in the upper scope. (no-shadow)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:78:5 | Expected method shorthand. (object-shorthand)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:116:7 | Expected method shorthand. (object-shorthand)
Comment hidden (mozreview-request)
I've addressed the eslint failures - should be good to go now.
(Assignee)

Comment 60

2 months ago
mozreview-review
Comment on attachment 8903786 [details]
bug 910207 - test that we don't show the client cert selection dialog on speculative connect

https://reviewboard.mozilla.org/r/175530/#review182926

::: npm-shrinkwrap.json
(Diff revision 3)
>          "htmlparser2": "3.9.2"
>        }
>      },
>      "eslint-plugin-mozilla": {
> -      "version": "file:tools/lint/eslint/eslint-plugin-mozilla",
> +      "version": "file:tools/lint/eslint/eslint-plugin-mozilla"
> -      "requires": {

Were these changes intentional?
Oh, shoot - no. Good catch. I'll update the patch.
Comment hidden (mozreview-request)
(Assignee)

Comment 63

2 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a51cf9c048a13a84b4027a43cb0532ab38755f9d
Bug 910207 - Prevent client certificate pop-up coming from a speculative connection, r=dkeeler

https://hg.mozilla.org/integration/mozilla-inbound/rev/be66eb2bb0d83a1899e3dd99298a6e994a7d3461
Bug 910207 - Test that we don't show the client cert selection dialog on speculative connect r=mak
(Assignee)

Comment 64

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aca96509e8a3db9b9159d9f2b1adba2d2a7c714
https://hg.mozilla.org/mozilla-central/rev/a51cf9c048a1
https://hg.mozilla.org/mozilla-central/rev/be66eb2bb0d8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 66

2 months ago
(In reply to Valentin Gosu [:valentin] from comment #64)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aca96509e8a3db9b9159d9f2b1adba2d2a7c714

I've applied the patches to beta, but the test fails for some reason. I suspect it's because of the test.
Flags: needinfo?(valentin.gosu) → needinfo?(dkeeler)

Comment 67

2 months ago
Ola, just for reference, I reported the #1389618 duplicate, just updated nightly to 2017-09-10 and didn't see any certificate popups when writing on the address bar (last week they were still showing up). So at least for me the tests are successful.

I can download and run these tests on Beta as well, if that helps.

Thanks for fixing this up!
status-firefox54: affected → wontfix
status-firefox55: affected → wontfix
status-firefox-esr52: affected → wontfix
Flags: in-testsuite+
(In reply to Valentin Gosu [:valentin] from comment #66)
> (In reply to Valentin Gosu [:valentin] from comment #64)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=4aca96509e8a3db9b9159d9f2b1adba2d2a7c714
> 
> I've applied the patches to beta, but the test fails for some reason. I
> suspect it's because of the test.

It's probably because I based the test on mousedown / bug 1355451, which isn't in 56. I'll have a look at updating the test.
Flags: needinfo?(dkeeler)
(Assignee)

Comment 69

2 months ago
It may depend on some helpers that weren't in beta.

It fails for me here:

16 INFO TEST-PASS | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | The second item is selected - 
17 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js | Uncaught exception - waiting for handshake to complete - timed out after 50 tries.

There's also an error at the end of the test when shutting down the server. Not a big deal, but worth fixing if possible:

GECKO(32103) | JavaScript error: chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js, line 79: ReferenceError: info is not defined
Created attachment 8906744 [details] [diff] [review]
910207-speculative-connect-test-for-beta.diff

For beta, I needed to base this test off of a different test (since the original test was based on a test not in 56).
Attachment #8906744 - Flags: review?(mak77)
(Assignee)

Comment 71

2 months ago
Created attachment 8906760 [details] [diff] [review]
[beta uplift] Prevent client certificate pop-up coming from a speculative connection,

MozReview-Commit-ID: IHKzHwsJUiQ

Updated

2 months ago
Attachment #8906744 - Flags: review?(mak77) → review+

Updated

2 months ago
Depends on: 1399300
Please request Beta approval on this ASAP.
Flags: needinfo?(valentin.gosu)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #72)
> Please request Beta approval on this ASAP.

It looks like this caused bug 1399300 so maybe it should wait for that to be investigated?
(Assignee)

Comment 74

2 months ago
Backed out in bug 1399300 comment 2. Not yet sure exactly why this fails.
Flags: needinfo?(valentin.gosu)
(Assignee)

Updated

2 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox57: fixed → affected
Target Milestone: mozilla57 → ---
(In reply to Marco Bonardo [::mak] from comment #50)
> 
> Is the patch safe to uplift to 56, since the feature is already enabled
> there and we need to either uplift this or disable the feature?

I guess we have to backout bug 1348275 for Fx56 because it seems that we might not be able to uplift to 56 in time. :(
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
(In reply to Evelyn Hung [:evelyn] from comment #75)
> I guess we have to backout bug 1348275 for Fx56 because it seems that we
> might not be able to uplift to 56 in time. :(

We should disable all the speculative connections prefs in case, not backout bugs. Could you please file such a bug and ask for tracking?
Also, what about other consumers of speculative connections? Like bug 902439, seems to point out at about:newtab.

Updated

2 months ago
Flags: needinfo?(ehung)

Updated

2 months ago
Depends on: 1399599
Filed bug 1399599 to flip the pref
Flags: needinfo?(ehung)
Bug 1399599 disabled speculative connections from the address bar in 56. We should still track this for 57.
status-firefox56: affected → disabled
tracking-firefox57: --- → ?
I was thinking backout because we still can keep bug 1355443 in 56, pref-off disables both of them. 
Thanks for filing the bug and patching for me. :)
(In reply to Evelyn Hung [:evelyn] from comment #81)
> I was thinking backout because we still can keep bug 1355443 in 56, pref-off
> disables both of them. 

True, but I suspect there may be edge cases (captive portals or such?) where even that may end up with this bug. I may be wrong, in any case it's safer to just delay the whole feature at this stage, imo.
Here's an idea: the vast majority of users don't have client certificates, so even if a site requested one, the UI would never come up. So, what we could do is enable speculative connections for everyone who doesn't have a client certificate.
(Assignee)

Comment 84

2 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #83)
> Here's an idea: the vast majority of users don't have client certificates,
> so even if a site requested one, the UI would never come up. So, what we
> could do is enable speculative connections for everyone who doesn't have a
> client certificate.

That's a good idea. Could you point me to the API I can use to determine if there are any client certificates installed?
Also, is there an observer notification when one gets installed?
It would probably be best to use CERT_FindUserCertsByUsage like the client certificate selection code does:

https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/security/manager/ssl/nsNSSIOLayer.cpp#2310

(if that returns an empty list, that code will never ask the user to select a client certificate because there aren't any)

It might be good to confirm that calling that function doesn't result in asking the user for their master password, if they have one (and aren't logged in). Another issue is that if a user has a 3rd party PKCS#11 module, they might not have any client certificates available at the moment, but as soon as they plug in their token they will, so maybe we should disable speculative connections if they have any such modules. Here's code that enumerates the loaded module list:

https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/security/manager/ssl/nsNSSComponent.cpp#2100

You'll have to filter out the internal module and the "builtin roots module".

There aren't currently any observer notifications when client certificates get added, but I believe these are the only places where they can be added while Firefox is running:

nsNSSCertificateDB::ImportUserCertificate
nsNSSCertificateDB::ImportPKCS12File

( https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSCertificateDB.cpp )
Duplicate of this bug: 902439
(Assignee)

Comment 87

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcbb5224692e3be3ab896e03e6cf93499355d96e
Comment hidden (mozreview-request)
(Assignee)

Comment 89

2 months ago
I should only prevent speculative connections to https hosts when certificates are installed.
Also, the specultative connect tests are failing, because there is a user certificate installed - CN=Mochitest client. I guess I should skip it?
Waiting for feedback on current patch before updating.
tracking-firefox57: ? → +

Comment 90

2 months ago
mozreview-review
Comment on attachment 8908601 [details]
Bug 910207 - Disable preconnect when user certificates are installed

https://reviewboard.mozilla.org/r/180266/#review185486

Nice - this is impressive. I think we should restructure things a bit due to NSS requirements, but this is probably basically what we want.
In terms of tests, it would probably be good to have a test that confirms we don't speculative connect when we do have the client certificate installed. We still want the original speculative connection tests, though, so maybe it would work to remove the certificate first and then add it back as a cleanup task for those tests?

::: netwerk/protocol/http/nsHttpHandler.cpp:533
(Diff revision 1)
>          obsService->AddObserver(this, "net:prune-all-connections", true);
>          obsService->AddObserver(this, "last-pb-context-exited", true);
>          obsService->AddObserver(this, "browser:purge-session-history", true);
>          obsService->AddObserver(this, NS_NETWORK_LINK_TOPIC, true);
>          obsService->AddObserver(this, "application-background", true);
> +        obsService->AddObserver(this, "nss:user-certificate-added", true);

"psm" might be a more appropriate identifier than "nss" in this case

::: netwerk/protocol/http/nsHttpHandler.cpp:2446
(Diff revision 1)
>      return NS_OK;
>  }
>  
>  // nsISpeculativeConnect
>  
> +static bool HasUserCerts()

I think it might be safest to implement this in nsNSSComponent. The concern is that calling NSS functions is undefined behavior if you're not between a NSS_Init and NSS_Shutdown pair of calls. In this case, we're probably after NSS_Init, and we're also probably before NSS_Shutdown because either we saw a "nss:user-certificate-deleted" notification or we're just about to do a speculative connection for the first time, but if this ever changes we're in for a bad time. nsNSSComponent has a better idea of if/when NSS shuts down, so it would be safer to do there. I think you should be able to use nsNSSComponent::mMutex and nsNSSComponent::mNSSInitialized.

::: netwerk/protocol/http/nsHttpHandler.cpp:2477
(Diff revision 1)
> +  // Check if any 3rd party PKCS#11 module are installed, as they may produce
> +  // client certificates
> +
> +  nsCOMPtr<nsINSSComponent> component(do_GetService(PSM_COMPONENT_CONTRACTID));
> +  bool activeSmartCards = false;
> +  component->HasActiveSmartCards(activeSmartCards);

I would actually do this check first since we probably don't even want to query for user certificates if the user does have a smart card (because that might cause NSS to ask the user to log in to their smart card, which would defeat the point of this a bit).

::: security/manager/ssl/nsNSSCertificateDB.cpp:716
(Diff revision 1)
>  
>  NS_IMETHODIMP
>  nsNSSCertificateDB::ImportUserCertificate(uint8_t* data, uint32_t length,
>                                            nsIInterfaceRequestor* ctx)
>  {
> +  nsresult rv = NS_OK;

nit: might as well just declare this closer to where it's used

::: security/manager/ssl/nsNSSCertificateDB.cpp:1012
(Diff revision 1)
> -  return blob.ImportFromFile(aFile);
> +  rv = blob.ImportFromFile(aFile);
> +
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();
> +  if (observerService) {
> +    observerService->NotifyObservers(nullptr, "nss:user-certificate-added", nullptr);

Hmmm - I imagine we should only emit the notification when rv is a success code, right?
Attachment #8908601 - Flags: review?(dkeeler) → review-
Comment hidden (mozreview-request)

Comment 92

2 months ago
mozreview-review
Comment on attachment 8908601 [details]
Bug 910207 - Disable preconnect when user certificates are installed

https://reviewboard.mozilla.org/r/180266/#review186070

PSM parts look good to me with comments addressed. I'm not familiar with the speculative connect code to be able to definitively say that part will work as expected, so you might get a necko peer to review that. Let me know if you want any pointers on the mochitests - I think you should just be able to use the nsIX509CertDB apis to remove and add back the client cert.

::: netwerk/protocol/http/nsHttpHandler.cpp:71
(Diff revision 2)
>  #include "mozilla/BasePrincipal.h"
>  
>  #include "mozilla/dom/ContentParent.h"
>  
> +#include "nsNSSComponent.h"
> +#include "nsNSSCertificate.h"

I suspect this include isn't necessary anymore.

::: security/manager/ssl/nsNSSComponent.cpp:1144
(Diff revision 2)
>  }
>  
>  nsresult
> +nsNSSComponent::HasActiveSmartCards(bool& result)
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Main thread only");

nit: for these main-thread-only functions, we generally also have non-debug code that handles being called on the wrong thread:

https://dxr.mozilla.org/mozilla-central/rev/ae39864562c6048fdc2950c5dfedb48e247c3300/security/manager/ssl/nsNSSComponent.cpp#895

::: security/manager/ssl/nsNSSComponent.cpp:1169
(Diff revision 2)
> +
> +  nsNSSShutDownPreventionLock lock;
> +  MutexAutoLock nsNSSComponentLock(mMutex);
> +
> +  result = false;
> +  UniqueCERTCertList certList(

While nsNSSShutDownPreventionLock will prevent shutdown from happening while we're in this function, it doesn't tell us if NSS has already been shut down. Let's check mNSSInitialized here before calling CERT_FindUserCertsByUsage.
Attachment #8908601 - Flags: review?(dkeeler) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 94

2 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #92)
> Comment on attachment 8908601 [details]
> Bug 910207 - Disable preconnect when user certificates are installed
> 
> https://reviewboard.mozilla.org/r/180266/#review186070
> 
> PSM parts look good to me with comments addressed. I'm not familiar with the
> speculative connect code to be able to definitively say that part will work
> as expected, so you might get a necko peer to review that. Let me know if
> you want any pointers on the mochitests - I think you should just be able to
> use the nsIX509CertDB apis to remove and add back the client cert.

For some reason I can't properly delete the cert.

I tried this, but even though the delete-cert observer event is sent, the cert is not deleted from the DB.

var gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
                .getService(Ci.nsIX509CertDB);
function findCertByCommonName(commonName) {
  let certEnumerator = gCertDB.getCerts().getEnumerator();
  while (certEnumerator.hasMoreElements()) {
    let cert = certEnumerator.getNext().QueryInterface(Ci.nsIX509Cert);
    if (cert.commonName == commonName) {
      return cert;
    }
  }
  return null;
}

var cert = findCertByCommonName("Mochitest client");
info("deleting cert "+cert);
gCertDB.deleteCertificate(cert);

cert = findCertByCommonName("Mochitest client");
info("found cert" + cert);
(Assignee)

Comment 95

2 months ago
Comment on attachment 8908601 [details]
Bug 910207 - Disable preconnect when user certificates are installed

Preemptively requesting release manager approval for landing on fx57
Attachment #8908601 - Flags: checkin?(sledru)
Comment on attachment 8908601 [details]
Bug 910207 - Disable preconnect when user certificates are installed

Let's use NI instead so it doesn't show up on the "patches that need landing" radar in the mean time :)
Flags: needinfo?(sledru)
Attachment #8908601 - Flags: checkin?(sledru)
Shoot - I forgot that certificates aren't actually deleted until they're garbage-collected, so as long as you have a reference to it, it won't go away ( https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/security/manager/ssl/nsNSSCertificate.cpp#152 ).

From what I can tell, we load the client certificate here for all mochitests: https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/testing/mochitest/runtests.py#1751
As far as I know, it's actually only necessary for a handful of tests (namely some in security/manager/ssl/tests/mochitest/ and I think anything that uses requestclientcert.example.com or requireclientcert.example.com). We could potentially remove the code that loads it by default and instead load it as-needed for those tests. We would then have to somehow guarantee that the speculative connect tests are run in a different slice than (or at least before) the client certificate tests.

Another option would be to add/modify the certificate removal API to actually delete the certificate when the delete function is called. I'm not sure how well this will work, because certificates are actually reference-counted at two different levels: the gecko/nsIX509Cert layer and the NSS/CERTCertificate layer (and possibly even the underlying STANCertificate and PKCS#11 layers underneath?). So in theory it might be possible to still have a handle on a certificate we've deleted, and we would have to be smart about that. If we could get this right, it would address things like bug 454782 and bug 1366995, though.
(Assignee)

Comment 98

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=631daa678d13690e7e86ed3e1f49bc6ea47c9224
(Assignee)

Comment 99

2 months ago
(In reply to David Keeler [:keeler] (use needinfo?) from comment #97)
> Shoot - I forgot that certificates aren't actually deleted until they're
> garbage-collected, so as long as you have a reference to it, it won't go
> away

I'm thinking we land the patch, and test that we do not preconnect on https with client certs (because there will always be one).
The reverse (that we do preconnect when there are no client certs) can be verified manually.
Comment hidden (mozreview-request)

Comment 101

2 months ago
mozreview-review
Comment on attachment 8910413 [details]
Bug 910207 - Test that speculative connect is not enabled when there is a user cert installed (https only)

https://reviewboard.mozilla.org/r/181856/#review187504

::: browser/base/content/test/urlbar/browser_urlbar_search_no_speculative_connect_with_client_cert.js:177
(Diff revision 1)
> +  await new Promise(resolve => setTimeout(resolve, 200));
> +
> +  // Now mouseup, expect that we choose a client certificate, and expect that
> +  // we successfully load a page.
> +  expectingChooseCertificate = true;
> +  EventUtils.synthesizeMouse(listitem, 10, 10, {type: "mouseup"}, window);

nit: may test synthesizeMouseAtCenter instead of absolute coords
Attachment #8910413 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)

Comment 103

2 months ago
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/86d1c12919b8
Disable preconnect when user certificates are installed r=keeler
https://hg.mozilla.org/integration/autoland/rev/566ab2ce5329
Test that speculative connect is not enabled when there is a user cert installed (https only) r=mak
Please fill an uplift request to beta!
thanks
Flags: needinfo?(sledru)
https://hg.mozilla.org/mozilla-central/rev/86d1c12919b8
https://hg.mozilla.org/mozilla-central/rev/566ab2ce5329
Status: REOPENED → RESOLVED
Last Resolved: 2 months ago2 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 106

2 months ago
Comment on attachment 8908601 [details]
Bug 910207 - Disable preconnect when user certificates are installed

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1348275

[User impact if declined]:
the user certificate chooser may pop-up before visiting a page

[Is this code covered by automated tests?]:
yes

[Has the fix been verified in Nightly?]:
yes

[Needs manual test from QE? If yes, steps to reproduce]: 
yes.

add a user certificate for the test website
load a page that requires a user certificate
restart browser
type part of the test URL into the URL bar and use the down key to select it (without navigating)
make sure the certificate chooser doesn't pop up, and that the browser didn't pre-connect to the server (using wireshark, server logs, or about:networking)

remove all user certificates and restart the browser
type part of the test URL into the URL bar and use the down key to select it (without navigating)
make sure that the browser preconnects to the test website (using wireshark, server logs, or about:networking)
note that in this case the website doesn't need to require a user cert, but does need to be https.

make sure that we always preconnect to any HTTP websites, regardless if there are any user certs installed.

[List of other uplifts needed for the feature/fix]:
attachment 8910413 [details]

[Is the change risky?]:
no

[Why is the change risky/not risky?]:
It just disables the preconnect when there are any user certificates installed.

[String changes made/needed]:
none
Attachment #8908601 - Flags: approval-mozilla-beta?
Comment on attachment 8908601 [details]
Bug 910207 - Disable preconnect when user certificates are installed

We will probably ship speculative connections in 57, let's take it.
should be in 57b3
Attachment #8908601 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 108

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d81018c99946
https://hg.mozilla.org/releases/mozilla-beta/rev/078e47662790
status-firefox57: affected → fixed
Flags: qe-verify+

Updated

2 months ago
Duplicate of this bug: 1401788

Comment 110

16 days ago
Can you please give me some information about how can I add a user certificate? And an example of a site that uses user certificate? 

Thank you.
Flags: needinfo?(valentin.gosu)
(Assignee)

Comment 111

16 days ago
Hi Oana,

To install a certificate go to:
about:preferences#privacy
Scroll or search to Certificates. Click View certificates. Click import and select the certificate.

If you don't have a client certificate around, you can generate on using these instructions:
[1] https://gist.github.com/mtigas/952344

You can use the links in this example to test. I remember it working.
[2] https://stackoverflow.com/questions/38095559/https-test-server-that-checks-client-certificates

Otherwise you can set up your own server with a client certificate
Instructions for nginx at [1] or apache at [3]

[3] https://stuff-things.net/2015/09/28/configuring-apache-for-ssl-client-certificate-authentication/

Let me know if you encounter any issues.
Flags: needinfo?(valentin.gosu)

Comment 112

15 days ago
First of all thank you, Valentin for all your help.

I managed to partially reproduce the bug using an older version of Nightly from 2017-08-11 on Ubuntu 16.04 x64 with the steps from comment 106. 

I say partially because when I started to write a part of the URL and use the down key to select it (without navigating), the certificate chooser didn't pop up, but the  browser did preconnect to the server. 


I retested everything everything on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13 using latest Nightly 58 and beta 57.0b13. I got the same result with all three browsers:

1. With a user certificate:
 - the certificate chooser didn't pop up 
 - the browser didn't preconnect to the server

2. Without a user certificate:
 - browser preconnected to the test website
 - browser preconnected to any HTTP websites

Can you consider these results valid? Can we mark this bug as verified fixed considering that I was able only to partially reproduce the bug on the machines I used?
(Assignee)

Comment 113

15 days ago
I think this is OK. I'm not exactly sure when the cert chooser would have popped up before, but what is important is that now it doesn't do that without visiting the page.

David, can you confirm?
Flags: needinfo?(dkeeler)

Comment 114

15 days ago
I forgot to mention, but sometimes the url appears in about:networking, but I think that happens because of Activity Stream (the new tab page), because after I dismiss it from Activity Stream and restart the browser, it's not there anymore.
(In reply to Oana Botisan from comment #112)
> 1. With a user certificate:
>  - the certificate chooser didn't pop up 
>  - the browser didn't preconnect to the server

If you then complete the navigation (as in, actually try to go to the site), does the certificate chooser come up?

> 2. Without a user certificate:
>  - browser preconnected to the test website
>  - browser preconnected to any HTTP websites

This sounds like the expected behavior.
Flags: needinfo?(dkeeler) → needinfo?(oana.botisan)

Comment 116

15 days ago
> If you then complete the navigation (as in, actually try to go to the site),
> does the certificate chooser come up?

Yes. If you complete the navigation, then the certificate chooser comes up.
Flags: needinfo?(oana.botisan)
Sounds like it's working as expected, then - thanks!

Updated

15 days ago
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.