feat(storage): SignedUrl can use existing creds to authenticate #4604
Conversation
product-auto-label
bot
added
the
api: storage
google-cla
bot
added
the
cla: yes
| if len(newopts.PrivateKey) == 0 { | ||
| newopts.SignBytes = b.defaultSignBytesFunc(newopts.GoogleAccessID) | ||
| } | ||
| } |
There was a problem hiding this comment.
We probably want an error here if for whatever reason PrivateKey or SignBytes hasn't been populated at this point.
There was a problem hiding this comment.
SignBytes should definitely be populated by b.defaultSignBytesFunc(newopts.GoogleAccessID), but I can guess an extra check wouldn't hurt?
There was a problem hiding this comment.
Hmm yeah I guess that's correct-- so then if there is an issue with signBytes, it will occur when it is called to do the signing. That seems fine. Can you verify that the error looks sensible if, for example, we don't have correct permissions and so signBytes fails?
There was a problem hiding this comment.
Error if the service account email is not found by the IAM client (for example can occur if you use your email instead of a service account email or there's a typo in your email):
unable to sign bytes: googleapi: Error 404: Requested entity was not found.
This error isn't the best but it's what we receive from the IAM api: https://cloud.google.com/iam/docs/reference/credentials/rest/v1/projects.serviceAccounts/signBlob
Error if the service account does not have permissions:
unable to sign bytes: googleapi: Error 403: The caller does not have permission
Error if the Service Account Credentials API is not enabled:
unable to sign bytes: googleapi: Error 403: IAM Service Account Credentials API has not been used in project 1074109021461 before or it is disabled. Enable it by visiting https://console.developers.google.com/apis/api/iamcredentials.googleapis.com/overview?project=1074109021461 then retry. If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry.
Details:
[
{
"@type": "type.googleapis.com/google.rpc.Help",
"links": [
{
"description": "Google developers console API activation",
"url": "https://console.developers.google.com/apis/api/iamcredentials.googleapis.com/overview?project=1074109021461"
}
]
},
{
"@type": "type.googleapis.com/google.rpc.ErrorInfo",
"domain": "googleapis.com",
"metadata": {
"consumer": "projects/1074109021461",
"service": "iamcredentials.googleapis.com"
},
"reason": "SERVICE_DISABLED"
}
]
storage/integration_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("unable to find test credentials: %v", err) | ||
| } | ||
| client, err := newTestClient(ctx, option.WithCredentials(creds)) |
There was a problem hiding this comment.
Is this creds magic necessary? What happens if you run the tests with a test client as-is?
There was a problem hiding this comment.
Looks like it's fine without it as long as env variables are set correctly. @codyoss is there a reason we can't call newTestClient without passing the creds?
There was a problem hiding this comment.
So here we are explicitly testing the SA use-case and signing with it IIRC. By default we are authed with a TS so don't have the context to read some of the fields from the keyfile. For completeness we could also have a test that does the iamcredentials flow where we auth like normal but set the email address field. Maybe this bit of code deserves a comment 😛
There was a problem hiding this comment.
I added a couple comments and the extra test case
| if len(newopts.PrivateKey) == 0 { | ||
| newopts.SignBytes = b.defaultSignBytesFunc(newopts.GoogleAccessID) | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmm yeah I guess that's correct-- so then if there is an issue with signBytes, it will occur when it is called to do the signing. That seems fine. Can you verify that the error looks sensible if, for example, we don't have correct permissions and so signBytes fails?
Conflicts: storage/go.mod storage/go.sum
|
See also the Go CDK’s google/go-cloud#2800 |
Part of the effort to improve Signed URLs in the Go Storage Client.
Users can omit filling in GoogleAccessID, PrivateKey, and SignBytes if they are okay with default values for these.
Fixes #1130