feat(rest): add SigV4 auth manager for the REST catalog - #2660
feat(rest): add SigV4 auth manager for the REST catalog#2660plusplusjiajia wants to merge 8 commits into
Conversation
|
There's a PR open for SigV4 signing, is this picking up from that one? #2311 I ask as its had a few rounds of feedback already. |
@dannycjones Thanks for the pointer — I'd missed #2311, just took a look and compared the two. Mine isn't based on it: it follows Iceberg Java's RESTSigV4AuthSessionRESTSigV4AuthSession(apache/iceberg#11995) and the merged iceberg-cpp version(apache/iceberg-cpp#616). The main difference I see is the base64-encoded x-amz-content-sha256 convention (the Java behavior), which #2311's hex-only signing doesn't cover and which some REST servers require. |
|
I haven't thought quite clearly about this part yet, my general intuition is that it would be better to start from something like a Would be happy to hear more thoughts on this |
| } | ||
|
|
||
| /// Injects a custom request signer, overriding the `rest.sigv4-*` configuration. | ||
| pub fn with_signer(mut self, signer: Arc<dyn HttpRequestSigner>) -> Self { |
There was a problem hiding this comment.
I think we need a more general design rather than just a signer. some authentication mechanism is token-based.
There was a problem hiding this comment.
@CTTY I dug into how Java structures this, and I think your point is well taken: OAuth2 token handling is hardcoded inside HttpClient today, and this PR adds a second, parallel mechanism that is mutually exclusive with token auth. In Java the two compose — SigV4AuthManager wraps a delegate session, relocates its Authorization header to X-Iceberg-Authorization, then signs — so SigV4-over-OAuth2 is a real combination the current design can't express.
Is something along these lines what you had in mind, mirroring Java's AuthManager/AuthSession?
#[async_trait]
pub trait AuthManager: Debug + Send + Sync {
/// Session used for catalog-level requests.
async fn catalog_session(
&self,
props: &HashMap<String, String>,
) -> Result<Arc<dyn AuthSession>>;
// room to grow, matching Java: init_session() for the config
// handshake, table_session() for table-scoped auth, close().
}
#[async_trait]
pub trait AuthSession: Debug + Send + Sync {
/// Applies authentication to an outgoing request (headers, signing, ...).
async fn authenticate(&self, request: &mut reqwest::Request) -> Result<()>;
}with NoopAuthManager / OAuth2Manager (existing logic extracted, behavior unchanged) / SigV4AuthManager (wrapping a delegate, Java-style) as the initial implementations, selected via rest.auth.type or injected through the builder.
If that matches your intuition, my instinct would be to land the interface plus the OAuth2 extraction as a small standalone refactor first, then rework this PR on top as the SigV4 implementation — which would also give #2311 a common landing spot.
There was a problem hiding this comment.
Prototype is up as a draft PR: #2815 — full AuthManager/AuthSession shape with Noop/OAuth2/SigV4 managers; details and known simplifications in the PR description.
| IcebergRest, | ||
| /// Standard AWS SigV4 style: hex everywhere (e.g. AWS Glue). | ||
| StandardAws, | ||
| } |
There was a problem hiding this comment.
I didn't know this detail until this PR. Thanks for capturing this!
f79d5dd to
a4c4d8e
Compare
- hold the token lock across the OAuth2 exchange (single flight) - three-state AuthRequestBody: Empty/Buffered/Streaming; pub AuthRequest::new - zeroize OAuth2 secrets via a redacting SensitiveString - split the OAuth2 session into static-token and client-credentials types - init_session returns Box<dyn AuthSession>; catalog_session stays Arc
- unset rest.auth.type resolves to oauth2 only when a token, credential or oauth2-server-uri is configured, none otherwise - drop RestCatalog::invalidate_token/regenerate_token and AuthSession::invalidate/refresh (out-of-box refresh tracked separately) - hint at with_auth_manager for unknown auth types
Replaces the crate-private SensitiveString with the equivalent iceberg::Credential introduced in apache#2836.
a4c4d8e to
d141c70
Compare
plusplusjiajia
changed the title
3 participants
Stacked on #2838 — the first 7 commits are from that PR; only the last commit is this PR. Will be rebased once it merges.
Which issue does this PR close?
What changes are included in this PR?
SigV4 as a
SigV4AuthManagerwrapping a delegate auth manager, per #2838's design:Authorizationis relocated toOriginal-Authorization(Java convention) and included in the signature, then the request is SigV4-signed.SigV4Signerfollows AWS canonical-request rules with Java parity (path normalization + double encoding, all-headers-minus-blacklist,IcebergRest/StandardAwspayload-hash modes).AwsProperties(rest.signing-region,rest.signing-namedefaultexecute-api,rest.access-key-id/rest.secret-access-key/rest.session-token, single-source credential resolution). Delegate defaults tooauth2; an injected signer is never rebuilt from server properties; signed catalogsdon't follow redirects.