Fix: Generate Signed Urls through a service account by providing service_account_email and access_token

[yohannes15]

fixes #941

• Adds sa_email setting in order to set the service_account_email used for signing URL if service_account key file not available in the env. Compute services (Cloud Run, Cloud Function, App Engine, Compute Engine ...) don't have access to the key file but get access tokens through the metadata server. Passing service_account_email and access_token will cause generate_signed_url to use the IAM API to signBlob using the provided service account email and token.

[jschneier]

One main comment about token freshness.

We also should add at least one test.

The note about authentication is helpful but I'm wondering if it needs to be more deeply integrated into the docs rather than a separate note.

[jschneier]

Forgot to say, this looks great! Minimal and clear.

[yohannes15]

I adjusted some existing tests that were failing due to error Default Creds missing by mocking _client. I will do some additional tests for the feature add plus a credentials test when I get time. The test you set up test_custom_endpoint_with_parameters is now passing as well.

[jschneier]

Curious why you've decided to go in the new direction? The last set of changes seemed easier to understand but I am likely missing some context.

[yohannes15]

@jschneier the previous set of changes was easier to understand but we would be forcing all envs that had the service account email to start signing using the IAM Sign Blob API. That API has a rate limit. Thats why I was thinking maybe it is better to make this a setting that has to be provided so we don't break any large service that potentially has a service account email in the env but is signing with a private key file. Signing with a key file in the env doesn't cause an extra API call, so we should probably keep that as is and only use the sign blob API if required.
Does that make sense? This might be helpful

https://stackoverflow.com/questions/65169199/google-cloud-storage-signed-urls-is-there-an-upper-limit-on-the-number-of-is
https://cloud.google.com/iam/quotas

[yohannes15]

This is the code that decides it. It is in the generate_signed_url functions. We shoud probably not pass service_account_email and access_token to the function at all times. We probably want to make it a setting (iam_blob_sign (default=False)) and require service_account_email to be provided through the adc or sa_email if True. If service account email missing and iam_blob_sign is True, we throw a attribute error because that doesn't make sense.

[jschneier]

Thanks, that makes sense, need to digest a bit.

[joonhyungshin]

This looks good to me. I also agree with the decision of adding a separate config iam_sign_blob.

[jschneier]

I agree that this direction looks good, just have a few more questions before we merge it in.

[jschneier]

Sounds good, definitely missing a few tests. We will bump the minimum required Google SDK library with this so you can remove the version mention.

[yohannes15]

@jschneier I have added some tests around the new settings initialization and also tests around the generate_signed_url calls with this new branch of code

[jschneier]

Thanks, the code for this looks very good. I think we should reword the authentication docs a bit before merging. My concern is that there is a very large blob of text discussing the limitation and for the mitigation the django-storages settings is mixed in with discussion of kwargs for generate_signed_url. I think most users won't read or untangle the details needed to get the project working.

Can you explain here what the requirements are, it sounds like setting iam_sign_blob isn't always needed? An example of how we might reword this is to add a 4. to the list of things to do with a conditional if in front of it. Then the reasoning could go into some kind of .. note or .. info block?

Given that there are now multiple ways to authenticate it also might make sense to make sub-sections so the delineation is more clear.

EDIT: Can we also fix the linter?

[yohannes15]

@jschneier I have made the changes requested. Let me know what you think

[yohannes15]

@jschneier Just checking to remind you like you said in the README : ). No worries if busy

[rwblokzijl]

@jschneier would you mind taking a look at this? Getting this working would be lovely :)