alamb commented on code in PR #771:
URL:
https://github.com/apache/arrow-rs-object-store/pull/771#discussion_r3798977592
##########
CONTRIBUTING.md:
##########
@@ -148,6 +149,75 @@ export TEST_S3_SSEC_ENCRYPTION=1
cargo test --features aws --package object_store --lib
aws::tests::test_s3_ssec_encryption_with_minio -- --exact --nocapture
```
+#### Presigned URL signature-enforcement tests
+
+A handful of presigned-URL tests assert that the *storage backend* rejects an
invalid request:
+a tampered signature, an expired URL, or a signed header whose value the
client changed. These
+require a backend that actually validates SigV4. LocalStack does not (it
accepts presigned
+requests regardless of signature or expiry), so these tests are gated behind a
separate
+`TEST_S3_SIGNATURE_ENFORCEMENT` variable rather than running in the main
LocalStack integration
+pass. CI runs them against MinIO instead, which does validate SigV4 (the "Run
presigned-URL
+signature-enforcement tests (MinIO)" step in `ci.yml`), so they stay
exercised. The steps below
+reproduce that locally or point at real S3.
+
+These tests use a dedicated `test-bucket-for-signing` bucket so their writes
cannot contaminate the
+shared `test-bucket` whose exact contents `s3_test` asserts. To point at a
bucket in your own
+account instead, set `OBJECT_STORE_SIGNING_BUCKET` (no source edit required).
+
+MinIO is the recommended local backend: unlike LocalStack it validates SigV4
signatures and expiry,
+so the enforcement assertions actually exercise. Plain HTTP is enough (these
tests don't use SSE-C,
+so the self-signed-cert setup from the SSE-C section is not needed, and the
test client would
+reject that cert anyway):
+
+```shell
+docker run -d -p 9000:9000 \
Review Comment:
I tested this and it works great
##########
CONTRIBUTING.md:
##########
@@ -148,6 +149,75 @@ export TEST_S3_SSEC_ENCRYPTION=1
cargo test --features aws --package object_store --lib
aws::tests::test_s3_ssec_encryption_with_minio -- --exact --nocapture
```
+#### Presigned URL signature-enforcement tests
+
+A handful of presigned-URL tests assert that the *storage backend* rejects an
invalid request:
+a tampered signature, an expired URL, or a signed header whose value the
client changed. These
+require a backend that actually validates SigV4. LocalStack does not (it
accepts presigned
+requests regardless of signature or expiry), so these tests are gated behind a
separate
+`TEST_S3_SIGNATURE_ENFORCEMENT` variable rather than running in the main
LocalStack integration
+pass. CI runs them against MinIO instead, which does validate SigV4 (the "Run
presigned-URL
Review Comment:
👍
##########
src/aws/mod.rs:
##########
@@ -141,6 +141,66 @@ impl Signer for AmazonS3 {
/// # }
/// ```
async fn signed_url(&self, method: Method, path: &Path, expires_in:
Duration) -> Result<Url> {
+ self.signed_url_opts(method, path, expires_in,
&SignedUrlOptions::default())
+ .await
+ }
+
+ /// Create a signed URL, additionally folding the query parameters and
headers in `options`
Review Comment:
this is really nicely written and explained. Thank you
--
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]
