Client certificates seem to be ignored when using rustls #903

alexhexabeam · 2020-05-06T00:22:13Z

#[derive(Serialize, Deserialize)]
struct TokenResponse {
    #[serde(rename = "accessToken")]
    access_token: String,
}

async fn acquire_token(url: &str) -> Result<String, MyError> {
    let cert_bytes = fs::read("combined.pem")?;
    let id = reqwest::Identity::from_pem(&cert_bytes)?;

    let ca = reqwest::Certificate::from_pem(&fs::read("ca.pem")?)?;
    let client = reqwest::ClientBuilder::new()
        .danger_accept_invalid_certs(true)
        .add_root_certificate(ca)
        .identity(id)
        .timeout(Duration::new(10, 0))
        .build()?;
    let jwt_token_response = client.post(url)
        .header("Accept", "application/json")
        .send().await?.error_for_status()?.json::<TokenResponse>().await?;

    Ok(jwt_token_response.access_token)
}

The above code errors with a 401 when it gets to the .error_for_status()?.

Below is combined.pem in this example (just a test cert, don't worry):

Note that the certificates themselves are correct, as I used them to generate the identity.p12 file in this working native-tls example:

async fn acquire_token(url: &str) -> Result<String, MyError> {
    let p12_bytes = fs::read("identity.p12")?;
    let id = reqwest::Identity::from_pkcs12_der(&p12_bytes, "password")?;

    let ca = reqwest::Certificate::from_pem(&fs::read("ca.pem")?)?;
    let client = reqwest::ClientBuilder::new()
        .danger_accept_invalid_hostnames(true)
        .add_root_certificate(ca)
        .identity(id)
        .timeout(Duration::new(10, 0))
        .build()?;
    let jwt_token_response = client.post(url)
        .header("Accept", "application/json")
        .send().await?.error_for_status()?.json::<TokenResponse>().await?;

    Ok(jwt_token_response.access_token)
}

The text was updated successfully, but these errors were encountered:

kyasu1 · 2020-05-15T01:29:22Z

Hi

I have been having similar issue when using rustls and I could resolve it by adding use_rustls_tls() option to the ClientBuilder as stated here.

https://docs.rs/reqwest/0.10.4/reqwest/struct.ClientBuilder.html#method.use_rustls_tls

Hope this may help

edevil · 2020-06-29T17:21:08Z

Do you know if it is possible to specify the whole accepted list of CAs, instead of just adding a new one, and if it is possible to specify the expected servername per TLS request instead of using danger_accept_invalid_hostnames?

lostiniceland · 2022-03-19T17:05:09Z

Hi

I have been having similar issue when using rustls and I could resolve it by adding use_rustls_tls() option to the ClientBuilder as stated here.

https://docs.rs/reqwest/0.10.4/reqwest/struct.ClientBuilder.html#method.use_rustls_tls

Hope this may help

Thanks. I spent hours trying to get my client-cert working and just did not see this option. I thought that rustls is automatically active when activating the feature (since event the API changes, eg from_der -> from_pem)

eric-seppanen · 2023-05-24T22:05:56Z

I encountered a similar situation. I had an end-to-end unit test that created a server and a client and verified that client certificate auth works. The client was constructed like this:

let client = ClientBuilder::new()
    .resolve("test_server", server_addr)
    .identity(client_identity())
    .add_root_certificate(root_cert)
    .build()
    .unwrap();

My unit test was passing until I merged this crate into a workspace with some other crates, and then it started to fail.

What I found is that if reqwest is specified with default-features = false and features = ["rustls-tls"], then my unit test passes. If I remove default-features = false (or build in a workspace where another crate enables default features) then the test fails.

This is quite surprising, that enabling additional features can cause working code to fail.

I think what's happening is that I created an Identity using Identity::from_pem() (which requires the rustls feature), but that identity gets silently ignored if the Client defaults into a native-tls configuration.

Adding .use_rustls_tls() fixes my test failure.

Assuming I understand the problem correctly, it would be helpful if this caused an error instead of silently doing the wrong thing. If there's an incompatibility between the Identity variant and the Client backend config, perhaps that should cause ClientBuilder::build to fail instead of silently doing the wrong thing?

seanmonstar · 2023-05-24T22:10:56Z

Thanks for investigating! If that's indeed what the code is doing, I agree it should be fixed! Would you like to take a shot at that?

eric-seppanen · 2023-05-24T23:14:32Z

Is the best fix to just add a failure to ClientBuilder::build? If yes, than I could give it a shot.

I'm not sure if there are other options, but I might wish for one of these instead:

.identity() could be backend-agnostic, so anything that compiles should work as expected. I'm not sure if this is possible.
ClientBuilder could automatically switch backends based on the Identity that was passed. Maybe that's a bit too subtle, though? It would be strange

Even if a better solution exists down the road, perhaps it's still a good first step to add the short-term fix (fail build if the backend and identity are incompatible).

seanmonstar · 2023-05-25T01:10:54Z

Yea, returning a builder error seems better than silently not working. If the Identity type can parse it into both formats, even better. I don't know if it can.

cpu · 2023-08-16T13:17:29Z

Just chiming in to say this bubbled up as an issue on the Rustls repo (rustls/rustls#1297). The produced behaviour is confusing from a user perspective and takes some time to diagnose from first principles.

Here are some more detailed reproduction steps for reqwest 0.11.18 that show both the client and server behaviour in case it's helpful:

Client reqwest program

use std::time::Duration;

#[tokio::main]
async fn main() -> Result<(), reqwest::Error> {
    let buf = include_bytes!("../minica.pem").as_slice();
    let cacert = reqwest::Certificate::from_pem(&buf)?;

    let client_ident_pem = include_bytes!("../combined.pem").as_slice();
    let id = reqwest::Identity::from_pem(&client_ident_pem)?;

    let client = reqwest::ClientBuilder::new()
        .timeout(Duration::from_secs(30))
        .add_root_certificate(cacert)
        .identity(id)
        .build()?;

    client.get("https://localhost:4433/").send().await?;

    Ok(())
}

Server setup using Rustls tlsserver-mio example program:

SSLKEYLOGFILE=/tmp/tlsmioserver.key.txt cargo run --bin tlsserver-mio -- --certs ../hypertest/cert.pem --key ../hypertest/key.pem --require-auth --auth ../hypertest/minica.pem --verbose --port 4433 http

Tshark packet capture showing empty client `Certificate` handshake message:

sudo tshark -i any -n -V -o 'tls.keylog_file:/tmp/tlsmioserver.key.txt' 'port 4433' 
<snipped>
Transport Layer Security
    TLSv1.3 Record Layer: Change Cipher Spec Protocol: Change Cipher Spec
        Content Type: Change Cipher Spec (20)
        Version: TLS 1.2 (0x0303)
        Length: 1
        Change Cipher Spec Message
    TLSv1.3 Record Layer: Handshake Protocol: Certificate
        Opaque Type: Application Data (23)
        Version: TLS 1.2 (0x0303)
        Length: 25
        [Content Type: Handshake (22)]
        Handshake Protocol: Certificate
            Handshake Type: Certificate (11)
            Length: 4
            Certificate Request Context Length: 0
            Certificates Length: 0
    TLSv1.3 Record Layer: Handshake Protocol: Finished
        Opaque Type: Application Data (23)
        Version: TLS 1.2 (0x0303)
        Length: 69
        [Content Type: Handshake (22)]
        Handshake Protocol: Finished
            Handshake Type: Finished (20)
            Length: 48
            Verify Data

Notably the problematic configuration does send a Certificate handshake message in response to the server's Certificate Request, it's just empty:

        Handshake Protocol: Certificate
            Handshake Type: Certificate (11)
            Length: 4
            Certificate Request Context Length: 0
            Certificates Length: 0

As described above adding use_rustls_tls() to the builder, or disabling default features and only enabling features = ["rustls-tls"] fixes the issue.

I've also applied #1852 and confirmed it produces a helpful error message instead of sending an empty client Certificate message.

eric-seppanen · 2023-08-16T18:08:24Z

I think the root of the problem is that Identity commits to a particular backend, then later ClientBuilder may commit to a different backend, and then try to load that Identity, and we have a problem.

The problem only arises when multiple backends are enabled, so we could create some logic that extracts the key + certificate from the backend-A Identity and transforms it into a backend-B Identity. This seems unsatisfying; it would be nice to find a way so that the correct type is always generated and we don't have this problem. Also, there's no guarantee that the two backends support all the same cryptographic capabilities so it's possible the transformation could fail.

Another idea would be to delay parsing the key and certificate until the ClientBuilder is consuming them. In this world, Identity would only be a container for the raw key and certificate bytes, and attempting to insert them into a ClientBuilder would run that backend's parsing logic. This might mean that reqwest has to carry around extra code for managing a few different file formats (DER, PEM, PEM with multiple objects) that could otherwise have been handled by the backend. But it might be worth exploring to see how burdensome it would be.

In another universe, ClientBuilder could carry generic parameters so that ClientBuilder<RustlsFlavor> would only accept RustlsIdentity, which would make all of this a compile-time error. I'm assuming that's not in the cards, but I still wish there was a way to get to compile-time errors instead of runtime errors.

Another way to look at the problem: the naming of the current Identity constructors invites mistakes. For example, Identity::from_pem creates an Identity that can only work with a rustls ClientBuilder, but nothing about that name indicates to the caller that this is happening. To make this less surprising, either an Identity should be fully backend-agnostic, or the constructor name (and type?) should proclaim its restrictions.

eric-seppanen mentioned this issue May 25, 2023

fail Client build if Identity + default backend + !cfg(native-tls) #1852

Merged

cpu mentioned this issue Aug 16, 2023

Connection reset by peer rustls/rustls#1297

Closed

seanmonstar closed this as completed in #1852 Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client certificates seem to be ignored when using rustls #903

Client certificates seem to be ignored when using rustls #903

alexhexabeam commented May 6, 2020

kyasu1 commented May 15, 2020

edevil commented Jun 29, 2020

lostiniceland commented Mar 19, 2022

eric-seppanen commented May 24, 2023

seanmonstar commented May 24, 2023

eric-seppanen commented May 24, 2023

seanmonstar commented May 25, 2023

cpu commented Aug 16, 2023 •

edited

eric-seppanen commented Aug 16, 2023 •

edited

Client certificates seem to be ignored when using rustls #903

Client certificates seem to be ignored when using rustls #903

Comments

alexhexabeam commented May 6, 2020

kyasu1 commented May 15, 2020

edevil commented Jun 29, 2020

lostiniceland commented Mar 19, 2022

eric-seppanen commented May 24, 2023

seanmonstar commented May 24, 2023

eric-seppanen commented May 24, 2023

seanmonstar commented May 25, 2023

cpu commented Aug 16, 2023 • edited

eric-seppanen commented Aug 16, 2023 • edited

cpu commented Aug 16, 2023 •

edited

eric-seppanen commented Aug 16, 2023 •

edited