Add HTTPS proxy support

[puffi]

This has a lot of interface changes, every change should be mentioned in the commit message.

Some things are still not clear to me, one is how to handle different TLS implementations when the connection to the proxy is made. We can't just reuse the TlsConnector for both the proxy connection as well as the connection to the target.

[algesten]

Thanks for this PR!

Currently it makes changes that would break implementations for people using ureq. That means we would need to release these changes with a major version bump – and we're not keen to do that right now.

We can park this PR until we are ready to do a major bump. Alternatively if there's some way of preserving the current public API – that would be preferable.

[algesten]

@puffi I just had a realisation. Because the fix for #461 was never released, we are actually in a situation where no one can have implemented a TLS provider against 2.4.0. That means we can still change the API from TcpStream to Stream as you are proposing (it's obviously the better choice!).

I've pushed two commits to your PR. One that fixes some variable naming (tcp_stream -> stream), and one that again removes the Sync trait bound for Stream.

[algesten]

@jsha Do you have time to look at this?

HTTPS proxy support means we need to have a TLS stream inside a TLS stream. That means changing our public API so that TlsConnector::connect works over Stream instead of TcpStream. That's technically a breaking change.

However. Because of #461 no one has so far been able to implement a TlsConnector using our released version, which means I'm ok with changing that API. On a side note, I think it's more elegant TlsConnector works over the more generic Stream. What do you think?

This does mean Stream becomes part of our public API, and I'm not sure it's the good way to go.

TlsConnector::connect trait fn requires a return of Box<dyn ReadWrite>, and it strikes me there's overlap between the Stream type and the ReadWrite trait. Why wouldn't we define the trait like this:

pub trait TlsConnector: Send + Sync {
    fn connect(
        &self,
        dns_name: &str,
        io: Box<dyn ReadWrite>,
    ) -> Result<Box<dyn ReadWrite>, crate::error::Error>;
}

I.e. keep Stream an internal concern, especially since Stream::inner has a Box anyway.

[algesten]

#510 explores changing the TlsConnector trait in a way that would make this PR much simpler.

[jsha]

I gave some feedback on #510, which seems like a good approach. Would you still like me to look at this implementation?

[algesten]

@jsha I agree #510 is where it's at, and if we land that, this becomes a trivial implementation.

[puffi]

Now that 2.5.0 is released and I got reminded about this pull request by a coworker, I'll work on this again.
Since lots of things have changed, would you recommend to close this one and open a new pull request?

[algesten]

@puffi

Whatever is easier for you. I think we got all the history we need for the change either way.

[puffi]

I'll open a new request once the code is ready.
But until then there's still one more question I have. Since the TlsConnector is part of the Agent and can only be used once, how can we know which TLS provider to use when creating the proxy connection? If we use the one from the agent, then it will fail in connect_https.
We at Proxmox use the native-tls feature, while the default one would be rustls (part of the default features). Then there's also the mbedtls.

[puffi]

@algesten

Or would it be an option to make the TlsConnector clonable to be able to get a new connector of the same provider? Currently that's not required as part of the trait, and as such not possible.

[algesten]

@puffi not sure i follow here. I don't see how it "can only be used once". An agent can (per definition) issue many requests with the same connector.

The TlsConnector is stored away internally as this (notice Clone).

#[derive(Clone)]
pub(crate) struct TlsConfig(Arc<dyn TlsConnector>);

And connect_https doesn't even use the Clone, but accesses the connector via a shared (not even mut) reference:

    let tls_conf = &unit.agent.config.tls_config;
    let https_stream = tls_conf.connect(hostname, Box::new(sock))?;

Seems like the TlsConnector is highly reusable as it is. What am I missing?

[puffi]

Sorry for the noise, it seems to work now.
Back on 2.4.0 when I tried to use the TlsConnector to connect to both the proxy and the target host it failed, but since it works now it should be fine.
Probably wrong usage of it back then.

I'll create the new pull request soon, thanks for the help!

[puffi]

Closing this since I've opened a new one (#544).