-
Notifications
You must be signed in to change notification settings - Fork 533
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
Support workload identity and default app credentials for GCS signed URLs and or provide example of how to sign the blob yourself #2410
Comments
It's unclear what you mean by that. Supported in what respect? Please be more specific. |
Apologies, I hadn't noticed that @amanda-tarafa had specifically requested raising an issue here instead of in the other repo. (She's worked on the signing infrastructure more recently than I have, so I trust her judgement. The sample code change would be in google-cloud-dotnet, but we can handle that.) |
Cool thanks! Yeah I'm just looking for some kind of official-ish example snippet so I know I'm doing it right as it seems non-standard/undocumented. |
We can work on creating a sample - we had a sample until recently, which was removed in this commit - you may be able to use that as a starting point until we can potentially restore it in the docs. @amanda-tarafa Can you remember why we removed the IAM service signer sample? Was it that we didn't expect it to be useful with the new support? @red8888: If you could clarify what you would expect to be able to do with default application credentials that doesn't currently work, that would help us to help you. |
@jskeet Sorry default app creds may work as expected so I guess I shouldn't have assumed. I just saw other somewhat related github issues saying it didn't or it behaved differently: octue/django-gcp#10 I really just wanted to confirm that if I use sigblob the same code will run correctly on gke with workload id and locally with default app creds without needing to introduce logic in the code to support both scenarios. |
Right, I believe ADC credentials will work if the underlying credential supports signing (which I believe is still not the case for WIF, as Amanda indicated in the Stack Overflow answer).
I'd expect that to be the case if both kinds of credential have access to IAM to perform the signing remotely. But it'll be significantly slower than any local signing. At this point we're really at the edge of my knowledge though, so I'll assign to @amanda-tarafa for further comment. |
We might be able to offer some out of the box sopport for URL signing with external account credentials, but we need to confirm a few things first, so please bare with us. If we cannot offer out of the box support we'll publish a sample. I'll update here as we know more. |
As described in the documentation, GoogleCredential (The type returnes by ADC) is only supported if the underlying credential type is itself supported, and user crendetial is not currently supported out of the box. I'll update when I know more. |
Here's a class that I was able to use to generate signed URLs. It uses the IAM API SignBlob operation since we wanted to use default application credentials instead of a service account key.
|
@amanda-tarafa any updates on this feature being part of the library? Or do you have an official example? |
@mbamm We'll work on this next year but we don't have an ETA yet. I'd expect this to happen in the first half of the year though. As a sample starting point, you can use the sample that we originally had and was removed in this commit. For context, we removed the sample because it was meant to demonstrate URL signing using a Compute credential, and that is currently supported by the library out of the box. It's just Workload Indentity Federation credentials that are not supported out of the box at the moment. |
@amanda-tarafa any updates on this feature being part of the library? thanks |
Just want to circle back on this, we really need doc updates ASAP while we wait on official SDK support. I just had to have this same conversation with python devs about a month ago. I don't think its hyperbolic at this point to say this is very bad. Google is directly contradicting their own security best practices and the docs are a real problem. They merely say you MUST use a json key. I have to explain to devs that this is in fact not required and its just a missing feature of the SDKs- essentially convincing them Google's own docs are bad and violate their own security best practices :( Maybe a stop gap would be to add an info/warning box or something to the docs so I don't have to keep having these conversations? Google is actively pitting my devs against me here lol. The least we could do is add notes/examples to the docs so there is something official that helps me press the devs to do a little extra work and not create static json keys especially after setting up workload id and oidc etc everywhere. |
@mbamm still on the backlog I'm afraid, but something we still want to support out of the box. I can't offer an ETA at the moment, I'm sorry about that. Please see my previous comment. You can use the sample linked there as a starting point for implementing IAM calls to the signing endpoint using a Workload Identity Credential. Just note the example is for a Compute Credential, which is already supported out of the box by the .NET Clud SDK. @red8888 if you can provide a link to the documentation contradicting best practices I'm happy to look into getting those fixed or pointing you to the correct repos to make such requests. |
See SO post: https://stackoverflow.com/questions/76266019/how-do-i-generate-signed-urls-for-gcs-with-workload-identity-in-the-c-sharp-sdk/76266912#76266912
Currently workload id is not supported for generating signed URLs for GCS. This is a problem because it forces a bad practice (generating a static json key instead of using workload id which is much more secure).
At the very least can someone provide a full example of how to use the SignBlob method in conjunction with SignedURL to work around this problem?
Also, I think default app creds are also not supported right? This makes the code painful to run locally for testing purposes.
The text was updated successfully, but these errors were encountered: