steveloughran commented on code in PR #15112:
URL: https://github.com/apache/iceberg/pull/15112#discussion_r2759181348
##########
open-api/rest-catalog-open-api.py:
##########
@@ -985,6 +985,39 @@ class PlanTask(BaseModel):
)
+class MultiValuedMap(BaseModel):
+ """
+ A map of string keys where each key can map to multiple string values.
+ """
+
+ __root__: dict[str, list[str]]
+
+
+class RemoteSignRequest(BaseModel):
+ """
+ The request to be signed remotely.
+ """
+
+ region: str
+ uri: str
+ method: Literal['PUT', 'GET', 'HEAD', 'POST', 'DELETE', 'PATCH', 'OPTIONS']
+ headers: MultiValuedMap
+ properties: dict[str, str] | None = None
+ body: str | None = Field(
+ None,
+ description='Optional body of the request to send to the signing API.
This should only be populated for requests which do not have the relevant data
in the URI itself (e.g. DeleteObjects requests)',
Review Comment:
This implies that the body of PUT and multipart uploads are to be be
included. They don't have to be, provided the checksum information is provided
as a signed header. This avoids having to upload many GB of data to the
service, needlessly.
Not sure about whether DeleteObjects qualifies here. I think technically
it'd be OK to just sign the checksum, but given the damage the request can do,
a production signing service SHOULD validate the entire request and every path
in the body.
This'd argue for a list of which commands the body MUST be included
(DeleteObjects), and where it SHOULD be omitted
* PUT
* UploadPart
* CompleteMultipartUpload
##########
aws/src/integration/java/org/apache/iceberg/aws/s3/signer/S3SignerServlet.java:
##########
@@ -65,113 +46,33 @@
* {@link S3SignerServlet} provides a simple servlet implementation to emulate
the server-side
* behavior of signing S3 requests and handling OAuth.
*/
-public class S3SignerServlet extends HttpServlet {
-
- private static final Logger LOG =
LoggerFactory.getLogger(S3SignerServlet.class);
+public class S3SignerServlet extends RemoteSignerServlet {
static final Clock SIGNING_CLOCK = Clock.fixed(Instant.now(),
ZoneId.of("UTC"));
static final Set<String> UNSIGNED_HEADERS =
Review Comment:
good time to review the headers which mustn't be used in signing to assist
in caching
Can you add "Referer" to this list; it's used used in s3a audit log, and if
people work off this list they won't include it.
it's almost worth pulling this is out to list headers to to skip.
* User-Agent
* Referer (yes, that's the spelling)
+ any others people know of where they'd taint cacheing and are harmless
##########
aws/src/integration/java/org/apache/iceberg/aws/s3/signer/S3SignerServlet.java:
##########
@@ -65,113 +46,33 @@
* {@link S3SignerServlet} provides a simple servlet implementation to emulate
the server-side
* behavior of signing S3 requests and handling OAuth.
*/
-public class S3SignerServlet extends HttpServlet {
-
- private static final Logger LOG =
LoggerFactory.getLogger(S3SignerServlet.class);
+public class S3SignerServlet extends RemoteSignerServlet {
static final Clock SIGNING_CLOCK = Clock.fixed(Instant.now(),
ZoneId.of("UTC"));
static final Set<String> UNSIGNED_HEADERS =
Sets.newHashSet(
Arrays.asList("range", "x-amz-date", "amz-sdk-invocation-id",
"amz-sdk-retry"));
- private static final String POST = "POST";
-
- private static final Set<SdkHttpMethod> CACHEABLE_METHODS =
- Stream.of(SdkHttpMethod.GET,
SdkHttpMethod.HEAD).collect(Collectors.toSet());
-
- private final Map<String, String> responseHeaders =
- ImmutableMap.of(HttpHeaders.CONTENT_TYPE,
ContentType.APPLICATION_JSON.getMimeType());
- private final ObjectMapper mapper;
-
- private List<SignRequestValidator> s3SignRequestValidators =
Lists.newArrayList();
-
- /**
- * SignRequestValidator is a wrapper class used for validating the contents
of the S3SignRequest
- * and thus verifying the behavior of the client during testing.
- */
- public static class SignRequestValidator {
- private final Predicate<S3SignRequest> requestMatcher;
- private final Predicate<S3SignRequest> requestExpectation;
- private final String assertMessage;
-
- public SignRequestValidator(
- Predicate<S3SignRequest> requestExpectation,
- Predicate<S3SignRequest> requestMatcher,
- String assertMessage) {
- this.requestExpectation = requestExpectation;
- this.requestMatcher = requestMatcher;
- this.assertMessage = assertMessage;
- }
-
- void validateRequest(S3SignRequest request) {
- if (requestMatcher.test(request)) {
-
assertThat(requestExpectation.test(request)).as(assertMessage).isTrue();
- }
- }
- }
-
- public S3SignerServlet(ObjectMapper mapper) {
- this.mapper = mapper;
- }
-
- public S3SignerServlet(ObjectMapper mapper, List<SignRequestValidator>
s3SignRequestValidators) {
- this.mapper = mapper;
- this.s3SignRequestValidators = s3SignRequestValidators;
- }
-
- @Override
- protected void doGet(HttpServletRequest request, HttpServletResponse
response) {
- execute(request, response);
- }
- @Override
- protected void doHead(HttpServletRequest request, HttpServletResponse
response) {
- execute(request, response);
- }
+ /** A fake remote signing endpoint for testing purposes. */
+ static final String S3_SIGNER_ENDPOINT =
"v1/namespaces/ns1/tables/t1/sign/s3";
- @Override
- protected void doPost(HttpServletRequest request, HttpServletResponse
response) {
- execute(request, response);
+ public S3SignerServlet() {
+ super(S3_SIGNER_ENDPOINT);
}
@Override
- protected void doDelete(HttpServletRequest request, HttpServletResponse
response) {
- execute(request, response);
- }
-
- private OAuthTokenResponse handleOAuth(Map<String, String> requestMap) {
- String grantType = requestMap.get("grant_type");
- switch (grantType) {
- case "client_credentials":
- return castResponse(
- OAuthTokenResponse.class,
- OAuthTokenResponse.builder()
- .withToken("client-credentials-token:sub=" +
requestMap.get("client_id"))
-
.withIssuedTokenType("urn:ietf:params:oauth:token-type:access_token")
- .withTokenType("Bearer")
- .setExpirationInSeconds(10000)
- .build());
-
- case "urn:ietf:params:oauth:grant-type:token-exchange":
- String actor = requestMap.get("actor_token");
- String token =
- String.format(
- "token-exchange-token:sub=%s%s",
- requestMap.get("subject_token"), actor != null ? ",act=" +
actor : "");
- return castResponse(
- OAuthTokenResponse.class,
- OAuthTokenResponse.builder()
- .withToken(token)
-
.withIssuedTokenType("urn:ietf:params:oauth:token-type:access_token")
- .withTokenType("Bearer")
- .setExpirationInSeconds(10000)
- .build());
-
- default:
- throw new UnsupportedOperationException("Unsupported grant_type: " +
grantType);
+ protected void validateSignRequest(RemoteSignRequest request) {
+ if (HttpMethod.POST.name().equalsIgnoreCase(request.method())
+ && request.uri().getQuery().contains("delete")) {
+ String body = request.body();
+ Preconditions.checkArgument(
Review Comment:
needs to be escalated to a 400 response somehow
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
