Fix/s3v4 signer and oauth2 token#3504
Conversation
… invalid Bearer token Two bugs affecting REST catalog S3 remote signing with FsspecFileIO: 1. LegacyOAuth2AuthManager.auth_header() returned the string "Bearer None" when no credential or initial_token was configured, instead of returning None. This caused S3V4RestSigner to forward an invalid Authorization header to the catalog signer endpoint, resulting in a 403 that propagated as SignError or was swallowed, leaving the S3 request unsigned. Fix: return None when self._token is None. 2. S3V4RestSigner was registered on the before-sign.s3 botocore event, but botocore short-circuits RequestSigner.sign() and returns early (without emitting before-sign) when signature_version=UNSIGNED is set. Since UNSIGNED is set alongside the signer to prevent botocore's own SigV4 from running, the S3V4RestSigner.__call__ was never invoked, and every S3 request went to the server completely unsigned, resulting in AccessDenied. Fix: register S3V4RestSigner on before-send.s3 instead of before-sign.s3. before-send fires unconditionally after signing (or after UNSIGNED's early return), giving the signer a chance to add catalog-signed headers before the HTTP request is dispatched. Update header assignment from add_header() to dict-style assignment, which is compatible with both AWSRequest (before-sign) and AWSPreparedRequest (before-send). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fix/s3v4-signer-and-oauth2-token
auth_header() makes no HTTP requests, so checking request_history against an unattached mock was both incorrect and unreachable. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@abhishekkumar2024 thanks for your contribution! Could you please update the PR description so reviewers can understand the motivation? |
|
+1 on @ebyhr's comment. I'm not sure what this PR is trying to accomplish. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that's incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
3 participants
Rationale for this change
Are these changes tested?
Are there any user-facing changes?