Allow some sharing of connections with different anonymous attributes

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
19 days ago

People

(Reporter: annevk, Assigned: mayhemer)

Tracking

({dev-doc-needed})

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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

2 years ago
+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.
(Reporter)

Comment 2

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

Comment 3

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

Updated

2 years ago
Whiteboard: [necko-backlog]
Summary: Change the way we allocate connections → Allow some sharing of connections with different anonymous attributes
(Assignee)

Comment 4

2 years ago
(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

2 years ago
(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.
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3

Updated

2 years ago
Assignee: nobody → juhsu
Priority: P3 → P2
Whiteboard: [necko-backlog] → [necko-triaged]
Posted patch test WIP, v1 (obsolete) — Splinter Review
A simple test case
Posted patch ntlmHandle WIP, v1 (obsolete) — Splinter Review
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
Flags: needinfo?(mcmanus)
(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?
Flags: needinfo?(honzab.moz)
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.
Flags: needinfo?(mcmanus)
cancel ni because of comment 13
Flags: needinfo?(honzab.moz)

Comment 15

a year ago
(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?
(Assignee)

Comment 16

a year ago
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!

Updated

a year ago
Assignee: juhsu → nobody
(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.
(Assignee)

Updated

a year ago
Assignee: nobody → honzab.moz
(Assignee)

Comment 18

a year ago
Posted patch wip1 (obsolete) — Splinter Review
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)
Attachment #8939768 - Attachment is obsolete: true
Attachment #8941775 - Attachment is obsolete: true
Attachment #8949846 - Flags: feedback?(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 on attachment 8949846 [details] [diff] [review]
wip1

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

yeah, that's my thought. does it work?
Attachment #8949846 - Flags: feedback?(mcmanus) → feedback+
(Assignee)

Comment 21

a year ago
(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.
(Assignee)

Comment 22

a year ago
Posted patch v1 (obsolete) — Splinter Review
final version, test is based on test_origin.js which is more suitable.
Attachment #8949846 - Attachment is obsolete: true
Attachment #8950636 - Flags: review?(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?
Attachment #8950636 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 24

a year ago
(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.
(Assignee)

Comment 25

a year ago
Posted patch v1.1Splinter Review
waiting for pat's dep bug
Attachment #8950636 - Attachment is obsolete: true
Attachment #8951313 - Flags: review+
Depends on: 1439105
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 26

a year ago
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00ba29877daf
HTTP/2 anonymous/onymous session (connection) coalescing, r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/00ba29877daf
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60

Comment 28

7 months ago
Has any relevant changes to the Fetch spec land with this behaviour change?
You need to log in before you can comment on or make changes to this bug.