GCP: ClientOptions default_headers (Accept-Encoding) break OAuth token fetch

[nkemnitz]

Describe the bug

ClientOptions::client() applies user default_headers (src/client/mod.rs:843) and also sets .no_gzip()... (mod.rs:929). The GCP token providers reuse that same client (src/gcp/builder.rs:606-608). So a data-plane Accept-Encoding: gzip header leaks onto the OAuth token POST (src/gcp/credential.rs:613-637, accounts.google.com/o/oauth2/token); Google returns gzip, no_gzip skips decoding, and .json() fails → Error::TokenResponseBody. The client tells the server "send gzip" while telling reqwest "don't decode gzip".

To Reproduce

let mut h = http::HeaderMap::new();
h.insert("accept-encoding", "gzip".parse().unwrap());
let store = GoogleCloudStorageBuilder::from_env()
    .with_bucket_name("my-bucket")
    .with_client_options(ClientOptions::new().with_default_headers(h))
    .build()?;
store.get(&"x".into()).await?; // authorized_user ADC: token fetch fails before the GET

Error: Generic { store: "GCS", source: TokenResponseBody(... "expected value at line 1 column 1") }.
(Accept-Encoding: gzip is legitimate: it makes GCS serve Content-Encoding: gzip objects as raw gzip.)

Expected behavior

Setting default_headers for the data plane should not break authentication.

Additional context

[ByteBaker]

I'd argue that this is not wrong. As ClientOptions has no awareness of the impact caused by headers passed by the caller. As such, the string "gzip" does not meaning anything to the client, it's just a string, and hence it won't try and derive meanings from the payload of default headers, even if one of those is "gzip".

However, the right way IMO would be to track encoding inside ClientOptions, and turn encoding on/off inside the client builder accordingly. Since the culprit line builder = builder.no_gzip().no_brotli().no_zstd().no_deflate(); turns off all forms of encoding.

@tustvold @alamb thoughts?

[ByteBaker]

Upon looking further, it turns out the surface area is far bigger than anticipated. reqwest defines the methods ClientBuilder::no_{gzip|brotli|zstd|deflate() like the following.

    pub fn no_gzip(self) -> ClientBuilder {
        #[cfg(feature = "gzip")]
        {
            self.gzip(false)
        }

        #[cfg(not(feature = "gzip"))]
        {
            self
        }
    }

This means that the said feature must be enabled for reqwest to process the given encoding.

However, object_store does not yet provide a way to opt into those reqwest-specific features when using object_store as a dependency. To ensure that a user externally enabling those features in reqwest via a parallel dependency does not change reqwest's behavior, it chooses to opt-out of all those algorithms. This is declared in the comment preceding the code:

    // Explicitly disable compression, since it may be automatically enabled
    // when certain reqwest features are enabled. Compression interferes
    // with the `Content-Length` header, which is used to determine the
    // size of objects.
    builder = builder.no_gzip().no_brotli().no_zstd().no_deflate();

Having said that, this is clear that object-store intentionally disables all forms of response compression. There's 3 ways forward to this:

1. Explicitly declare in client builders' docs that the user must avoid adding an Accept-Encoding header since object store doesn't support it. @nkemnitz's concern is valid, but the contract between the headers and compression features is not enforced by them. (Easiest, but errors can happen if users don't read it)
2. Remove Accept-Encoding header while storing default headers for the client and declare the behaviour in docs. This still stays with no compression, but the users know that including this header in default headers has no effect, and the client still requests content w/o compression. (Easy, ensures errors don't happen)
3. Object store adds feature flags that optionally enable the said features in reqwest. Along with exploring and handling the Content-Length problem mentioned in the comment. (Complex, complete solution to the problem)

Please suggest a way forward.

[nkemnitz]

To be clear, I need to send Accept-Encoding: gzip headers on the actual requests to GCS, otherwise Google will assume my client is unable to handle the gzip compressed objects, decode and serve them via decompressive transcoding instead. That's potentially slow and expensive (uncompressed transfer), and worse: it strips the Content-Length header, which breaks another object-stores assumption - see #774

[ByteBaker]

@nkemnitz I went through #774, and that seems to be linked to this. Unfortunately 1 & 2 as suggested above won't do you any good. We'll have to see what the team thinks.

[alamb]

The latest object store release allows you to use your own client (rather than the built in reqwest) -- maybe that mechanism can be used to configure the headers ?