Skip to content
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

(NOT FOR SUBMISSION) Prototype OIDC support #1415

Closed
wants to merge 2 commits into from

Conversation

jskeet
Copy link
Collaborator

@jskeet jskeet commented Jun 17, 2019

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 17, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 24, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Jul 3, 2019
@@ -92,6 +92,19 @@ public override async Task<bool> RequestAccessTokenAsync(CancellationToken taskC
return true;
}

/// <inheritdoc/>
public override async Task<TokenResponse> CreateOidcTokenAsync(string targetAudience, CancellationToken cancellationToken = default(CancellationToken))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we want want two additional settable parameters here:

license=true only works if format=full is set (my 2c is to auto set format=full if the user indicates license=true)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other optional parameters coming? If so, we'd probably want to create a new type for that - it's difficult to add extra parameters later.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know offhand if other parameters will be set but my guess would be yes (for example, applying user-defined claims)

if it helps, we're addign in vararg options fileds here for the other credential types:

https://github.com/googleapis/google-auth-library-java/blob/5fbaf0f211ca1a1dae05915b224a1ac3c132eb71/oauth2_http/java/com/google/auth/oauth2/IdTokenCredentials.java#L73-L89


also for ref, here is the dotnet support for the 'impersonated' credential type; i'll add a comment in that issue pointing out that for impersonated credential, an option is 'includeEmail=true'
#1312

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any varargs there - just a builder pattern. I may be missing something. (I definitely don't feel like I've got the complete context of what we're trying to achieve here, but that may just be due to coming back to this two months later.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What i meant is that you can passin arbitrary options to signal the variation of the identity token that the underlying credential type provides. For example, in java i could pass in the .setOptions() field takes in an array of potential values for the IdTokenCredential type that will get modified by the true unerlying provider

so in java for compute

.setOptions(Arrays.asList(ComputeEngineCredentials.ID_TOKEN_FORMAT_FULL))

can also have a value like

or

.setOptions(Arrays.asList(ComputeEngineCredentials.ID_TOKEN_FORMAT_FULL,
                            ComputeEngineCredentials.LICENSE_TRUE))
  • interms of why we're getting asking ComputeEngine for these extra values, the instance identity document provides the assertion of extended claims and even license values. Those can be used by remote system to attest certain things for the origin system. for info on generic id_tokens on gcp, see this link

If the pattern we're working with on google-auth-java (eg, appy a source credential like ComputeENgineCredential into IdTokenCredential` and then derive the token, thats even better

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see - so they're simply on/off options, not individual separate values. It still feels like I need a couple of hours to get back into this spec to work out what we should do. I'll see if I can find time later in the week.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certainly; pls feel free to ping at anytime; i'd be happy to help where i can.

just note, google-auth-python does idTokens different than in google-auth-java (as much as i'd like uniformity, that ship has sailed and was also subject to language constructs already in use).

for ref:
google-auth-python uses a source credential and then gets an id_token:

from google.auth import compute_engine
creds = compute_engine.IDTokenCredentials( request,
                        target_audience= target_audience)

(as a side note, google-auth-python doesn't even currently use the metadata server to get an id_token...i'm trying to fix that in a different PR here and at the same time apply whatever options i can (at the moment, theyr'e explicitly passed in but as that pr is open, i'll adopt 'options' technique there as well)

in google-auth-java, its inverted we get a source credential (eg. ComputeEngineCredentials), then init an IdTokenCredential and push the source credential to it to derive the id_token

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything i can do to assist here?

(i'm trying to make a version of this land on our official docs and it'd be good to have dotnet support)

btw, i'll also verify if the same changes here would work out of the box if we use the credential in grpc calls:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to find the time to get back to understanding this, basically. @chrisdunelm may be able to as well, of course.

bcoe
bcoe previously approved these changes Nov 12, 2019
@bcoe bcoe dismissed their stale review November 12, 2019 18:08

was just testing permissions.

@AndyClausen
Copy link

Is this ever going to happen? We might just have received this functionality in node (thanks btw), but we're still missing support for it in .net to be able to completely avoid using json keyfiles to communicate with IAP.

@jskeet
Copy link
Collaborator Author

jskeet commented Jan 15, 2020

@AndyClausen: It's definitely on our list of things to look at, and I'm hoping we'll get to it this quarter, but potentially not in exactly this form. Apologies for the delay - it's the normal matter of prioritization between many important things to do.

@AndyClausen
Copy link

No worries Jon, it's just because we are going into production soon, and for that it would be nice to not extract any keyfiles at all for any of our services - so I wanted to see if it was possible. We'll wait patiently then. Thanks for the quick answer :)

@salrashid123
Copy link

@AndyClausen i wrote up a small set of unofficial implementations here which can generate and verify google OIDC tokens:

its ofcourse unofficial (plus, i'm ot that profiicent in dotnet) but hopefully it'd be easy to migrate over to the official one in dotnet once its implemented.

@jskeet i don' tknow how dotnet bootstraps credentials for gRPC but perhaps another enhancement would be to use the OIDC token in grpc transports....so as another unofficial implementation:

@amanda-tarafa
Copy link
Contributor

Closing in favour of #1531

@jskeet jskeet deleted the oidc branch January 18, 2021 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants