feat(storage): post policy can be signed with a fn that takes raw bytes #5079

BrennaEpp · 2021-11-04T00:19:38Z

Adds a SignRawBytes field to PostPolicyV4Options. This field works like the SignedURLOptions SignBytes field. This function expects raw bytes as opposed to the SignBytes field which expects the bytes to be hashed before called.
Deprecates the SignBytes field of PostPolicyV4Options
Adds a test to ensure that both bucket.SignedURL and GenerateSignedPostPolicyV4 work with the same signing function for SignBytes and SignRawBytes respectively.
Refactors some test code to reuse checks for signed urls and post policies; adds a check to TestIntegration_SignedURL to error on an expected failure not failing.

google-cla · 2021-11-04T00:19:43Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-11-04T00:20:08Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-11-04T03:29:14Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

codyoss · 2021-11-04T15:15:10Z

@googlebot I consent.

codyoss

LGTM, in general. I'll let Chris look more closely. One minor comment.

storage/post_policy_v4.go

gcf-merge-on-green · 2021-11-06T00:16:13Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

tritone

Generally looks good, a few comments mostly on docs.

tritone · 2021-11-06T00:47:33Z

storage/integration_test.go

+
+// verifyPostPolicy uploads a file to the obj using the provided post policy and
+// verifies that it was uploaded correctly
+func verifyPostPolicy(pv4 *PostPolicyV4, obj *ObjectHandle, bytesToWrite []byte, statusCodeOnSuccess int) error {


Don't think passing StatusCodeOnSuccess is necessary; it should always be 200 right?

It can be different depending on what you specify in PostPolicyV4Options.PolicyV4Fields.StatusCodeOnSuccess

I think currently it is 200 in all these tests, so we shouldn't pass more params unnecessarily if possible.

storage/integration_test.go

tritone · 2021-11-06T01:09:58Z

storage/post_policy_v4.go

@@ -52,22 +52,31 @@ type PostPolicyV4Options struct {
 	// Exactly one of PrivateKey or SignBytes must be non-nil.
 	PrivateKey []byte

-	// SignBytes is a function for implementing custom signing. For example, if
+	// SignBytes is a function for implementing custom signing.


Maybe include a sentence about how it differs from SignRawBytes even though we are deprecating it.

tritone · 2021-11-06T01:11:40Z

storage/post_policy_v4.go

+	//		bytes = shaSum[:]
+	SignBytes func(hashBytes []byte) (signature []byte, err error)
+
+	// SignRawBytes is a function for implementing custom signing. For example, if


I would note explicitly that the same function that works for SignBytes for signed URLs will work for SignRawBytes for post policy. Obviously it's confusing but still better to state explicitly.

tritone · 2021-11-06T01:14:54Z

storage/post_policy_v4.go

+	var signature []byte
+	var signErr error
+	// SignBytes is used only if SignRawBytes is not defined
+	if opts.SignRawBytes == nil && opts.SignBytes != nil {


I worry about the fact that we are checking this condition multiple times. Maybe we just add an unexported field to the options about whether the signed bytes should be hashed or not, set that once when parsing the options, then make subsequent decisions accordingly?

tritone

Nice improvements, looks really good!

codyoss and others added 3 commits November 2, 2021 10:47

example

31cc7d7

add test and comments

e2739e4

change name

65d5d50

BrennaEpp requested review from a team as code owners November 4, 2021 00:19

product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Nov 4, 2021

google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Nov 4, 2021

Merge branch 'master' into storage-sign-post-policy

097840e

BrennaEpp requested review from tritone and codyoss November 4, 2021 00:50

Merge branch 'master' into storage-sign-post-policy

0193cdd

google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Nov 4, 2021

codyoss reviewed Nov 4, 2021

View reviewed changes

storage/post_policy_v4.go Outdated Show resolved Hide resolved

BrennaEpp added 4 commits November 4, 2021 12:17

example

9606c91

Merge branch 'master' into storage-sign-post-policy

f5d5002

Merge branch 'master' into storage-sign-post-policy

cf23063

Merge branch 'master' into storage-sign-post-policy

3e59217

BrennaEpp added the automerge Merge the pull request once unit tests and other checks pass. label Nov 5, 2021

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Nov 6, 2021

tritone reviewed Nov 6, 2021

View reviewed changes

BrennaEpp added 4 commits November 8, 2021 12:57

change obj name

2d387aa

add comment

879652c

add comment on function working w signbutes/signrawbytes

63e5a18

add unexported field

a562c83

BrennaEpp added 3 commits November 9, 2021 02:20

Merge branch 'main' into storage-sign-post-policy

c4960c2

add comment

64c59ff

Merge branch 'main' into storage-sign-post-policy

940ecfd

BrennaEpp requested a review from tritone November 9, 2021 18:25

tritone approved these changes Nov 9, 2021

View reviewed changes

BrennaEpp merged commit 25d1278 into googleapis:main Nov 9, 2021

BrennaEpp deleted the storage-sign-post-policy branch March 23, 2022 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(storage): post policy can be signed with a fn that takes raw bytes #5079

feat(storage): post policy can be signed with a fn that takes raw bytes #5079

BrennaEpp commented Nov 4, 2021 •

edited

google-cla bot commented Nov 4, 2021

google-cla bot commented Nov 4, 2021

google-cla bot commented Nov 4, 2021

codyoss commented Nov 4, 2021

codyoss left a comment

gcf-merge-on-green bot commented Nov 6, 2021

tritone left a comment

tritone Nov 6, 2021

BrennaEpp Nov 8, 2021

tritone Nov 9, 2021 •

edited

tritone Nov 6, 2021

tritone Nov 6, 2021

tritone Nov 6, 2021

tritone left a comment

feat(storage): post policy can be signed with a fn that takes raw bytes #5079

feat(storage): post policy can be signed with a fn that takes raw bytes #5079

Conversation

BrennaEpp commented Nov 4, 2021 • edited

google-cla bot commented Nov 4, 2021

google-cla bot commented Nov 4, 2021

google-cla bot commented Nov 4, 2021

codyoss commented Nov 4, 2021

codyoss left a comment

Choose a reason for hiding this comment

gcf-merge-on-green bot commented Nov 6, 2021

tritone left a comment

Choose a reason for hiding this comment

tritone Nov 6, 2021

Choose a reason for hiding this comment

BrennaEpp Nov 8, 2021

Choose a reason for hiding this comment

tritone Nov 9, 2021 • edited

Choose a reason for hiding this comment

tritone Nov 6, 2021

Choose a reason for hiding this comment

tritone Nov 6, 2021

Choose a reason for hiding this comment

tritone Nov 6, 2021

Choose a reason for hiding this comment

tritone left a comment

Choose a reason for hiding this comment

BrennaEpp commented Nov 4, 2021 •

edited

tritone Nov 9, 2021 •

edited