[I] Define wire request semantics across all signers [opendal-reqsign]

["Xuanwo (via GitHub)" <gi...@apache.org>]

Xuanwo opened a new issue, #784:
URL: https://github.com/apache/opendal-reqsign/issues/784

   ## Description
   
   reqsign currently uses `SigningRequest` as three different representations at once:
   
   - the URI that will be sent on the wire;
   - a decoded logical view of path and query parameters;
   - the canonical representation used to calculate a signature.
   
   `SigningRequest::build` parses an existing raw query with `form_urlencoded::parse`, while `SigningRequest::apply` rebuilds the outgoing URI from the decoded pairs. This can change percent encoding, parameter boundaries and order, `+`, repeated keys, and distinctions such as `flag` versus `flag=`. Canonical sorting or encoding can also leak into the wire request. Because `build` moves the URI and headers out of the caller's request, an error can leave the request empty or partially modified.
   
   #778 reports the raw-query corruption, and #782 adds useful cross-service coverage while attempting to preserve an unchanged decoded prefix. However, a decoded-prefix comparison cannot represent every raw-query distinction and still leaves canonicalization coupled to wire serialization. apache/opendal#7926 and apache/opendal#7927 also show the inverse problem: callers can accidentally depend on reqsign rebuilding a URI during signing.
   
   We should define one common request-signing contract and adapt every built-in signer to it, without changing caller-facing method signatures.
   
   ## Proposed contract
   
   ### Input
   
   - `http::request::Parts::uri` is a valid, fully percent-encoded, wire-ready URI.
   - Existing path and query representation, including ordering and encoding, is semantically significant.
   - Callers encode their business path and query. A signer only encodes authentication fields that it adds.
   - Method, HTTP version, and extensions are caller-owned and read-only for signing.
   
   ### Success
   
   - Header authentication leaves the URI unchanged and only adds or replaces protocol-owned headers.
   - Query authentication preserves the existing raw path/query and appends protocol-encoded authentication fragments.
   - Canonical path, query, and headers are read-only service-local views. Sorting, decoding, encoding, trimming, or lowercasing for canonicalization never rewrites the wire request.
   - Only URI and headers are signer-owned output fields. Method, version, and extensions remain unchanged.
   
   ### Failure
   
   If signing returns an error, the entire caller request head remains unchanged. Signer-internal state such as a credential cache is outside this request atomicity guarantee.
   
   `expires_in` is a service input, not a generic header-versus-query mode switch. For example, an Azure SAS credential can append a query fragment when `expires_in` is `None`.
   
   ## Implementation direction
   
   Keep the existing `Signer::sign`, `SignRequest::sign_request`, and `SigningRequest` calls and public field shape.
   
   At the core boundary, `Signer::sign` should clone `Parts` into a candidate, invoke the service signer on that candidate, and only commit `candidate.uri` and `candidate.headers` after `Ok(())`. This enforces failure atomicity and prevents service signers from changing caller-owned fields.
   
   Each built-in signer should then use local values instead of mutating the wire URI during canonicalization:
   
   - Header signing adds required headers locally, canonicalizes the candidate, computes the signature, and adds the authorization header without constructing a new URI.
   - Query signing appends encoded pre-sign authentication fields to the original raw URI, canonicalizes that unsigned candidate, computes the signature, and finally appends the signature carrier.
   - Credential-provided fragments such as Azure SAS are validated first and then appended as already encoded fragments.
   
   `SigningRequest::build` should clone URI and headers instead of using `mem::take`. `SigningRequest::apply` should only commit headers and must never rebuild method or URI. If its method/scheme/authority/path/query working view was changed, `apply` should return an explicit error before modifying the request. Query-signing implementations must construct their final URI directly from the original raw `Parts::uri`.
   
   This does not require a large transaction abstraction, a raw-query side channel, or a decoded-prefix heuristic.
   
   The audit should cover all current implementations:
   
   - AWS V4
   - Google
   - Azure Storage
   - Aliyun OSS
   - Huawei OBS
   - Oracle
   - Tencent COS
   - Volcengine TOS
   
   ## Compatibility
   
   Ordinary reqsign callers, including OpenDAL, keep the same invocation:
   
   ```rust
   let (mut parts, body) = req.into_parts();
   signer.sign(&mut parts, expires_in).await?;
   Request::from_parts(parts, body)
   ```
   
   The observable behavior intentionally changes: reqsign no longer silently decodes, sorts, or re-encodes an existing URI. Third-party `SignRequest` implementations that use `SigningRequest::query_*` plus `apply` to generate authentication query parameters need an internal implementation update, but their trait signature and callers remain unchanged.
   
   OpenDAL remains responsible for constructing a wire-ready URI. apache/opendal#7927 is still required because it fixes duplicate encoding before reqsign is called.
   
   ## Acceptance criteria
   
   - Core tests prove that a signer which mutates its candidate and then fails leaves the original `Parts` unchanged.
   - Success tests prove that only URI and headers are committed.
   - Every built-in signer asserts both the canonical representation and the final wire URI.
   - Header-signing tests assert byte-for-byte URI identity.
   - Query-signing tests assert that the original raw query is preserved and authentication fields are only appended.
   - Coverage includes `%2F`, `%23`, `%26`, `%3D`, `%20`, `%2B`, bare `+`, `%252F`, repeated keys, empty keys/values, `flag`, `flag=`, and a trailing `&`.
   - Oracle `(request-target)` coverage includes the original query and ordering.
   - Google and Azure bearer-token paths do not modify the URI.
   - Error injection at every fallible stage preserves the complete input request head.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] Define wire request semantics across all signers [opendal-reqsign]

["Xuanwo (via GitHub)" <gi...@apache.org>]

Xuanwo closed issue #784: Define wire request semantics across all signers
URL: https://github.com/apache/opendal-reqsign/issues/784


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org