feat(storage): implement V4 Signed Policy Documents#5914
Conversation
This reverts commit eded1b1.
There was a problem hiding this comment.
Code Review
This pull request implements support for GCS V4 Signed Policy Documents (POST Object Forms) by introducing PostPolicyV4Builder and PostPolicyV4Result, along with associated examples, integration tests, and conformance tests. Key feedback includes addressing a bug where custom endpoints with ports lose their port number during URL resolution, automatically prepending $ to starts-with fields, validating expiration limits and content length ranges, avoiding an unnecessary clone of client_email, and switching from HashMap to BTreeMap for deterministic field ordering.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5914 +/- ##
==========================================
- Coverage 97.90% 97.85% -0.05%
==========================================
Files 234 235 +1
Lines 59422 59831 +409
==========================================
+ Hits 58176 58548 +372
- Misses 1246 1283 +37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| let mut fields = BTreeMap::new(); | ||
| fields.insert("key".to_string(), self.object.clone()); | ||
| fields.insert( | ||
| "x-goog-algorithm".to_string(), | ||
| "GOOG4-RSA-SHA256".to_string(), | ||
| ); | ||
| fields.insert("x-goog-credential".to_string(), credential); | ||
| fields.insert("x-goog-date".to_string(), request_timestamp); | ||
| fields.insert("x-goog-signature".to_string(), signature_hex); | ||
| fields.insert("policy".to_string(), encoded_policy); | ||
|
|
||
| // Add user-supplied fields (including custom metadata or x-ignore- fields) | ||
| for (key, value) in &self.fields { | ||
| fields.insert(key.clone(), value.clone()); | ||
| } |
There was a problem hiding this comment.
What happens if we have
PostPolicyV4Builder::for_object(bucket, "object") .with_field("key", "another_object");
Looking at the code right now, I think that what would happen is that the signing happens with the original "object" (since user supplied fields with reserved keywords are ignored when building conditions), but lines 310-312 end up mapping key to another_object, meaning that the signature no longer matches the contents of the form. GCS will probably reject this?
Compare with SignedUrlBuilder (
Perhaps the safest way is to, in both SignedUrlBuilder and here, maintain a list of reserved keywords and reject with an error if the user tries to use one. We can leave that decision till later since we'll have to update SignedUrlBuilder.
There was a problem hiding this comment.
Done.
I had made some changes to this sign_with method to address your concerns as follows:
- In the output fields, I change the order in which reserved fields and user supplied fields are being added. Hence, now, the post_policy
sign_withmethod is handling the output fields like what signed_url is handling the query paramters, that user cannot override the reserved fields. This also ensures that the signature will match the contents of the output form. - Yes, I am maintaining a list of reserved keywords and does not add to the input JSON document at all (there is a strict order requirement in the JSON input document before signing, so not adding them in the beginning is the best solution). Based on my research with other SDK implementation and a cross-check with the Rust SDK community, a silent ignoring here is good enough; we don't need to explicitly reject user's entire builder.
|
@joshuatants I've made 9 more commits since your review, to address your concerns and also further improved the code. These are the changes after your review:
|
| /// Creates [V4 Signed Policy Documents] (POST Object Forms). | ||
| /// | ||
| /// This builder allows you to generate signed V4 POST policy documents for Google Cloud Storage. | ||
| /// A [Signed Policy Document] enables unauthenticated users to upload files to GCS using an HTML form |
There was a problem hiding this comment.
I don't see a definition for Signed Policy Document. Does this link work?
| if let Some(port) = url.port() { | ||
| host.push_str(&format!(":{port}")); | ||
| } |
There was a problem hiding this comment.
Perhaps confusingly, if an explicitly specified port is the default port for a given scheme,url::port will not return it (https://docs.rs/url/2.5.8/url/struct.Url.html#method.port), i.e.
https://host:443 -> host
http://host:80 -> host
Based on
google-cloud-rust/src/storage/src/storage/signed_url.rs
Lines 472 to 477 in bbfc163
| .with_expiration(Duration::from_secs(30 * 60)) // 30 minutes | ||
| .with_field("Content-Type", "text/plain") | ||
| .with_field("x-goog-meta-test", "data") | ||
| .with_starts_with("$key", "") |
There was a problem hiding this comment.
"" is the empty string, sowith_starts_with("$key", "") is trivially true.
Description
This PR introduces support for generating V4 Signed Policy Documents (
PostPolicyV4Builder), bringing the Rust SDK into feature parity with other official GCS SDKs for POST form object uploads.Design Doc: go/rust-sdk-feature-v4-signed-policy-document-implementation_plan
Key Additions
PostPolicyV4BuilderAPI: A fluent builder API to configure URL styles, exact-match fields,starts-withprefix conditions, andcontent-length-rangelimits.\uXXXX) to guarantee flawless Base64 JSON encoding, satisfying GCS's stringent signature validation requirements.Testing & Validation
generate_signed_post_policy_v4sample code against a real Cloud Storage bucket and verified the upload worked correctly.v4_signatures.jsontest harness, successfully passing 100% (11/11) of thepostPolicyV4Tests(covering Path Style, Virtual Hosted Style, Bucket Bound Hostnames, and Unicode conditions).$keycondition formulation and the maximum 7-day expiration bounds behave identically to the official Go and C++ SDKs.signed_post_policies_v4intests/storage/src/lib.rswhich dynamically constructs a policy and performs a live upload using areqwestmultipart form. It ran successfully against a real Cloud bucket.