FlorianUekermann / rustls-acme

Apache License 2.0
144 stars 28 forks source link

When reporting an HTTP error, include the response body #23

Closed joshtriplett closed 2 years ago

joshtriplett commented 2 years ago

ACME implementations such as Let's Encrypt provide useful error messages in the body. Provide the body as part of the error, to make it feasible for users to debug.

In theory, the error response should almost always be valid JSON, but this just treats the response body as bytes, to avoid masking an HTTP error with a parsing error.

I spent a while trying to debug an opaque error, and this change helped a great deal.

(The error formatting isn't ideal, but it's a substantial improvement over not having the information at all.)

FlorianUekermann commented 2 years ago

Yes, this makes sense. I checked the RFC and returning an error document spec is a SHOULD, not a MUST But I must admit that String::from_utf8_lossy doesn't feel great. Maybe the conversion to string should happen at display time to make the original response recoverable.

The only way I see without dropping derive(Error, ... on the enum is wrapping the error once and implementing a custom Debug for the wrapper.

#[derive(Error, Debug)]
pub enum HttpsRequestError {
    // ... snip
    #[error("non 2xx http status: {0:?}")]
    Non2xxStatus(Non2xxStatusError),
    // ... snip
}

struct Non2xxStatusError{
    status_code: u16,
    body: Vec<u8>,
}

impl Debug for Non2xxStatusError

That would also have the benefit of taking a step towards removing stuff from the http-types crate from the public interface (see #19), which I'm planning to do before the non-beta release.

joshtriplett commented 2 years ago

@FlorianUekermann Yeah, we can use a custom type. But also, I just realized that it is safe to assume UTF-8: the ACME RFC says "All requests and responses sent via HTTP by ACME clients, ACME servers, and validation servers as well as any inputs for digest computations MUST be encoded using the UTF-8 character set". So this can use String::from_utf8_lossy and not worry about losing any information.

joshtriplett commented 2 years ago

@FlorianUekermann I've updated the code to just use body_string(), since the ACME standard requires bodies to use UTF-8. I've also switched to u16 for the status code.

joshtriplett commented 2 years ago

That would also have the benefit of taking a step towards removing stuff from the http-types crate from the public interface

I submitted https://github.com/FlorianUekermann/rustls-acme/pull/25 to help further with that.