Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: address missing bucket in policy condition #31

Merged
merged 1 commit into from
Apr 25, 2020

Conversation

frankyn
Copy link
Member

@frankyn frankyn commented Apr 24, 2020

This prevents the possibility of changing the bucket name after the policy is signed allowing bad actors to upload to a different bucket that it was originally intended to.

Pending review with @ranylee.

  1. Test Example
  2. bucket name: rsaposttest-1579902670-h3q7wvodjor6bc7y

Before change

{"conditions":[{"key":"test-object"},{"x-goog-date":"20200123T043530Z"},{"x-goog-credential":"test-iam-credentials@dummy-project-id.iam.gserviceaccount.com/20200123/auto/storage/goog4_request"},{"x-goog-algorithm":"GOOG4-RSA-SHA256"}],"expiration":"2020-01-23T04:35:40Z"}

Change is to introduce bucket name as an additional condition in the Signed Policy. No additional field is outputted.

{"conditions":[{"bucket":"rsaposttest-1579902670-h3q7wvodjor6bc7y"},{"key":"test-object"},{"x-goog-date":"20200123T043530Z"},{"x-goog-credential":"test-iam-credentials@dummy-project-id.iam.gserviceaccount.com/20200123/auto/storage/goog4_request"},{"x-goog-algorithm":"GOOG4-RSA-SHA256"}],"expiration":"2020-01-23T04:35:40Z"}

@frankyn frankyn requested a review from quartzmo April 24, 2020 22:13
@quartzmo
Copy link
Member

All unit and acceptance tests passing in google-cloud-ruby with this change:

google-cloud-storage % git diff
diff --git a/google-cloud-storage/lib/google/cloud/storage/file/signer_v4.rb b/google-cloud-storage/lib/google/cloud/storage/file/signer_v4.rb
index 69a6242f6d..72af381a46 100644
--- a/google-cloud-storage/lib/google/cloud/storage/file/signer_v4.rb
+++ b/google-cloud-storage/lib/google/cloud/storage/file/signer_v4.rb
@@ -164,6 +164,7 @@ module Google
 
           def required_fields issuer, time
             {
+              "bucket" => @bucket_name,
               "key" => @file_name,
               "x-goog-date" => time.strftime("%Y%m%dT%H%M%SZ"),
               "x-goog-credential" => "#{issuer}/#{time.strftime '%Y%m%d'}/auto/storage/goog4_request",

quartzmo added a commit to quartzmo/google-cloud-ruby that referenced this pull request Apr 24, 2020
@frankyn
Copy link
Member Author

frankyn commented Apr 24, 2020

Perfect, thanks for confirming @quartzmo!

@jkwlui jkwlui changed the title fix: address missing condition bucket fix: address missing bucket condition Apr 25, 2020
@jkwlui jkwlui changed the title fix: address missing bucket condition fix: address missing bucket in policy condition Apr 25, 2020
@frankyn frankyn merged commit fa559a1 into master Apr 25, 2020
@frankyn frankyn deleted the fix-security-issue branch April 25, 2020 00:11
gcf-merge-on-green bot pushed a commit to googleapis/java-conformance-tests that referenced this pull request Apr 27, 2020
gopherbot pushed a commit to googleapis/google-cloud-go that referenced this pull request May 11, 2020
This implementation passes:
* all conformance tests
* the integration test
* the complete end-to-end example test

Also while here, updated the conformance tests to address
the security risk due to the lack of bucket enforcement
as per googleapis/conformance-tests#31

Fixes #1748
Fixes #1954

Change-Id: Id39948c8554952a640e83a108c1b3015bd110497
Reviewed-on: https://code-review.googlesource.com/c/gocloud/+/55330
Reviewed-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: Chris Cotter <cjcotter@google.com>
Reviewed-by: Chris Broadfoot <cbro@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants