We should use nsHttpConnectionInfo's hash fro this.

I would suggest to put this one under a pref for now.

Docs:
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/TunnelUtils.h
https://http2.github.io/http2-spec/#CONNECT

The model (tunneling) is going to look like this, IIUC:

[an end-to-end request transaction] (-> [h2 stream -> h2 session]) -> ["virtual" nsHttpConnection keyed by TOKEN] -> [h2 tunnel stream for TOKEN] -> [h2 session for TOKEN] -> ["real" nsHttpConnection for TOKEN]

the TOKEN and proxy settings being assigned per end-to-end request by an/the extension.

As I understand, one h2 session can have multiple tunneling streams - I mean by the spec, not by our Secure Proxy requirement. I don't want to oppose, just want to make clear if for each separate TOKEN we will create a new chain of one h2 tunnel stream->one h2 session->one connection and NOT multiple different h2 tunnel streams each with a different TOKEN->one token-less h2 sessions->one token-less connection.

this depends on how the authentication will be handled, specifically on the server side. the P-A request header is sent with the CONNECT request on the h2 tunneling stream, and not at the stage of establishing an h2 session+connection. Hence, it seems to me that separating the h2 session+connection with the TOKEN doesn't seem strictly necessary on the client side.

the TOKEN should be added to the proxy info structure
https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/netwerk/base/nsProxyInfo.h#27

and added to the connection key here:
https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/netwerk/protocol/http/nsHttpConnectionInfo.cpp#170-190

this WILL make "virtual" connections separate by the TOKEN. if we want to separate "real" connections as well, it needs to be done separately. what I can't easily read from the docs is how we establish new tunnel streams to the same proxy - if we find the existing session or we create a new one (with a new connection) or we let the connection coalescing kick in somehow.

Shane, Dragana, please see the previous comment.

Is this something generically useful? If so, I've no issue with exposing additional params on proxyinfo from the webext proxy api. The work underneath needs to be done. If it's something limited to mozilla only, we can probably just add a permission check when validating the proxy info returned by an extension.

(In reply to Honza Bambas (:mayhemer) from comment #2)

Docs:
https://searchfox.org/mozilla-central/source/netwerk/protocol/http/TunnelUtils.h
https://http2.github.io/http2-spec/#CONNECT

The model (tunneling) is going to look like this, IIUC:

[an end-to-end request transaction] (-> [h2 stream -> h2 session]) -> ["virtual" nsHttpConnection keyed by TOKEN] -> [h2 tunnel stream for TOKEN] -> [h2 session for TOKEN] -> ["real" nsHttpConnection for TOKEN]

the TOKEN and proxy settings being assigned per end-to-end request by an/the extension.

As I understand, one h2 session can have multiple tunneling streams - I mean by the spec, not by our Secure Proxy requirement. I don't want to oppose, just want to make clear if for each separate TOKEN we will create a new chain of one h2 tunnel stream->one h2 session->one connection and NOT multiple different h2 tunnel streams each with a different TOKEN->one token-less h2 sessions->one token-less connection.

Yes, we want that a single h2 connection carries only the same tokens. The tokens will be reused, we may have multiple request have the same token. At the end we want that request with privateBrowsing flag do not use the same token. Also request from different containers should have different tokens. Our existing nsHttpConnectionInfo's hash will already separate those but token will be updated at some point (after x hours) and we do not want to mix these on the same connection therefore we need to extend nsHttpConnectionInfo's hash.

this depends on how the authentication will be handled, specifically on the server side. the P-A request header is sent with the CONNECT request on the h2 tunneling stream, and not at the stage of establishing an h2 session+connection. Hence, it seems to me that separating the h2 session+connection with the TOKEN doesn't seem strictly necessary on the client side.

Theoretically multiple tokens can go on the same connection. we want to separate them because of tracking.

the TOKEN should be added to the proxy info structure
https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/netwerk/base/nsProxyInfo.h#27

and added to the connection key here:
https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/netwerk/protocol/http/nsHttpConnectionInfo.cpp#170-190

this WILL make "virtual" connections separate by the TOKEN. if we want to separate "real" connections as well, it needs to be done separately. what I can't easily read from the docs is how we establish new tunnel streams to the same proxy - if we find the existing session or we create a new one (with a new connection) or we let the connection coalescing kick in somehow.

why "if we want to separate "real" connections as well, it needs to be done separately." this? nsHttpConnectionInfo separates connections. nsHttpConnection manage will only dispatch a transaction onto a connection if nsHttpConnectionInfo hash matches. if it does not find one it will create a new one.

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

Is this something generically useful? If so, I've no issue with exposing additional params on proxyinfo from the webext proxy api. The work underneath needs to be done. If it's something limited to mozilla only, we can probably just add a permission check when validating the proxy info returned by an extension.

I think we can limit this only to mozilla. This could be useful generally, but we do not have demand for it now.

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

Is this something generically useful? If so, I've no issue with exposing additional params on proxyinfo from the webext proxy api. The work underneath needs to be done. If it's something limited to mozilla only, we can probably just add a permission check when validating the proxy info returned by an extension.

I was considering this as one possibility to implement the separation-by-token feature where the proxy+token information comes from an extension. We need to know the token before we start building the request (and its connection - which is the object carrying the separation information) to separate correctly and equip the CONNECT request with the token to prevent 407 handling inside necko. we should get 407 only when the token is expired or bad and the extension should handle it, not necko. this may sound like a detail, but we need a clear plan how to implement this whole feature.

Forgot about this:
https://searchfox.org/mozilla-central/rev/b59a99943de4dd314bae4e44ab43ce7687ccbbec/netwerk/protocol/http/nsHttpConnection.cpp#2216

We CAN set the P-A header from on-modify-request handlers too.

However, we'd still need another attribute exposed somewhere (on the channelwrapper probably) to provide the additional isolation keying.

Providing both (P-A and isolation key) with the proxy information still has following advantages:

• one perf-wise, we can do earlier preconnections
• there is only one place (callback) an ext has to implement to provide all the necessary information to use a privacy proxy

So, this code allows an extension to set the two new properties and necko to use them. I was struggling to create a test tho. There is a WIP patch for it, but it actually can never work, because we either need an h2 tunneling proxy or ssltunnel running for toolkit/components/extensions/test/xpcshell tests.

Shane?

Shane, please see comment 10.

this is a very drafty test, it never gets a response to the CONNECT request from httpd (even 400) for the first of the two async_tasks.

Marking this bug up for dev-doc so the mdn/proxy page gets updated (see url)

(In reply to Honza Bambas (:mayhemer) from comment #10)

So, this code allows an extension to set the two new properties and necko to use them. I was struggling to create a test tho. There is a WIP patch for it, but it actually can never work, because we either need an h2 tunneling proxy or ssltunnel running for toolkit/components/extensions/test/xpcshell tests.

Shane?

For extensions, you just want to test that the new values are set into channelwrapper and then into the details for a listener, when they have been added via proxy.onRequest.

You shouldn't need any tunneling for that (and the WIP seems to show you went this route). Any actual h2 and/or isolation testing should be done without extensions involved.

I'm building now to take a look at the test.

I dont think that createHttpServer provides https. If you want to do the request against https you'll have to write up a server to do that. I don't think it's necessary in this test since the important thing is to see the new proxy values reflected back to the webrequest listeners and that proxy-auth happens without calling into the extension. Attaching a working test.

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)

I dont think that createHttpServer provides https. If you want to do the request against https you'll have to write up a server to do that. I don't think it's necessary in this test since the important thing is to see the new proxy values reflected back to the webrequest listeners and that proxy-auth happens without calling into the extension. Attaching a working test.

Thanks! And you are right, I will write a network xpcshell test where we have https support for just the internal parts.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f29b741097c83c989fe184ab3f264ce096d5a1e

https://hg.mozilla.org/mozilla-central/rev/5cb05da8e3ab

I have added the following to the definition of proxyInfo:

proxyAuthorizationHeader
string. The authorization header value for the request attempt. Contains the credentials to authenticate a user agent to a proxy server.
connectionIsolationKey {{optional_inline}}
string. An optional key used for additional isolation of this proxy connection.

Please check what I've added and let me know if there is more I should say here. As far as this issue is concerned, I realize it is the bare minimum of information but I think it covers this change.

I'm not sure I understand how you would like to present additional information that should go in other places to describe the consequences of the change. If what I have is right, a pointer to where else this information needs to go and a rough description of what Add-on developers need to know would be greatly appreciated. If you could add the information to this issue, or send me an email with the information, I will create an issue in the documentation repo on GitHub to track the additional changes.

(In reply to Irene Smith from comment #23)

I have added the following to the definition of proxyInfo:

proxyAuthorizationHeader
string. The authorization header value for the request attempt. Contains the credentials to authenticate a user agent to a proxy server.

I'd rather write it like this:

"This string, if set to non-empty, is passed directly as a value to Proxy-Authorization request header sent to HTTP proxies as part of plain HTTP requests and CONNECT requests. Simply said, this can be used to directly authenticate to HTTP proxies requiring (non-challenging) authentication.

For instance, if you want to send "username" and "password" for "basic" authentication, you can set proxyAuthorizationHeader to Basic dXNlcm5hbWU6cGFzc3dvcmQ="

connectionIsolationKey {{optional_inline}}
string. An optional key used for additional isolation of this proxy connection.

I think this is fine.

Please check what I've added and let me know if there is more I should say here. As far as this issue is concerned, I realize it is the bare minimum of information but I think it covers this change.

I'm not sure I understand how you would like to present additional information that should go in other places to describe the consequences of the change. If what I have is right, a pointer to where else this information needs to go and a rough description of what Add-on developers need to know would be greatly appreciated. If you could add the information to this issue, or send me an email with the information, I will create an issue in the documentation repo on GitHub to track the additional changes.

I think what you have is fine.

Thanks!

Thanks! I will update according to your suggestion.

Can you please confirm the contents of the example you included above? This is what it says:

For instance, if you want to send "username" and "password" for "basic" authentication, you can set proxyAuthorizationHeader to Basic dXNlcm5hbWU6cGFzc3dvcmQ="

Should it really end with a single quotation mark as you have or should there have been more?

(In reply to Irene Smith from comment #26)

Can you please confirm the contents of the example you included above? This is what it says:

For instance, if you want to send "username" and "password" for "basic" authentication, you can set proxyAuthorizationHeader to Basic dXNlcm5hbWU6cGFzc3dvcmQ="

Should it really end with a single quotation mark as you have or should there have been more?

It should have stand like this:

Basic dXNlcm5hbWU6cGFzc3dvcmQ= (or no quotes at all, here the ` is just for markdown).

Sorry for the confusing typo.