Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The remaining comments are minor documentation nits with no blocking issues.
Pull request overview
Adds presigned-urls support and signing-mode selection to the REST Catalog OpenAPI specification.
Changes:
- Adds
presigned-urlstodata-access. - Documents
signRequestsigning-mode selection. - Adds handling for unsupported signing modes.
File summaries
| File | Summary |
|---|---|
open-api/rest-catalog-open-api.yaml |
Defines the new delegation value and signing behavior. |
Review details
Suppressed comments (2)
open-api/rest-catalog-open-api.yaml:1416
- This operation now allows a presigned-URL response, but the referenced
RemoteSignResponsecomponent is still described as containing “signed & unsigned headers” and does not describe the URI carrying the presigned URL. That leaves the generated 200-response documentation contradictory for this new mode; update the reusable response description (or introduce a mode-specific response schema) to cover both forms.
`RemoteSignResult` takes one of two forms. With `remote-signing`, `uri` is the
requested URI and `headers` carries the signature. With `presigned-urls`, `uri`
carries the signature and `headers` is empty.
open-api/rest-catalog-open-api.yaml:1449
UnsupportedOperationResponseis documented as “The server does not support this operation,” but this new 406 is for a supportedsignRequestoperation that cannot honor the requested signing mode. Reusing that response makes the generated contract and example error message misleading; add a response description for an unsupported requested mode or broaden the shared component.
406:
$ref: '#/components/responses/UnsupportedOperationResponse'
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| enum: | ||
| - vended-credentials | ||
| - remote-signing | ||
| - presigned-urls |
There was a problem hiding this comment.
Minor: I feel like remote-presigning would be more consistent.
|
|
||
| `RemoteSignResult` takes one of two forms. With `remote-signing`, `uri` is the | ||
| requested URI and `headers` carries the signature. With `presigned-urls`, `uri` | ||
| carries the signature and `headers` is empty. |
There was a problem hiding this comment.
and
headersis empty.
This is incorrect. The headers should be included with the request. We shouldn't ignore them just because the url is signed.
There was a problem hiding this comment.
Thanks @danielcweeks - I had used an empty headers as the way to tell the pre-signed forms apart, because I think it'll be useful for the client to verify that the response is in the mode it wants for the specific use case.
I agree with you that we can simply require that the headers are sent in the request to be more consistent with the protocol. And we could consider giving up on the client being able to reliably distinguish which mode of signing it had received from the REST server.
I'll add this as a future discussion point for the FILE access governance discussions at the FILE sync.
There was a problem hiding this comment.
I think the request as provided should be executable regardless. The implementation should be able to distinguish a presigned from remote signed request. This distinction is trivial and we shouldn't rely on side effects (like empty headers) as a signalling mechanism.
There was a problem hiding this comment.
@danielcweeks I thought about this more, and I also see use cases where we may want to keep the headers in the response when using presigned URL with FILE: https://docs.aws.amazon.com/AmazonS3/latest/userguide/using-presigned-url.html
Q: Why am I getting an AccessDenied error with HeadersNotSigned: if-range?
When Range is included in X-Amz-SignedHeaders, Amazon S3 requires that If-Range also be signed if present in the request. Add If-Range to X-Amz-SignedHeaders when you generate the presigned URL.
If a FILE value has an offset, I think we want to keep the protocol open to being able to support presigned urls that are bound to a Range. I agree fully that keeping the spec generalized in the current form across both modes makes sense
| Only `remote-signing` and `presigned-urls` are evaluated on this operation. A request | ||
| that selects both modes is malformed, 400. When no mode is present the request is | ||
| treated as `remote-signing`. |
There was a problem hiding this comment.
I don't agree with this. If both modes are provided, it means that the client will support and use either one. The header was intended to allow for multiple modes, so that should be supported.
There was a problem hiding this comment.
Agree, I can see a client doing that. If a client doesn't want to ask for both, it'll just send one mode in the header.
I agree it's simpler to keep this consistent with the original semantic of X-Iceberg-Access-Delegation
|
@sungwy and @singhpk234 we need to align this and #17332 I think this PR has the right property names, but is missing optional fields in the response. |
data-access to signRequest , and presigned-urls to data-accessdata-access to signRequest , and remote-presigning to data-access
Adds a new enum value
remote-presigningtodata-accessorX-Iceberg-Access-Delegationheader.This new value can be used as the existing capability negotiation mechanism by the client to signal to the server that it supports
remote-presigning.In addition, add
data-accesstosignRequestoperation as a selection mechanism for the client to request a specific mode of signing (remote-presigningvsremote-signingaka signed header mode) when using thesignRequestoperation.POC: #18110