1363284 - Allow some sharing of connections with different anonymous attributes

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Anne (:annevk)]

Currently anonymous requests (Anon R) and credentialed requests (Cred R) go over different connections (Cs), even when the origin is the same. Cs that take Cred R can also become authenticated (Auth C).

(In https://groups.google.com/d/topic/mozilla.dev.tech.network/glqron0mRko/discussion we did not discuss what currently happens with Cs that take Anon Rs and try to authenticate themselves. I suspect the Anon R that triggers it must fail in the NTLM case and the C must fail in the TLS client certificate case?)

The proposed change to this model is that Anon and Cred Rs go over the same C as long as it's not an Auth C. If an Anon R triggers the server to negotiate NTLM, then fail the Anon R. If C has carried Anon Rs and the server tries to negotiate TLS client certificates, then fail C. An Auth C can continue to take any Cred Rs, but a new Anon R would create a new C. (Presumably any Cred Rs would continue to go over the Auth C as long as it's active. Don't think we discussed this.)

HTTP/2 Cs cannot be authenticated to begin with and can therefore always share Anon and Cred Rs (though note that this is also a change from our current behavior).

Comment 1

[Jake Archibald]

+1

It feels like we could extend this to other origins under the same authority.

Currently, if example.com includes <link rel="stylesheet" href="https://cdn.example.com/styles.css">, which references a font also on https://cdn.example.com, you end up with three connections. It'd be great to use one.

Comment 2

[Anne (:annevk)]

Yeah, if they share a certificate with the appropriate SAN entries that would fall out of OP.

Comment 3

[Yoav Weiss]

+1 - this would resolve a lot of confusion around `<link rel=preconnect>` and H2 push for resources fetched with anonymous requests.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Yoav Weiss from comment #3)
> +1 - this would resolve a lot of confusion around `<link rel=preconnect>`
> and H2 push for resources fetched with anonymous requests.

How exactly?

Comment 5

[Yoav Weiss]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> (In reply to Yoav Weiss from comment #3)
> > +1 - this would resolve a lot of confusion around `<link rel=preconnect>`
> > and H2 push for resources fetched with anonymous requests.
> 
> How exactly?

Preconnect hints have to include the `crossorigin` attribute for connections that will use the anonymous connection pool. So we're forcing users to be aware of that implementation detail and take it into account.

H2 pushed resources are stored in a container linked to a specific H2 connection, so pushing resources that would use a separate connection (e.g. Fonts on a sharded domain) often results in wasted bandwidth.

Comment 6

[Jake Archibald]

I wrote a little about the issues in terms of H2 push https://jakearchibald.com/2017/h2-push-tougher-than-i-thought/#requests-without-credentials-use-a-separate-connection

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 8

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 9

[Junior [inactive]]

Attachment: test WIP, v1

A simple test case

Comment 10

[Junior [inactive]]

Attachment: ntlmHandle WIP, v1

Comment 11

[Junior [inactive]]

Alleged that we don't have a notification to know when client auth has
completed. Therefore, I'd like to do a conservative way: Whenever we set
the client cert during the server requests [1], we won't allow anonymous
requests to share this connection.

What do you think, Patrick?

[1] https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/security/manager/ssl/nsNSSIOLayer.cpp#2216

Comment 12

[Junior [inactive]]

(In reply to :junior from comment #10)
> Created attachment 8941775 [details] [diff] [review]
> ntlmHandle WIP, v1

We want to know the time we complete a ntlm auth. I check (a) the request
contains ntlm auth (b) response is 200ok.

Does it make sense, Honza?

Comment 13

[Patrick McManus [:mcmanus]]

This feature should not apply to h1 at all beacuse ambient auth (i.e. ntlm or client certs)) may be applied at any time and the connectionInfo for the connection is immutable.

h2 does not allow ambient auth. period.

you therefore do not need to specifically check ntlm.

Comment 14

[Junior [inactive]]

cancel ni because of comment 13

Comment 15

[Ryan Sleevi]

(In reply to Patrick McManus [:mcmanus] from comment #13)
> This feature should not apply to h1 at all beacuse ambient auth (i.e. ntlm
> or client certs)) may be applied at any time and the connectionInfo for the
> connection is immutable.
> 
> h2 does not allow ambient auth. period.

You don't allow the negotiation of client certs if requested during the initial handshake (e.g. non-renego client cert scenarios)? Given that the client has already offered the ALPN identifiers, do you continue authenticating and then fail if the server chose H/2, do you abort the connection and then retry without ALPN (and with a selected cert), or something else?

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8941775 [details] [diff] [review]
ntlmHandle WIP, v1

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1619,5 @@
>  
> +        if (mHttpVersion <= NS_HTTP_VERSION_1_1 &&
> +            HasConnectionBasedRequestHeader() &&
> +            mHttpResponseCode == 200) {
> +            // TODO: set anonymous hash key for the following HTTP reqeust

better is to do < 400.


let me understand what is the intention of this patch:

1. when an H1 CONNECTION becomes authenticated with NTLM/Negotiate/Kerberos (aka connection-based auth) it can no longer be used for sending anonymous requests

2. when an ANONYMOUS REQUEST made on an unauthenticated H1 CONNECTION gets 401/NTLM etc response, we fail that request w/o re-attempting it  (I'm not sure what to do with the connection then since sending a different request is likely going to break the server negotiation state)

3. when there is a 401/NTLM etc response to a REGULAR (not anonymous) REQUEST on an H2 STREAM, we restart that request using H1

4. when there is a 401/NTLM etc response to an ANONYMOUS REQUEST on an H2 STREAM, we fail that request (same as in H1 case) w/o re-attempting it


We probably need to implement 1 as an attribute of an H1 CONNECTION object.  However, great care has to be taken to connection limits.  If we have 6 ambient authenticated H1 CONNECTIONS and we need to send an ANONYMOUS REQUEST, we are at the per-host limit.  Hence, a pool of anonymous connections, as we have now, should start opening connection when we reach this limit.

OTOH, when we have ambient authenticated idle H1 CONNECTION and 5 other unauthenticated, we can use one of the 5 for the anon request.

I've recently implemented kinda related logic for urgent-start transactions in bug 1405446.  You may end up with similar thing for anonymous request dispatch.

We need to implement 2 and 4 independently of H version, probably.

We already have 3!

Comment 17

[Patrick McManus [:mcmanus]]

(In reply to Ryan Sleevi from comment #15)
> You don't allow the negotiation of client certs [during handshake]

its a good point ryan, thanks. we do. fortunately that doesn't add significant complexity to the approach.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: wip1

roughly tested only for stability (seems not to break anything).  real life example to test with would be nice to have.

next step is to write a test for this (probably hack on test_http2.js)

Comment 19

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #18)
> Created attachment 8949846 [details] [diff] [review]
> wip1
> 
> roughly tested only for stability (seems not to break anything).  real life
> example to test with would be nice to have.
> 
> next step is to write a test for this (probably hack on test_http2.js)

I haven't looked at the patch yet.. so just replies to the comment:

* for an example, a font fetch would be a likely candidate (.woff or .woff2) - they get fetched without credentials and so have traditionally gone into separate namespaces

* test - altsvc.js (or whatever it is named) is a better template and it also uses the http2 server in the CI. (you need to track it down in the testing dir to add services).. its just better in how it does cert management.

Comment 20

[Patrick McManus [:mcmanus]]

Comment on attachment 8949846 [details] [diff] [review]
wip1

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

yeah, that's my thought. does it work?

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #20)
> Comment on attachment 8949846 [details] [diff] [review]
> wip1
> 
> Review of attachment 8949846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> yeah, that's my thought. does it work?

seems to!  I've made a manual test at https://www.janbambas.cz/moz/bug1363284/ loading some fonts from a 3rd party domain and the connection of the css referring the fonts is shared with the font payload connection.

if writing an automated test comes too complicated, I will go with that confirmation.

thanks for the tip on how to write this, Patrick.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v1

final version, test is based on test_origin.js which is more suitable.

Comment 23

[Patrick McManus [:mcmanus]]

Comment on attachment 8950636 [details] [diff] [review]
v1

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

awesome. thanks.

please don't check this in yet - I want to make it dependent on another bug I will file (and have a patch for) later today.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3797,5 @@
> +    RefPtr<nsHttpConnectionInfo> anonInvertedCI(specificCI->Clone());
> +    anonInvertedCI->SetAnonymous(!specificCI->GetAnonymous());
> +    nsConnectionEntry *invertedEnt = mCT.GetWeak(anonInvertedCI->HashKey());
> +    if (invertedEnt) {
> +        nsHttpConnection* h2conn = gHttpHandler->ConnMgr()->GetSpdyActiveConn(invertedEnt);

isn't ghttphandler->ConnMgr() this?

@@ +3798,5 @@
> +    anonInvertedCI->SetAnonymous(!specificCI->GetAnonymous());
> +    nsConnectionEntry *invertedEnt = mCT.GetWeak(anonInvertedCI->HashKey());
> +    if (invertedEnt) {
> +        nsHttpConnection* h2conn = gHttpHandler->ConnMgr()->GetSpdyActiveConn(invertedEnt);
> +        if (h2conn && h2conn->IsExperienced() && h2conn->NoClientCertAuth()) {

maybe MOZ_ASSERT(h2conn->UsingSpdy()) ?

::: netwerk/test/unit/test_origin.js
@@ +65,5 @@
>    }).QueryInterface(Ci.nsIHttpChannel);
>  }
>  
>  var nextTest;
> +var origin;

this was an accident?

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #23)
> isn't ghttphandler->ConnMgr() this?

yes!

> maybe MOZ_ASSERT(h2conn->UsingSpdy()) ?

done

> > +var origin;
> 
> this was an accident?

Produces just a warning, no functional change.  I had some failure initially in my new test and spot this when looking into the logs, so I fixed that.  But it will be removed from the patch, since I don't want to fight the TV errors again after pushing the patch, it's just annoying and not worth.

thanks.

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: v1.1

waiting for pat's dep bug

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00ba29877daf
HTTP/2 anonymous/onymous session (connection) coalescing, r=mayhemer

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/00ba29877daf

Comment 28

[Yoav Weiss]

Has any relevant changes to the Fetch spec land with this behaviour change?