378637 - Add support for connecting to HTTP proxy over HTTPS

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Dan Harkless]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

I wanted to set up a remote SSL- and password-protected HTTP proxy on my server so that when I'm on unfriendly networks (like the schoool where my girlfriend works) I can use it to byppass URL / content filtering.  Squid appeared to do what I needed, providing an https_port option that had parameters to point to my site's SSL certificate, but when I tried to point Firefox at the proxy, they wouldn't talk.  Apparently Firefox and the other Mozilla browsers have no way to talk to an HTTP proxy over HTTPS.

The only workaround I can see is to have Squid just listen on the usual unencrypted HTTP port 3128 and use SSH port-forwarding to securely tunnel my proxy authentication and requests over the Internet.  This solution has issues if I'm using someone else's machine without (a port-forwarding-capable) SSH, etc.  It would be nice if the browser could support securely talking to the proxy itself, since the HTTP proxies appear to support HTTPS connections to them (I think this can be set up with Apache mod_proxy as well).

Reproducible: Always

Comment 1

[Dan Harkless]

Oh, I should add that desire for this feature is not limited to just personal remote proxies.  The corporation I work at has recently gotten strict about requiring credentials to be encrypted when sent over our (large) WAN, and new web applications that don't have support for HTTPS are getting denied deployment.

Comment 2

[Andris Atteka]

Chrome can do this: http://www.chromium.org/developers/design-documents/secure-web-proxy

They also have introduced a new PAC file directive to specify an HTTPS proxy (e.g. "HTTPS proxy.com:443")

Comment 3

[Honza Bambas (:mayhemer)]

Bug 573043 is related.  We must make sure that when both or just one of the proxy and the server are using NTLM authentication with Extended Protection enabled/enforced we correctly send the EP data to both.

Comment 4

[Al Billings [:abillings - ex-MoCo]]

Is anyone interested in taking this on?

Comment 5

[Patrick McManus [:mcmanus]]

(In reply to Al Billings [:abillings] from comment #4)
> Is anyone interested in taking this on?

I was until honza said ntlm :)

Comment 6

[Al Billings [:abillings - ex-MoCo]]

The NTLM work is done though, I thought.

Comment 8

[sunyucong]

with the SPDY support in this should have been a easy one, right? Please don't block on features such as NTLM support, let's just get a basic HTTPS proxy support first.

Comment 9

[Curt Wetzel]

This is a pretty important capability for any organization using a forward proxy. Without it web traffic can be sniffed from the local network.

With the sophistication of malware, this is opens an unacceptable security hole for any organization using a standard proxy. Although proxy use is uncommon, here in the US it is used by many public libraries and schools to comply with COPA (the Child Online Protection Act). 

Any login or access to secure website is at risk of being compromised. 

Allowing an HTTPs connection between FireFox and the proxy would greatly reduce the threat.

Considering FireFox can connect to destination websites using HTTPs it seems that allowing a secure connection to the proxy server would be reasonably straight forward.

Comment 10

[otacon]

This is a pretty big issue.

We wanted to authenticate our Proxy against Active Directory using LDAP.

We DON'T want our users to send they internal account Data, also used for VPN connection, unencrypted over the network.

Comment 11

[naegelin]

So with all the recent wiretapping news does anyone else feel like this should float back up to the priority queue =) . I'd be willing to throw down some sponsorship $$ to get this working

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 01/12] add leak detection macros to http classes

From 0089e06dda86afd004c31e9f5eeb098428c51061 Mon Sep 17 00:00:00 2001
---
 netwerk/protocol/http/nsHttpConnectionMgr.cpp | 5 ++---
 netwerk/protocol/http/nsHttpConnectionMgr.h   | 2 +-
 netwerk/protocol/http/nsHttpRequestHead.cpp   | 6 ++++++
 netwerk/protocol/http/nsHttpRequestHead.h     | 1 +
 4 files changed, 10 insertions(+), 4 deletions(-)

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 02/12] change some stack nscstrings to spdystream* autocstring

From 21a282b4b1e5dddc251ab7db51d2cc0bb29d6401 Mon Sep 17 00:00:00 2001
---
 netwerk/protocol/http/Http2Stream.cpp  | 4 ++--
 netwerk/protocol/http/SpdyStream3.cpp  | 4 ++--
 netwerk/protocol/http/SpdyStream31.cpp | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

Comment 14

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 03/12] fix http style: comments, whitespace, formatters

From 78f6022131455068ab29cbcafe2ed3120c944f00 Mon Sep 17 00:00:00 2001
---
 netwerk/protocol/http/Http2Session.cpp        |  4 +-
 netwerk/protocol/http/Http2Stream.cpp         | 56 ++++++++++----------
 netwerk/protocol/http/SpdySession3.cpp        | 52 +++++++++----------
 netwerk/protocol/http/SpdySession31.cpp       | 64 +++++++++++------------
 netwerk/protocol/http/SpdySession31.h         |  6 +--
 netwerk/protocol/http/SpdyStream3.cpp         | 73 +++++++++++++--------------
 netwerk/protocol/http/SpdyStream31.cpp        | 65 ++++++++++++------------
 netwerk/protocol/http/nsHttpConnection.cpp    |  2 +-
 netwerk/protocol/http/nsHttpConnection.h      | 20 ++++----
 netwerk/protocol/http/nsHttpConnectionMgr.cpp | 22 ++++----
 10 files changed, 182 insertions(+), 182 deletions(-)

Comment 15

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 04/12] nshttpconnection - more proactive failed to connect

From 0ca9d0e556e7a032abcb77aa144f6d6a4f982b1d Mon Sep 17 00:00:00 2001
 detection
---
 netwerk/protocol/http/nsHttpConnection.cpp    | 26 +++++++++++++++++++++++-
 netwerk/protocol/http/nsHttpConnection.h      |  5 +++--
 netwerk/protocol/http/nsHttpConnectionMgr.cpp | 29 +++++++++++++++++++--------
 netwerk/protocol/http/nsHttpConnectionMgr.h   |  5 +++++
 4 files changed, 54 insertions(+), 11 deletions(-)

Comment 16

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 05/12] move Spdy*::EnsureBuffer to nsHttp so non spdy code can

From 96ba7732ebece73cfc4bb7cfbf473abe7de886ec Mon Sep 17 00:00:00 2001
 share
---
 netwerk/protocol/http/Http2Push.cpp     |  6 ++----
 netwerk/protocol/http/Http2Session.cpp  | 28 ---------------------------
 netwerk/protocol/http/Http2Session.h    |  3 ---
 netwerk/protocol/http/Http2Stream.cpp   | 18 ++++++-----------
 netwerk/protocol/http/SpdyPush3.cpp     |  6 ++----
 netwerk/protocol/http/SpdyPush31.cpp    |  6 ++----
 netwerk/protocol/http/SpdySession3.cpp  | 34 ---------------------------------
 netwerk/protocol/http/SpdySession3.h    |  4 ----
 netwerk/protocol/http/SpdySession31.cpp | 34 ---------------------------------
 netwerk/protocol/http/SpdySession31.h   |  4 ----
 netwerk/protocol/http/SpdyStream3.cpp   | 18 ++++++-----------
 netwerk/protocol/http/SpdyStream31.cpp  | 18 ++++++-----------
 netwerk/protocol/http/nsHttp.cpp        | 34 +++++++++++++++++++++++++++++++++
 netwerk/protocol/http/nsHttp.h          |  5 +++++
 14 files changed, 63 insertions(+), 155 deletions(-)

Comment 17

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 06/12] nsAHttpTransaction::QueryHttpTransaction

From 77f2b2f795e9e780b0d8180127c51d5406adac87 Mon Sep 17 00:00:00 2001
---
 netwerk/protocol/http/nsAHttpTransaction.h | 5 +++++
 netwerk/protocol/http/nsHttpTransaction.h  | 2 ++
 2 files changed, 7 insertions(+)

Comment 18

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 07/12] newspdysession() no longer takes first transaction

From fc10147320d9cb4f1e3b0ecdf66c96d570e251cf Mon Sep 17 00:00:00 2001
---
 netwerk/protocol/http/ASpdySession.cpp     | 19 ++++++------
 netwerk/protocol/http/ASpdySession.h       |  5 +---
 netwerk/protocol/http/Http2Session.cpp     | 46 +++++++++++++++++-------------
 netwerk/protocol/http/Http2Session.h       |  2 +-
 netwerk/protocol/http/SpdySession3.cpp     | 22 ++++++++------
 netwerk/protocol/http/SpdySession3.h       |  2 +-
 netwerk/protocol/http/SpdySession31.cpp    | 22 ++++++++------
 netwerk/protocol/http/SpdySession31.h      |  2 +-
 netwerk/protocol/http/nsHttpConnection.cpp | 28 ++++++++----------
 9 files changed, 78 insertions(+), 70 deletions(-)

Comment 19

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=51ab0a3f99ee

Comment 20

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 08/12] add https proxy info type

From 7b4412954c763e70fb2cd70a9bacd8c46a483201 Mon Sep 17 00:00:00 2001
---
 netwerk/base/src/ProxyAutoConfig.h          |  2 +-
 netwerk/base/src/nsProtocolProxyService.cpp | 27 +++++++++++++++++++++------
 netwerk/base/src/nsProxyInfo.cpp            |  7 +++++++
 netwerk/base/src/nsProxyInfo.h              |  1 +
 4 files changed, 30 insertions(+), 7 deletions(-)

Comment 21

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 09/12] https proxy info added to connection info and reqeuest

From 6203c5e9ee033757cb8ded299cd8e2b298fa3e55 Mon Sep 17 00:00:00 2001
 head
---
 netwerk/protocol/http/nsHttpChannel.cpp        | 11 ++++++-----
 netwerk/protocol/http/nsHttpConnection.cpp     | 12 ++++++------
 netwerk/protocol/http/nsHttpConnectionInfo.cpp | 25 +++++++++++++++++-------
 netwerk/protocol/http/nsHttpConnectionInfo.h   | 27 +++++++++++++++++++-------
 netwerk/protocol/http/nsHttpConnectionMgr.cpp  | 24 ++++++++++++-----------
 netwerk/protocol/http/nsHttpRequestHead.cpp    |  1 +
 netwerk/protocol/http/nsHttpRequestHead.h      |  4 ++++
 7 files changed, 68 insertions(+), 36 deletions(-)

Comment 22

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 09/12] https proxy info added to connection info and reqeuest

From 6203c5e9ee033757cb8ded299cd8e2b298fa3e55 Mon Sep 17 00:00:00 2001
 head
---
 netwerk/protocol/http/nsHttpChannel.cpp        | 11 ++++++-----
 netwerk/protocol/http/nsHttpConnection.cpp     | 12 ++++++------
 netwerk/protocol/http/nsHttpConnectionInfo.cpp | 25 +++++++++++++++++-------
 netwerk/protocol/http/nsHttpConnectionInfo.h   | 27 +++++++++++++++++++-------
 netwerk/protocol/http/nsHttpConnectionMgr.cpp  | 24 ++++++++++++-----------
 netwerk/protocol/http/nsHttpRequestHead.cpp    |  1 +
 netwerk/protocol/http/nsHttpRequestHead.h      |  4 ++++
 7 files changed, 68 insertions(+), 36 deletions(-)

Comment 23

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 10/12] nsAHttpTransaction::ConnectionInfo

From e64b1060afcabdabc1e82ec74b5d7041ed9ea40f Mon Sep 17 00:00:00 2001
---
 netwerk/protocol/http/Http2Push.cpp           | 13 +++++++++++++
 netwerk/protocol/http/Http2Session.cpp        |  8 ++++++++
 netwerk/protocol/http/NullHttpTransaction.cpp |  6 ++++++
 netwerk/protocol/http/NullHttpTransaction.h   |  2 --
 netwerk/protocol/http/SpdyPush3.cpp           | 13 +++++++++++++
 netwerk/protocol/http/SpdyPush31.cpp          | 13 +++++++++++++
 netwerk/protocol/http/SpdySession3.cpp        |  8 ++++++++
 netwerk/protocol/http/SpdySession31.cpp       |  8 ++++++++
 netwerk/protocol/http/nsAHttpTransaction.h    |  5 +++++
 netwerk/protocol/http/nsHttpPipeline.cpp      | 10 ++++++++++
 netwerk/protocol/http/nsHttpTransaction.cpp   |  6 ++++++
 netwerk/protocol/http/nsHttpTransaction.h     |  1 -
 12 files changed, 90 insertions(+), 3 deletions(-)

Comment 24

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 11/12] static nsHttpHandler::makeconnectstring instead of

From 15d4b4167472ea71fe8f03b1778b26b83eae6500 Mon Sep 17 00:00:00 2001
 member of nsHttpConnection
---
 netwerk/base/public/nsNetUtil.h            |  2 +-
 netwerk/protocol/http/nsHttpConnection.cpp | 66 ++++++++++++++++++------------
 netwerk/protocol/http/nsHttpConnection.h   |  4 ++
 netwerk/protocol/http/nsHttpHandler.cpp    |  2 +-
 netwerk/protocol/http/nsHttpHandler.h      |  2 +-
 5 files changed, 46 insertions(+), 30 deletions(-)

Comment 25

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 12/12] https proxying

From cc55b16e9d2580ae374f088b4e060ad211cd1144 Mon Sep 17 00:00:00 2001
---
 netwerk/base/src/nsSocketTransport2.cpp        |   36 +-
 netwerk/base/src/nsSocketTransport2.h          |    2 +
 netwerk/protocol/http/ASpdySession.cpp         |   10 +-
 netwerk/protocol/http/ASpdySession.h           |    8 +-
 netwerk/protocol/http/Http2Session.cpp         |   21 +-
 netwerk/protocol/http/Http2Session.h           |    3 +-
 netwerk/protocol/http/NullHttpTransaction.cpp  |    2 +-
 netwerk/protocol/http/NullHttpTransaction.h    |    4 +-
 netwerk/protocol/http/SpdySession3.cpp         |  158 ++-
 netwerk/protocol/http/SpdySession3.h           |   13 +-
 netwerk/protocol/http/SpdySession31.cpp        |   21 +-
 netwerk/protocol/http/SpdySession31.h          |    3 +-
 netwerk/protocol/http/SpdyStream3.cpp          |  127 ++-
 netwerk/protocol/http/SpdyStream3.h            |   23 +-
 netwerk/protocol/http/SpdyStream31.cpp         |    4 +-
 netwerk/protocol/http/TunnelUtils.cpp          | 1326 ++++++++++++++++++++++++
 netwerk/protocol/http/TunnelUtils.h            |  216 ++++
 netwerk/protocol/http/moz.build                |    1 +
 netwerk/protocol/http/nsAHttpConnection.h      |    9 +-
 netwerk/protocol/http/nsAHttpTransaction.h     |   20 +-
 netwerk/protocol/http/nsHttpAtomList.h         |    1 +
 netwerk/protocol/http/nsHttpConnection.cpp     |  455 +++++---
 netwerk/protocol/http/nsHttpConnection.h       |   25 +-
 netwerk/protocol/http/nsHttpConnectionInfo.cpp |   26 +-
 netwerk/protocol/http/nsHttpConnectionInfo.h   |   12 +-
 netwerk/protocol/http/nsHttpConnectionMgr.cpp  |  317 ++++--
 netwerk/protocol/http/nsHttpConnectionMgr.h    |   15 +-
 netwerk/protocol/http/nsHttpHandler.h          |    1 +
 netwerk/protocol/http/nsHttpTransaction.cpp    |    2 +
 netwerk/protocol/http/nsHttpTransaction.h      |    4 +
 30 files changed, 2534 insertions(+), 331 deletions(-)
 create mode 100644 netwerk/protocol/http/TunnelUtils.cpp
 create mode 100644 netwerk/protocol/http/TunnelUtils.h

Comment 26

[Patrick McManus [:mcmanus]]

nick - 

first, sorry.

second, I split a bunch of stuff out to make it easier. but most of it still ended up in patch 12.

third, maybe a vidyo session is in order to make it easier?

fourth, I can do a github clone if it would make it easier

fifth - at least I can go do those http2 reviews now :)

Comment 27

[Patrick McManus [:mcmanus]]

fyi- this is a convenient way to setup the config

function FindProxyForURL(url, host) {
return "HTTPS localhost:2443;"
}

Comment 28

[Patrick McManus [:mcmanus]]

the tlsproxy01 branch of the git clone
https://github.com/mcmanus/gecko-dev

has these patches

https://github.com/mcmanus/gecko-dev/tree/tlsproxy01

Comment 29

[u408661]

Comment on attachment 8414560 [details] [diff] [review]
[PATCH 03/12] fix http style: comments, whitespace, formatters

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

::: netwerk/protocol/http/SpdySession31.cpp
@@ +48,5 @@
>  SpdySession31::SpdySession31(nsAHttpTransaction *aHttpTransaction,
>                               nsISocketTransport *aSocketTransport,
>                               int32_t firstPriority)
>    : mSocketTransport(aSocketTransport),
> +    mSegmentReader(nullptr),

Do you want to put commas at the beginning of the line, like in the other blocks in this patch like this?

Comment 30

[u408661]

Comment on attachment 8414563 [details] [diff] [review]
[PATCH 05/12] move Spdy*::EnsureBuffer to nsHttp so non spdy code can

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

Hooray, less duplication of code!

Comment 31

[u408661]

Comment on attachment 8414568 [details] [diff] [review]
[PATCH 09/12] https proxy info added to connection info and reqeuest

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

::: netwerk/protocol/http/nsHttpConnectionInfo.h
@@ +78,4 @@
>      bool UsingProxy();
>  
> +    // Returns true when proxying over HTTP or HTTPS
> +    bool UsingHttpProxy() const { return mUsingHttpProxy | mUsingHttpsProxy; }

|| instead of |

Comment 32

[u408661]

Patrick - just to keep you not so in the dark, first round of review is incoming. At this point, I just have to copy my notes into splinter, and then we can get the ball moving forward on this again.

Comment 33

[u408661]

Comment on attachment 8414571 [details] [diff] [review]
[PATCH 12/12] https proxying

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

OK, here we go!

First - oh my, is this ugly (but in a GOOD way!)
Second - thanks for the gigantic comment at the top of TunnelUtils.h - it made things much easier to understand. We should write this up in more prose and put it on a wiki somewhere (same with spdy and http/2, since they follow a similar pattern of indirection mania).
Third - I'm adding Honza here to get a second pair of eyes. Something this large I don't want to r+ and then have it blow up :)
Fourth - see the rest of my comments below. Most of the really important ones are bigger blobs of text, so they should be easy to pick out. Doesn't mean the others are unimportant, though :)

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +832,5 @@
> +        if (proxyType && (proxyInfo->IsHTTP() ||
> +                          proxyInfo->IsHTTPS() ||
> +                          proxyInfo->IsDirect() ||
> +                          !strcmp(proxyType, "unknown"))) {
> +        proxyType = nullptr;

indentation

::: netwerk/protocol/http/ASpdySession.cpp
@@ +21,5 @@
>  #include "Http2Push.h"
>  #include "SpdySession3.h"
>  #include "SpdySession31.h"
>  #include "Http2Session.h"
> +#include <algorithm>

This doesn't seem used by any of the new code (though maybe it was used in old code that's not showing up in splinter?) Remove it if not used anywhere.

::: netwerk/protocol/http/TunnelUtils.cpp
@@ +155,5 @@
> +    }
> +
> +    mEncryptedTextUsed -= amt;
> +    if (mEncryptedTextUsed) {
> +      memmove (mEncryptedText, mEncryptedText + amt, mEncryptedTextUsed);

Extra space before the paren here (this seems to be a common occurrence with memmove and memcpy, so I'm not going to call them all out).

@@ +1317,5 @@
> +
> +} // namespace mozilla::net
> +} // namespace mozilla
> +
> +#if 0

Get rid of this block, and file followups.

::: netwerk/protocol/http/TunnelUtils.h
@@ +43,5 @@
> +
> +now pop the stack back up to SpdyConnectTransaction::WriteSegment
> +The data has been read and is stored in mInputData
> +
> +  mTunnelConn::OnInputStreamRead(mTunnelStreamIn)

s/OnInputStreamRead/OnInputStreamReady/

@@ +46,5 @@
> +
> +  mTunnelConn::OnInputStreamRead(mTunnelStreamIn)
> +  mTunnelConn::OnSocketReadable()
> +  TLSFilterTransaction::WriteSegment()
> +  RealHttpTrans::WriteSegment()

s/RealHttpTrans/nsHttpTransaction/ (and mark that this is the real trans in parens, like you do with sockets above)

@@ +120,5 @@
> +  void Cleanup();
> +  int32_t FilterWrite(const char *aBuf, int32_t aAmount);
> +  int32_t FilterRead(char *aBuf, int32_t aAmount);
> +
> +  static PRStatus sGetPeerName(PRFileDesc *fd, PRNetAddr*addr);

Not totally crazy about naming these sBlahBlah. It's good to know they're static, but at first glance, I thought they were variables. Maybe use capital S here? Up to you, really.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +12,5 @@
>  #define LOG(args) LOG5(args)
>  #undef LOG_ENABLED
>  #define LOG_ENABLED() LOG5_ENABLED()
>  
> +#include <algorithm>

Just like in ASpdySession.cpp - doesn't seem used (but maybe I missed something). If unused, kill it.

@@ +302,4 @@
>          // By writing 0 bytes to the socket the SSL handshake machine is
>          // pushed forward.
>          uint32_t count = 0;
> +        // write to wrong socket :)

Clarify this comment so we know what's up with writing to the wrong socket.

@@ +1541,5 @@
> +                if (mTLSFilter) {
> +                    LOG(("  blocked tunnel (handshake?)\n"));
> +                    mTLSFilter->NudgeTunnel(this, this, this);
> +                }
> +                else {

Move this up by the preceding closing brace, to match other changes.

@@ +1835,4 @@
>          return rv;
>      }
>  
> +

Extra blank line.

::: netwerk/protocol/http/nsHttpConnection.h
@@ +189,5 @@
>      static nsresult MakeConnectString(nsAHttpTransaction *trans,
>                                        nsHttpRequestHead *request,
>                                        nsACString &result);
> +    void    SetupSecondaryTLS();
> +    void    SetInHttp2Tunnel(bool arg);

I'm not wild about using Http2 here, but Spdy pretty much everywhere else (unless I've missed something and this really is http/2 specific). Can we settle on one (most likely Spdy, since that's what we're already using everywhere else)?

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1072,5 @@
>                  alreadyHalfOpen = true;
>                  break;
>              }
>          }
> + 

Extra whitespace.

@@ +2380,5 @@
>      nsConnectionEntry *ent = LookupConnectionEntry(conn->ConnectionInfo(),
>                                                     conn, nullptr);
> +    if (!ent) {
> +        // this can happen if the connection is made outside of the
> +        // connection manager an is being "reclaimed" for use with

s/an/and/

@@ +2447,2 @@
>      }
>      else {

Move this up by the preceding closing brace, since you've done it elsewhere.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +102,5 @@
>  
>      bool      ProxyConnectFailed() { return mProxyConnectFailed; }
>  
> +    void SetDontRouteViaWildCard(bool var) { mDontRouteViaWildCard = var; }
> +    bool DontRouteViaWildCard() { return mDontRouteViaWildCard; }

I'm not wild (ha!) about calling it DontRouteViaWildCard - in some ways it sounds like setting this to true means it wouldn't go over the tunnel (since the tunnel is like a wild card for all your connections). At least that's how I read it at first. It makes more sense now that I've read the whole patch, but it might be good to at least comment here (and perhaps other key points, such as where you set don't route via wild card to true) that it doesn't mean the transaction won't go over the tunnel, just that it's not responsible for setting up the tunnel.

Comment 34

[Honza Bambas (:mayhemer)]

I'll start next week..  This is.. HUGE!

Comment 35

[u408661]

Comment on attachment 8414571 [details] [diff] [review]
[PATCH 12/12] https proxying

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

So somehow, my intention to r+ and also r? honza turned into clearing r? and r?'ing honza. Let's fix that.

Comment 36

[Jason Duell]

Comment on attachment 8414571 [details] [diff] [review]
[PATCH 12/12] https proxying

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

Talked to Nick--given how busy Honza is and how thoroughly Nick reviewed the code here already, Nick's +r is enough.

Comment 37

[Patrick McManus [:mcmanus]]

(In reply to Nicholas Hurley [:hurley] from comment #31)
> Comment on attachment 8414568 [details] [diff] [review]
> [PATCH 09/12] https proxy info added to connection info and reqeuest
> 
> Review of attachment 8414568 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionInfo.h
> @@ +78,4 @@
> >      bool UsingProxy();
> >  
> > +    // Returns true when proxying over HTTP or HTTPS
> > +    bool UsingHttpProxy() const { return mUsingHttpProxy | mUsingHttpsProxy; }
> 
> || instead of |

thank you!

Comment 38

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=64e4cf18d045

Comment 39

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=2a607cddc4cb

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bafee610170

https://tbpl.mozilla.org/php/getParsedLog.php?id=39377377&tree=Mozilla-Inbound

Comment 41

[Patrick McManus [:mcmanus]]

given the below it seems likely this test will show the error on desktop with debug and pipelining on..

Android 4.0 Panda mozilla-inbound debug test mochitest-4 on 2014-05-09 11:47:26 PDT for push 2a607cddc4cb

PROCESS-CRASH | /tests/content/svg/content/test/test_onerror.xhtml | application crashed [@ mozilla::net::nsHttpPipeline::SetConnection(mozilla::net::nsAHttpConnection*)]
05-09 11:56:34.953 F/MOZ_Assert( 2253): Assertion failure: !mConnection (already have a connection), at /builds/slave/m-in-and-d-0000000000000000000/build/netwerk/protocol/http/nsHttpPipeline.cpp:411
Return code: 1
05-09 11:56:34.953 F/MOZ_Assert( 2253): Assertion failure: !mConnection (already have a connection), at /builds/slave/m-in-and-d-0000000000000000000/build/netwerk/protocol/http/nsHttpPipeline.cpp:411

11:57:29     INFO -   0  libxul.so!mozilla::net::nsHttpPipeline::SetConnection(mozilla::net::nsAHttpConnection*) [nsHttpPipeline.cpp:2a607cddc4cb : 411 + 0x14]
11:57:29     INFO -       r4 = 0x00000000    r5 = 0x68cbb280    r6 = 0x68bf2f40    r7 = 0x804b000d
11:57:29     INFO -       r8 = 0x68cdb9d0    r9 = 0x00000023   r10 = 0x68cdb9d0    fp = 0x5be58260
11:57:29     INFO -       sp = 0x661fab90    lr = 0x62040d3f    pc = 0x62041052
11:57:29     INFO -      Found by: given as instruction pointer in context
11:57:29     INFO -   1  libxul.so!mozilla::net::nsHttpConnectionMgr::DispatchAbstractTransaction(mozilla::net::nsHttpConnectionMgr::nsConnectionEntry*, mozilla::net::nsAHttpTransaction*, unsigned int, mozilla::net::nsHttpConnection*, int) [nsHttpConnectionMgr.cpp:2a607cddc4cb : 1866 + 0x5]
11:57:29     INFO -       r4 = 0x661faba8    r5 = 0x00000000    r6 = 0x68bf2f40    r7 = 0x804b000d
11:57:29     INFO -       r8 = 0x68cdb9d0    r9 = 0x00000023   r10 = 0x68cdb9d0    fp = 0x5be58260
11:57:29     INFO -       sp = 0x661faba0    pc = 0x6203b903
11:57:29     INFO -      Found by: call frame info
11:57:29     INFO -   2  libxul.so!mozilla::net::nsHttpConnectionMgr::DispatchTransaction(mozilla::net::nsHttpConnectionMgr::nsConnectionEntry*, mozilla::net::nsHttpTransaction*, mozilla::net::nsHttpConnection*) [nsHttpConnectionMgr.cpp:2a607cddc4cb : 1797 + 0xf]
11:57:29     INFO -       r4 = 0x5be54940    r5 = 0x00000023    r6 = 0x66381340    r7 = 0x0000000b
11:57:29     INFO -       r8 = 0x68bf2f40    r9 = 0x00000000   r10 = 0x68cdb9d0    fp = 0x5be58260
11:57:29     INFO -       sp = 0x661fabd0    pc = 0x6203ba63
11:57:29     INFO -      Found by: call frame info
11:57:29     INFO -   3  libxul.so!mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady(nsIAsyncOutputStream*) [nsHttpConnectionMgr.cpp:2a607cddc4cb : 3155 + 0xb]
11:57:29     INFO -       r4 = 0x68bf2fa0    r5 = 0x63f96b30    r6 = 0x661fac30    r7 = 0x00000000
11:57:29     INFO -       r8 = 0x00000000    r9 = 0x68bf2fc0   r10 = 0x00000000    fp = 0x5be58260
11:57:29     INFO -       sp = 0x661fac00    pc = 0x6203e1cd
11:57:29     INFO -      Found by: call frame info

Comment 42

[Ryan VanderMeulen [:RyanVM]]

So far, the occurrences have all been Android 4.0 debug mochitest-4, FWIW.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

And B2G debug.
https://tbpl.mozilla.org/php/getParsedLog.php?id=39381502&tree=Mozilla-Inbound

Comment 44

[Patrick McManus [:mcmanus]]

I think patch 4 has uncovered a latent bug..

connection manager does transaction->setconnection(foo)
if (failed(foo->activate())) {
 transaction->setconnection(nullptr);
 destroy (foo)
}

and that's right enough.. nshttppipeline::setconnection(c)
says assert (!mConnection) which makes sense because you don't want a double connection, but doesn't take into consideration the case where c == nullptr where mConnection is being freed.

patch 4 is the reason the test coverage has shifted to show this bug.. a connection failure now shows up in the activate() path at an earlier time than before.

oh that's the theory. let's see what try says :)

Comment 45

[Patrick McManus [:mcmanus]]

Attachment: patch 13

I'm going to present the update as interdiffs to the previous series for ease of review.

This patch tries to ease some of the object heirarchy magic that was going on with weak pointer and do_queryObject in patch 12. If I combined it with patch 12 it would actually simplify that patch, but I think that makes for a harder incremental review.

It also sets the KA Caps flag on a transaction that has forced the creation of a connect tunnel before that transaction is given back to the connection manager for dispatch (in order to use the new tunnel - which is now technically an unused persistent connection.) Given that the transaction honored a no-KA setting on its first trip through the CM in order to create the tunnel, this seems ok.

The Filter transaction now takes the default read/write handlers at ctor time (though they can be updated with individual read/write segment calls). Again - the odd nature of TLS deciding to read when it has only been called for write necessitates it.

Comment 46

[Patrick McManus [:mcmanus]]

Attachment: patch 14 - https proxying definitions for spdy 3.1 and http2

support for http/2 and spdy3.1 that was omitted last time

Comment 47

[Patrick McManus [:mcmanus]]

Attachment: patch 15 - its ok to clear a connection in a nshttppipeline

see comment 44

Comment 48

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=75d2ed5d813c

Comment 49

[u408661]

Comment on attachment 8422640 [details] [diff] [review]
patch 13

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

Looks pretty good, One question below, r=me contingent on an answer :)

::: netwerk/protocol/http/SpdyStream3.cpp
@@ +1558,5 @@
>  
>  void
>  SpdyStream3::MapStreamToHttpConnection()
>  {
> +  nsRefPtr<SpdyConnectTransaction>qiTrans(mTransaction->QuerySpdyConnectTransaction());

nit: space before qiTrans

::: netwerk/protocol/http/TunnelUtils.cpp
@@ -333,5 @@
>    }
>  
>    mSegmentWriter = aWriter;
>    nsresult rv = mTransaction->WriteSegments(this, aCount, outCountWritten);
> -  mSegmentWriter = nullptr;

Was this deletion intentional, given we still set mSegmentWriter above? It seems like this is (or at least could be) right, given the new way of getting mSegment{Writer,Reader} in the ctor, but I want to double-check to make sure it wasn't an oversight.

Also, assuming this is right, what about a similar change for mSegmentReader in ReadSegments?

Comment 50

[Patrick McManus [:mcmanus]]

(In reply to Nicholas Hurley [:hurley] from comment #49)

> 
> Also, assuming this is right, what about a similar change for mSegmentReader
> in ReadSegments?

yes, it was intentional now that segmentreader/segmentwriter can be used in ways other than the corresponding readsegment/writesegment stack. (i.e. ssl reads when asked to write).

I'll add a similar change for ReadSegments() - good suggestion.

Comment 51

[u408661]

Comment on attachment 8422642 [details] [diff] [review]
patch 14 - https proxying definitions for spdy 3.1 and http2

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

::: netwerk/protocol/http/Http2Stream.cpp
@@ +1195,5 @@
> +
> +void
> +Http2Stream::MapStreamToHttpConnection()
> +{
> +  nsRefPtr<SpdyConnectTransaction>qiTrans(mTransaction->QuerySpdyConnectTransaction());

nit: space before qiTrans

::: netwerk/protocol/http/Http2Stream.h
@@ +88,5 @@
>  
>    void UpdateTransportSendEvents(uint32_t count);
>    void UpdateTransportReadEvents(uint32_t count);
>  
> +  // NS_ERROR_ABORT terminates stream, other failure terminates session

random commentary: ugh. not entirely wild about this, but worse things have happened, and we have special-case failure modes all over the place already. One more won't hurt, right?

::: netwerk/protocol/http/SpdyStream31.cpp
@@ +1582,5 @@
> +
> +void
> +SpdyStream31::MapStreamToHttpConnection()
> +{
> +  nsRefPtr<SpdyConnectTransaction>qiTrans(mTransaction->QuerySpdyConnectTransaction());

nit: space before qiTrans

Comment 52

[Patrick McManus [:mcmanus]]

  https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=9cbd4cf8ce64

Comment 53

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9917d4e93398
https://hg.mozilla.org/mozilla-central/rev/856b2a9e1428
https://hg.mozilla.org/mozilla-central/rev/25c7d263eb6e
https://hg.mozilla.org/mozilla-central/rev/ec87f45db49e
https://hg.mozilla.org/mozilla-central/rev/1455ff7a4ecf
https://hg.mozilla.org/mozilla-central/rev/fcf7f73bd7bb
https://hg.mozilla.org/mozilla-central/rev/a6a1c61cba7d
https://hg.mozilla.org/mozilla-central/rev/3b293e8b4713
https://hg.mozilla.org/mozilla-central/rev/c3d1fc8d2c26
https://hg.mozilla.org/mozilla-central/rev/fe6cab453921
https://hg.mozilla.org/mozilla-central/rev/5f549d1d837b
https://hg.mozilla.org/mozilla-central/rev/1d1935f80c97
https://hg.mozilla.org/mozilla-central/rev/ab6d433e563b
https://hg.mozilla.org/mozilla-central/rev/3e4ecea736c3
https://hg.mozilla.org/mozilla-central/rev/9cbd4cf8ce64

Comment 54

[Patrick McManus [:mcmanus]]

Attachment: BO

Approval Request Comment
[Feature/regressing bug #]: this one
[User impact if declined]: new feature that has reliability bugs related only to use of the feature (i.e. it is not generally destabilizing)
[Describe test coverage new/current, TBPL]: backout
[Risks and why]: minimal
[String/UUID change made/needed]: none

I'm going to ask that we disable this on FF32 via flipping an existing pref and instead target FF33. There is no UI - this is just new syntax in the PAC file format that won't be supported yet.

FF32 and FF33 are missing the related fixes for 1040930, 1040323, and 1043402. The first is on FF34 (and will be uplifted to 33) the latter two haven't landed at all yet. They're all expected to be small and suitable for FF33 but FF32 is too aggressive at this stage.

If it goes worse than planned we have plenty of time to disable on FF33, but this feature is eagerly awaited to give parity with Google's Compressing Data Proxy and it is receiving positive feedback.

Comment 55

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8463535 [details] [diff] [review]
BO

OK. We'll back this out as per your suggestion. Can you please file a separate bug for the backout as that is much easier to track.

Comment 56

[Patrick McManus [:mcmanus]]

(In reply to Lawrence Mandel [:lmandel] from comment #55)
> Comment on attachment 8463535 [details] [diff] [review]
> BO
> 
> OK. We'll back this out as per your suggestion. Can you please file a
> separate bug for the backout as that is much easier to track.

https://bugzilla.mozilla.org/show_bug.cgi?id=1045640

Comment 57

[Jason Haar]

Not quite sure where else to comment on this (the links in here fly off to other closed tickets or real dev ones), but if you're going to add proxy support via TLS, then can you make sure client certs are supported too? I'm thinking there are large organizations that already run their own PKI and "everyone" already has client certs installed, so allowing them to use a central/corporate Internet-based proxy server would be a great feature. Obviously it would have to support some form of authentication, and client certs is infinitely preferable to username/password, because infosec best practice is not to expose internal authentication to the Internet. ie they wouldn't like using Active Directory/etc, but a strong authentication model like client certs would be brilliant

Hopefully this wouldn't be too difficult. I've always thought Mozilla were the best at client cert support, being using it for 10+ years via firefox AND thunderbird ;-)

Comment 58

[Sylvestre Ledru [:Sylvestre]]

Already existing in the release notes with "Connect to HTTP proxy over HTTPS" as wording.
I removed it from the 32 release notes and added it to 33 aurora & beta.

Comment 60

[Eric Jung [:ericjung]]

(In reply to Jason Haar from comment #57)
> Not quite sure where else to comment on this (the links in here fly off to
> other closed tickets or real dev ones), but if you're going to add proxy
> support via TLS, then can you make sure client certs are supported too? I'm
> thinking there are large organizations that already run their own PKI and
> "everyone" already has client certs installed, so allowing them to use a
> central/corporate Internet-based proxy server would be a great feature.
> Obviously it would have to support some form of authentication, and client
> certs is infinitely preferable to username/password

I think you should file a new bug for this.

Comment 61

[Jeremy Akers]

I've been watching this bug/issue since 2013 and was quite pleased when I saw that it was being addressed.

However I've been trying to find documentation on how to actually use this new feature to connect to proxies via HTTPS and I can't seem to find it.  My Google searches keep taking me to either bugs like this or old documents that say it's not supported.

I've tried setting it up in the UI as well as in a proxy.pac file.  I'm sure I'm doing something wrong but I can't find the "manual" for how to do it so I thought maybe someone watching this bug would know where I could find the documentation for this feature?

Comment 62

[Xander D Harkness]

@Jeremy Akers

I wrote a very quick and rough how to set up and connect to squid over SSL here:
https://ssl.harkness.se/proxy-ssl/

Comment 63

[toddy.sun]

@Xander D Harkness,

I tried the same as you instructed, but without any luck. The ssl proxy does not work for me.

I’m using squid-3.4.10, configured below:
http_port 8080
https_port 8443 cert=/path/to/cert.pem

When I set my chrome to connect to port 8080, it works. but when I used 8443 the https_port, it does not. The error is below:

A proxy server is a server that acts as an intermediary between your computer and other servers. Right now, your system is configured to use a proxy, but Google Chrome can’t connect to it.
If you use a proxy server…
Check your proxy settings or contact your network administrator to make sure the proxy server is working. If you don’t believe you should be using a proxy server: Go to the Chrome menu > Settings > Show advanced settings… > Change proxy settings… and make sure your configuration is set to “no proxy” or “direct.”
Error code: ERR_PROXY_CONNECTION_FAILED

Do you have any ideas about why this is happening?

Is there any other steps I need to do to allow the browser trust the cert from proxy server?

Thanks.

Comment 64

[toddy.sun]

BTW, when I'm using firefox 36, the browser keeps asking me "This Connection is Untrusted" and let me "add exception". but after I added exception, it redirects to the same page, like a infinite loop.

Comment 65

[Patrick McManus [:mcmanus]]

toddy.sun - it sounds like your proxy has an invalid cert according to your trust list. you've got at least 3 choices

1] add CA signing cert used to create the proxy cert to your root store.. there are lots of tutorials for this on the web (http://www.herongyang.com/Cryptography/Web-Browser-Firefox-Import-CA-Certificate.html)

2] get yourself a cert firefox considers valid by default. startssl.com will do that for you for free.

3] you can do a TOFU exception for the proxy case, but you have to do it a little differently.
 a] turn off the proxy use in firefox.
 b] put https://PROXYNAME:PROXYPORT/ in the location bar (use the same name and port number as you have configured in the PAC - you can't use ip addresses or default ports.. you can't use ip addresses because they can't be verified by the PKI and exceptions are stored per port.
 c] override the cert warning and perm. store the exception. The response you get will be meaningless as you are now addressing the proxy port as if it was an endpoint
 d] turn the proxy back on and use it.

hth

Comment 66

[toddy.sun]

Thank you Patrick for the instruction, that solves my issue.

Comment 67

[herrernst]

Thanks, this is really cool. Is it possible already to set an such an proxy in settings dialog or in about:config? I am using a PAC file at the moment. Haven't found any more information about it.

Comment 68

[Patrick McManus [:mcmanus]]

network.proxy.autoconfigurl = data:text/plain,function FindProxyForURL(u,h){return HTTPS host:port;}

Comment 69

[herrernst]

Thanks Patrick, nice way to use a PAC without having to host it somewhere. (You can get rid of another 3 extra characters, the unused params "u,h".)

Still, a nice setting would be preferable.

Comment 70

[Eric Jung [:ericjung]]

This is now supported natively in the FoxyProxy addon. No PAC required. See screenshot here: http://i.imgur.com/TGIEk8c.jpg

If the proxy server's certificate is not valid, you are prompted to accept the certificate into the Fx store.

Be sure you are using FoxyProxy Standard 4.6.1 or FoxyProxy Basic 3.6.1 (or higher). They are submitted to AMO and awaiting approval.

Comment 71

[Erick Zite]

BTW, when I'm using firefox 36, the browser keeps asking me "This Connection is Untrusted" and let me "add exception". but after I added exception, it redirects to the same page, like a infinite loop.


:cool: https://logoreg.com/favicon.png

Comment 72

[Patrick McManus [:mcmanus]]

1]get your proxy a cert from LE

or

2a] momentarily disable the proxy
2b] put https://PROXYIP:PORT/ in the location bar
2c] get cert warning and store the exception for it
2d] re-enable the proxy

basically what is happening is that you are storing exceptions per origin rather than for the proxy.

Comment 73

[Eric Jung [:ericjung]]

(In reply to Patrick McManus [:mcmanus] from comment #72)
> 1]get your proxy a cert from LE
> 
> or
> 
> 2a] momentarily disable the proxy
> 2b] put https://PROXYIP:PORT/ in the location bar
> 2c] get cert warning and store the exception for it
> 2d] re-enable the proxy
> 
> basically what is happening is that you are storing exceptions per origin
> rather than for the proxy.

FYI, you don't need to do any of this if you use the FoxyProxy extension 4.6.1. You'll get prompted to store the cert when you setup the proxy for the first time, and that's all.

Comment 74

[Ilguiz Latypov]

(In reply to Patrick McManus [:mcmanus] from comment #68)

network.proxy.autoconfigurl = data:text/plain,function
FindProxyForURL(u,h){return HTTPS host:port;}

The latest beta 77.0b8 removes spaces in the Settings dialog, so I had to stick %20 (but in the copy below I edited out the domains and the proxy host:port).

data:text/plain,function%20FindProxyForURL(u,h){if(isPlainHostName(h)||dnsDomainIs(h,".dom1.com")||dnsDomainIs(h,".dom2.net")){return%20"DIRECT";}else{return%20"HTTPS%20host:port"};}