feat(catalog-rest): introduce AuthManager/AuthSession, rework SigV4 as a wrapping auth manager#2815
feat(catalog-rest): introduce AuthManager/AuthSession, rework SigV4 as a wrapping auth manager#2815plusplusjiajia wants to merge 1 commit into
Conversation
e58189f to
9b56016
Compare
9b56016 to
e1ba91d
Compare
e1ba91d to
a97a247
Compare
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Hey @plusplusjiajia , thanks for this contribution! I really like the general direction. To move forward, could you break this down to several smaller PRs? For example you can break it down to 2-3 PRs:
- Introduce AuthManager/Session traits and migrate existing oauth2 to use the new traits
- Add Sigv4 support
|
|
||
| /// How the payload hash is encoded in the `x-amz-content-sha256` header. | ||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub enum PayloadHashMode { |
There was a problem hiding this comment.
these should live in sigv4.rs?
There was a problem hiding this comment.
@CTTY Agreed — it's SigV4-specific. In the split, the signer and the manager will live together under auth/sigv4, so PayloadHashMode moves there (part of the SigV4 PR).
| warehouse: None, | ||
| props: HashMap::new(), | ||
| client: None, | ||
| custom_auth_manager: None, |
There was a problem hiding this comment.
let's just name it auth_manager
There was a problem hiding this comment.
@CTTY Will rename to auth_manager in the split.
| pub(crate) async fn token(&self) -> Option<String> { | ||
| let mut req = self | ||
| .request(Method::GET, &self.token_endpoint) | ||
| .request(Method::GET, "http://localhost/unused") |
There was a problem hiding this comment.
@CTTY It's a test-only shim: HttpClient no longer holds the token itself, so the #[cfg(test)] token() helper builds a throwaway request (never sent — authenticate only mutates headers) and reads the bearer back from the Authorization header. I'll replace it in the split with assertions on the headers the mock server observes.
@CTTY Thanks for the review! Agreed — I'll split this into:
|
| &self, | ||
| props: &HashMap<String, String>, | ||
| ) -> Result<Arc<dyn AuthSession>>; | ||
| } |
There was a problem hiding this comment.
Could we add other methods like fn contextual_session() and fn table_session() too?
We'll need to extend the trait eventually, and since it's public it would otherwise become a breaking change.
Both of the methods will take a parent AuthSession and can get a default implementation like
fn table_session<'a>(&self, _: &TableIdent, parent: &'a dyn AuthSession) -> Result<&'a dyn AuthSession> {
Ok(parent)
}There was a problem hiding this comment.
Also note that the fn contextual_session() would take something like a SessionContext which I'm only introducing in #2836. I can split it into its own PR so we can get it in faster.
fn contextual_session<'a>(&self, _: &SessionContext, parent: &'a dyn AuthSession) -> Result<&'a dyn AuthSession> {
Ok(parent)
}There was a problem hiding this comment.
I just realized that we can still extend the trait later with methods like this that have a default implementation... never mind my above comment 🤦
There was a problem hiding this comment.
I'm happy to extend it as a follow-up because the fn contextual_session() will be relevant for my goal to ship #2774 🙂
| #[async_trait] | ||
| pub trait AuthSession: Debug + Send + Sync { | ||
| /// Applies authentication to the request (adds headers, signs, ...). | ||
| async fn authenticate(&self, request: &mut Request) -> Result<()>; |
There was a problem hiding this comment.
IMO it would help to introduce a small wrapper around reqwest::Request to not expose the HTTP library we're using (internally) as part of public traits
| /// The auth manager living for the lifetime of the catalog. | ||
| auth_manager: Arc<dyn AuthManager>, | ||
| /// The session authenticating requests in the current phase. | ||
| session: Arc<dyn AuthSession>, |
There was a problem hiding this comment.
I'm thinking that the AuthManager/ AuthSession might be better placed in the RestCatalog.
In this current implementation both are working fine, but if we add table_session() and contextual_session() to the mix, the AuthManager will have to translate TableIdents and SessionContexts to AuthSessions.
IMO this is not the layer of abstraction that the HttpClient should operate on. In my mind, an AuthManager manages the creation of AuthSessions which the client can then use to authenticate its requests. The client should therefore not itself have to deal with creating those sessions.
3 participants
Prototype for the
AuthManagerdirection discussed in the review of #2660 (see the inline thread there). This draft includes #2660's SigV4 commits squashed in.What it does
Mirrors Java's AuthManager API:
AuthManager/AuthSessiontraits in a newauth/module:init_session()serves theGET /v1/confighandshake,catalog_session(merged_props)serves everything after, so a manager can rebuild itssession from server-merged properties.
Noop/OAuth2/SigV4managers, selected via a newrest.auth.typeproperty (the legacyrest.sigv4-enabledstill works), injectable throughRestCatalogBuilder::with_auth_manager.HttpClientintoOAuth2Manager, with the cached token surviving the config handshake.SigV4Managerwraps a delegate session instead of being mutually exclusive with token auth: the delegate'sAuthorizationheader is relocated toX-Iceberg-Authorizationbefore signing, so it is covered bythe signature — matching the Java
RESTSigV4AuthSessionbehavior.rest.signing-*from the config response is honored when building the post-handshake session;rest.auth.sigv4.delegate-auth-type(none/oauth2) explicitly selects the wrapped auth,defaulting to inference from token/credential presence;
OAuth2Manageris publicly constructible (new()+with_*).Java references:
org.apache.iceberg.rest.auth(AuthManager/AuthSession) andRESTSigV4AuthSession.Known prototype simplifications