algesten / ureq

A simple, safe HTTP client
Apache License 2.0
1.72k stars 177 forks source link

Add support for rustls with mbedtls #791

Closed kostko closed 2 months ago

kostko commented 2 months ago

The current ureq only supports rustls with ring which is not available (e.g. build fails) on some targets. This PR adds support for using mbedtls as a cryptography backend by splitting rustls support into two features, rustls-ring (default) and rustls-mbedtls. The existing rustls feature has been renamed to _rustls.

algesten commented 2 months ago

Hi @kostko, thanks for this!

This PR is a great starting point for a discussion about what should be in ureq. Here's some context I wrote recently https://github.com/algesten/ureq/issues/765#issuecomment-2284176962

Ureq's first goal is to Just Work. I don't want a beginner to Rust trying ureq to have to think about TLS provider. We recently landed a PR to respect the user's choice of rustls crypto #773 - but judging by this PR, that is not enough to fully use mbedtls.

I'm pretty much alone in maintaining ureq, thus I'm a bit hesitant about accepting RPs that expands the functionality that I need to maintain. One hope for ureq 3.x is that the Connector/Transport mechanism would allow creating separate crates that could for instance provide other TLS providers.

The mbedtls crate got a few downloads on crates.io, but it doesn't look like a major TLS provider in Rust, is that right? What's the argument for having mbedtls in ureq tree vs providing it as a separate crate?

kostko commented 2 months ago

The Connector/Transport mechanism might actually be sufficient (albeit with some code duplication), the main problem is the ring dependency which fails to build in some platforms (e.g. I'm using ureq together with the Fortanix SGX target where ring does not compile).

mcr commented 2 months ago

Martin Algesten @.***> wrote:

I'm pretty much alone in maintaining ureq, thus I'm a bit hesitant about accepting RPs that expands the functionality that I need to maintain. One hope for ureq 3.x is that the Connector/Transport mechanism would allow creating separate crates that could for instance provide other TLS providers.

As someone who struggled to make mbedtls work with ureq before, I think that is likely that the Connector/Transport mechanism ought to work, and I'm working on moving to ureq 3.x.

> The mbedtls crate got a few downloads on crates.io, but it doesn't look
> like a major TLS provider in Rust, is that right? What's the argument
> for having mbedtls in ureq tree vs providing it as a separate crate?

mbedtls, aka psa-crypto, is often the TLS of choice for embedded systems (RIOT-OS, Zephyr, ...). So the expense of the (C) code is already paid for. [WolfSSL is the other major choice]

I don't understand having mbedtls in ureq tree though.

kostko commented 2 months ago

You're right, the connector mechanism should be fine, I'll close this.

algesten commented 2 months ago

The Connector/Transport mechanism might actually be sufficient (albeit with some code duplication), the main problem is the ring dependency which fails to build in some platforms (e.g. I'm using ureq together with the Fortanix SGX target where ring does not compile).

I'd be very interested in hearing how this works out. Since ureq 3.x is unreleased and all is still up in the air, I'd very much enjoy feedback and suggestions on how to improve the external transport API.

mbedtls, aka psa-crypto, is often the TLS of choice for embedded systems (RIOT-OS, Zephyr, ...). So the expense of the (C) code is already paid for. [WolfSSL is the other major choice] I don't understand having mbedtls in ureq tree though.

Ah, makes sense. My own journeys into embedded Rust hasn't taken me there yet. Thanks for the context!

kostko commented 2 months ago

I'd be very interested in hearing how this works out. Since ureq 3.x is unreleased and all is still up in the air, I'd very much enjoy feedback and suggestions on how to improve the external transport API.

Seems to work, see linked PR above.

kostko commented 2 months ago

Actually one thing that didn't work is the timeout field on the TransportAdapter that is not exposed (and there is no setter). The in-tree TLS implementation mutates that directly.

algesten commented 2 months ago

Good catch! Let's fix it!