Implement HTTPS proxy support - #322

Is it OK? What was the purpose of this method? I couldn't find any references to it

Good question, I have no idea.

I feel really bad about this. There must be a better way to wrap server socket. Any ideas?

I think this is fine, TBH. Maybe promote _wrap_socket to wrap_socket, or even pull that wrap_socket method off the server thread and make it available as a test helper function instead.

I feel that it should extend a more generic ConnectionError exception, but there's no such common exception. But there's one in hyper.http20.exceptions.ConnectionError. Maybe we could introduce it here?

I'm happy to introduce a common ConnectionError that the HTTP/2 one subclasses.

There is a hyper.common.exceptions.SocketError, which is not referenced anywhere in the project. It's very similar to the ConnectionError I've just introduced, I guess.

I suggest to leave them both here for now, but I would like to work on exceptions structure in another PR after this one

Is it OK to explicitly depend on HTTP11Connection here?

Yes, I think so.

Yup, totally fine.

We should probably do this at a higher scope so that we can pass request.url instead of rebuilding it ourselves. That's what the Requests version of this code does.

So this is weird, I know, but I kind of hate classmethods for this use case. IMO, classmethods are for custom constructors, and nothing else. This should just be a function that uses HTTP11Connection directly, I think.

The reason I've made it a classmethod is to ensure that the HTTP11Connection instance, created to connect to a proxy, is not usable/alterable anymore, because its socket is moved to an original connection instance, which picks it up.

Maybe a staticmethod will be better? I'm afraid that an instance method doesn't incapsulate this fragile socket transition properly

Sorry, I should have been clearer. I wasn't suggesting an instance method: I was suggesting a function. Don't hang it off a class at all, it doesn't need one.

This branch is probably simpler as two elif branches that just consist of a one-line call to socket.create_connection.

As this is a constant, this can just be b'CONNECT'. We only need to_bytestring to allow string formatting operations.

Is there any reason we can't just actually use the regular request/response logic, and just not preload any data?

Yes, the reason is that HTTP11Response asserts on presence of some headers like Connection/Content-length which might be absent in the proxy response to a CONNECT.

Maybe I should add a comment here explaining this. Or should we adapt the HTTP11Response to accept responses without these headers?

I think we should probably adapt them to accept responses without those headers.

s/reader/header/

Aww, what a shameful copy-pasted typo 😃

BTW, CONNECT is a method, according to RFC 7231 HTTP/1.1
I will look through my comments again, thanks!

It is a method, yes, but in this case we're reading the whole request header. 😉

I think this is fine, TBH. Maybe promote _wrap_socket to wrap_socket, or even pull that wrap_socket method off the server thread and make it available as a test helper function instead.

s/reader/header/

s/reader/helper/

s/reader/header/

Calling get_connection with the same arguments but different proxy_headers would reuse already created connection, so the new proxy_headers will be ignored.

Our proxy_headers are generated from proxy url, so we should be fine, but this is not strictly guaranteed in this code.

Maybe we should move in the select_proxy logic here, thus generating proxy_headers inside this function?

Any reason not to just make proxy_headers part of the tuple?

yes, dict is unhashable

Yeah, but frozenset of tuple isn't. You could write a transform that does proxy_headers_key = frozenset((k, v) for k, v in proxy_headers.items()).

Please let me know what do you think about bbf59ff

I don't like storing the whole dict as a key. It seems to bloat the key without much benefit

I don't think it resolves the core issue. If you are setting headers for the proxy (e.g. proxy-authorization) then we absolutely must respect that. We need to keep track of them. In 99%+ of cases we will never store anything other than one set of proxy headers and a None value, which is pretty minimal IMO.

Well, OK, will do, but still, I think storing proxy headers in this key is redundant, because they are generated from a proxy which is already a part of this key

They needn't be. Requests has a proxy_headers method expressly to allow it to be overridden, in case users want to provide other forms of proxy header handling.

Alright, that makes sense!
Should I revert that commit, or just add the dict to the key?

At this point we have enough commits that you don't need to worry about reverting: we'll need to squash before we merge to clean them up anyway. 😄 So just make the change directly to get the code looking the way you want it. 😁

Any reason not to just make proxy_headers part of the tuple?

Do we need this code? We inherit from the HTTPAdapter, which already provides this method. Why can't we just call it?

Good point! I must have missed that

As a minor coding style note, it's better to put functions like these towards the top of the file rather than towards the bottom.

The standard style in this codebase for breaking the line would be like this:

sock = _create_tunnel(
    self.proxy_host,
    self.proxy_port,
    self.host,
    self.port,
    proxy_headers=self.proxy_headers
)

Same multiline note.

So, I don't think this feels right. A connection is not a request: a request is just one of many things that can happen on a connection. In this case, all of the places it's used except for the response have access to the request verb, so they can just check what the request verb is. The better solution is to tell the response what the request verb was, which is good practice anyway.

OK, I think I've got this point, will try this out tomorrow

This shouldn't be necessary: as we don't send Upgrade headers on a CONNECT request, we're not going to get a 101 Switching Protocols response back. The standard logic is fine.

It would be better to try to be a bit clever here. We can avoid duplicating the loop above by doing this:

all_headers = headers.items()
if self.proxy_host and not self.secure:
    proxy_headers = self.proxy_headers or {}
    all_headers = itertools.chain(all_headers, proxy_headers.items())

for name, value in all_headers:

That'll keep a single loop, and generally seem a lot cleaner.

I don't love this name. Maybe SocketSecuritySetting?

We don't appear to need the values to be set.

Could you please elaborate on this?
I think we actually need the values to be able to convert boolean secure to an instance of this enum

Hrm. I suppose? It feels pretty weird though, let's put a comment in here to say that's why we're doing it.

It might be nicer to actually return the response object in this case as well, so that users can grab it to read any response body or headers that might be there.

This is intended to be a private API, so we are the users of it :)
We don't seem to need this response as for now. Maybe we could leave it as is? 🙂

The problem is to differ socket from response on consumer side, which seems like an unnecessary complication as for now, I think

It's a private API in the sense that no-one else calls it, but presumably we allow this exception to bubble up, right?

Right. Do you mean that we should add response as exception attribute?

Yup.

Hrm, this doesn't make me much happier, as it turns out. 😞

I think the thing to do is to keep track of only the currently outstanding request method. We don't allow pipelining, really, so we shouldn't have a list: just a single value. We can then use that in get_response. Does that sound more sensible?

Yeah, I was thinking about pipelining while considering a list here.
OK, I will turn this to a single value