ComputeEngine ID Tokens to use metadata server - #347
Conversation
|
|
||
|
|
||
| class IDTokenCredentials(credentials.Credentials, credentials.Signing): | ||
| class IDTokenCredentials(credentials.Credentials): |
There was a problem hiding this comment.
Removing Signing breaks the existing interface of this class. Why can't these credentials continue to sign using the IAM API?
There was a problem hiding this comment.
Yeah, but the capability shouldn't be there anyway, IMO. GCE itself doens' thave the capability to sign: it can get access_tokens and even an id_token but not sign arbitrary blobs by itself. IMO the computeCredential object should just reflect its baseline capabilities the underlying provider gives here anyway (in this case, the provider is the metadata service).
The credentials.Signing capability is baked into the existing class as really just a side effect too interms of gettig an IDtoken: if you were using the IAM api, you could 'just get' an id_token directly without signing
Anyway, her'es the biggest problem i see, that in the existing implementation, the singer and sign_bytes was already exposed directly here
def signer(self):
return self._signer
def sign_bytes(self, message):
return self._signer.sign(message)I don't think people are using that capability to direct sign via IAM but it is there.
What the existing implementation is doing is basically allowing the GCE to "impersonate itself" and use the IAM API to get an id token and along the way also allow blob signing. I would say if someone wants to sigin via the IAM api, the impersonated_credential object could do that (i'll file a separate PR for something like this later
There was a problem hiding this comment.
don't think people are using that capability to direct sign via IAM but it is there.
This isn't a good enough reason for us to break a 1.0 interface. this will be a breaking change to users and it will cause issues.
There was a problem hiding this comment.
no, thats not the reason (its just a statement). the reasons are outlined above as well as the ability to sign isn't privilege a GCE instance has.
per offline, we'll can keep the singer capability as well as using the IAM API to do that bit. Getting the ID token will be through the metadta sever per the most recent commit.
|
note, i kept the def __init__(self, request, target_audience,
token_uri=_DEFAULT_TOKEN_URI,
additional_claims=None,
service_account_email=None):i can also add in warning messages to this PR if someone tries to user the |
This should make this library more robust in case of breaking changes in one of its dependencies.
Add additional way to supply creds to client without relying on I/O
Use cls parameter instead of explicit `Credentials` reference to allow subclassing
|
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 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 ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
Fixes:
Refactors google.auth.compute_engine implementation to use the metadata server (which is present in GCP's compute implementations (gce, gke*, cloud run, gcf, gae new runtimes)