-
Notifications
You must be signed in to change notification settings - Fork 227
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
feat: idtoken user credential #469
feat: idtoken user credential #469
Conversation
idTokenWithAudience method not tested (I don't know how to do this to be useful) Depend on this update googleapis/google-http-java-client#1100
|
||
private static final String GRANT_TYPE = "refresh_token"; | ||
private static final String PARSE_ERROR_PREFIX = "Error parsing token refresh response. "; | ||
private static final long serialVersionUID = -4800758775038679176L; | ||
public static final String GOOGLE_CLIENT_ID = "32555940559.apps.googleusercontent.com"; | ||
public static final String GOOGLE_CLIENT_SECRET = "ZmssLNjJy2998hD4CTg2ejr2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's arguable how secret these are when they ship in binaries, but in the past security team has advised me in other projects not to put these values in github but to bundle them in at build time.
Please file a ticket for an internal security review on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, Google Client_id and secret (Yes, Google, because it's not my secrets, but Google secrets!) are clear and it can seem not safe. But, I don't think that is a real "secret".
I'm not sure to catch what change if you add only these values at build time? The library will be ship with the secret in plain text and a simple IDE allow you to easily find them..
Here how I found the Google secrets
gcloud config set log_http_redact_token false
gcloud auth print-identity-token --log-http
You can see the request body in clear with this client_id and client_secret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while (it.hasNext()) { | ||
customScopes += "+" + it.next(); | ||
} | ||
tokenRequest.set("scope", customScopes); |
There was a problem hiding this comment.
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 documentation for this parameter against https://oauth2.googleapis.com/token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, the scope parameters is not documented -> https://developers.google.com/identity/protocols/oauth2/web-server#offline
But required to scope correctly the generated token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chingor13 I don't know what to say! The request needs it to work.
Of course, it's not documented, and I don't know why, maybe a mistake in the current documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible we need to update the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the scope parameter on https://developers.google.com/identity/protocols/oauth2/web-server#offline
Make sure you're looking at the HTTP/RES documents and not at one of the language specific tabs.
Any updates on this? Any helps/comments that I can bring to you? |
Let me see if I can pass this on to the auth PM who might know more about this. |
@@ -89,13 +102,21 @@ private UserCredentials( | |||
String clientSecret, | |||
String refreshToken, | |||
AccessToken accessToken, | |||
Collection<String> scopes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this collection is passed in from outside, this could be a security issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exact same behavior as ServiceAccountCredentials class.
ServiceAccountCredentials have protected constructor. In this class the constructor is private. Can be only called by the Builder.
while (it.hasNext()) { | ||
customScopes += "+" + it.next(); | ||
} | ||
tokenRequest.set("scope", customScopes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible we need to update the docs.
@@ -331,6 +339,34 @@ public void equals_false_refreshToken() throws IOException { | |||
assertFalse(otherCredentials.equals(credentials)); | |||
} | |||
|
|||
@Test | |||
public void equals_false_scopes() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equalsFalseScopes per google style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept the naming consistent with the rest of the file. I can update it, but it will be the only one like this. Or I can update the whole file if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google style overrides pre-existing violations. You don't need to fix old violations, but please don't introduce new ones.
@@ -331,6 +339,34 @@ public void equals_false_refreshToken() throws IOException { | |||
assertFalse(otherCredentials.equals(credentials)); | |||
} | |||
|
|||
@Test | |||
public void equals_false_scopes() throws IOException { | |||
final URI tokenServer1 = URI.create("https://foo1.com/bar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google style doesn't use final on local variables unless it's mandatory for a closure or inner class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept the style consistent with the rest of the file. I can update it, but it will be the only one like this. Or I can update the whole file if needed.
(Is the best skill of a developer copy-paste? and then adapt)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@silvolu Can you look into this or assign someone to do it? Thanks. |
Fix forgot commented code
Working on Bazel, I got bit by this one when trying to call a bazel cache running behing Cloud Run. Talk about timing! |
The issue #472 has been reviewed and closed (no secret to keep secret). So we can go ahead! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge with master
@@ -331,6 +339,34 @@ public void equals_false_refreshToken() throws IOException { | |||
assertFalse(otherCredentials.equals(credentials)); | |||
} | |||
|
|||
@Test | |||
public void equals_false_scopes() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google style overrides pre-existing violations. You don't need to fix old violations, but please don't introduce new ones.
@@ -331,6 +339,34 @@ public void equals_false_refreshToken() throws IOException { | |||
assertFalse(otherCredentials.equals(credentials)); | |||
} | |||
|
|||
@Test | |||
public void equals_false_scopes() throws IOException { | |||
final URI tokenServer1 = URI.create("https://foo1.com/bar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
Hi, Apologies for the delay in replying here. @guillaumeblaquiere thanks for the contribution and @elharo thanks for spending time reviewing this. We need to rewrite this change, as there are some core things that are not correct:
I'll do it personally or find someone that can take care of it asap. We'll add both of you to the PR review so you can see what I mean. |
Hello @silvolu, yes, you can generate an id_token with the client_id and the client_secret contained in the user credential (obtained with a BUT you can't use this id_token to be authorized on private Cloud Functions, private Cloud Run or on IAP. Only these hardcoded "Google" client_id and the client_secret allow you to generate a valid id_token, as the My initial target is to allow the OAuth client library to do this because today we need a service account key file, on the developer machine, to be able to generate a correct id_token accepted by the serverless product on GCP. And a service account key file is a nightmare in term of security (with internal and external developer, because the file are sent by mail, or committed externally,... I wrote an article on this) Therefore, IMO don't waste your time to implement the id_token generation on user credential, it's useless! (at least for me!) Anyway, you are right about the scope, we can remove them. @elharo sorry for all the discussions around this useless addition! I sent an update because I believe in the usefulness of this feature! Finally, I don't understand what is not correct in the token acquisition! I copied the logic of the ServiceAccountCredentials.idTokenWithAudience() method. How I can do this differently? |
For what it's worth, the client id and secret are hardcoded in google-cloud-sdk itself and the official |
just a couple comments... If you want to acquire an id_token intended for cloud run where the root-source credential is a user's account, you can use impersonation and then idtokencredentials. the gcloud equivalent of that is $ gcloud auth print-identity-token --audiences=https://foo.bar --impersonate-service-account impersonated-account@fabled-ray-104117.iam.gserviceaccount.com will give an id token for the service account w/o holding a key locally {
"aud": "https://foo.bar",
"azp": "102101550834200708568",
"exp": 1601497491,
"iat": 1601493891,
"iss": "https://accounts.google.com",
"sub": "102101550834200708568"
} if you want the equivalent in java, its // for ImpersonatedCredentials
ImpersonatedCredentials imCreds = ImpersonatedCredentials.create(saCreds,
"impersonated-account@project.iam.gserviceaccount.com", null,
Arrays.asList("https://www.googleapis.com/auth/cloud-platform"), 300);
IdTokenCredentials tokenCredential = IdTokenCredentials.newBuilder()
.setIdTokenProvider(imCreds)
.setTargetAudience(targetAudience)
.setOptions(Arrays.asList(ImpersonatedCredentials.INCLUDE_EMAIL))
.build(); (replace |
@salrashid123, Yes I know the impersonation. But, it's not a solution in my ultimate dream! In fact, I would like to use the same code on my local environment, with my user credential, and on Google Cloud environment, with service account credentials provided by metadata servers. I mean, based on ADC (Application Default Credential) I can achieve this when I have to use all the Google APIs, because they only require an access_token, and it's perfect. But, for Cloud Run, Cloud Functions and App Engine (behind IAP), I can't. And it's a problem for the development of services that consume existing and protected deployments.
So, this PR is for solving these issues and finally make the development easier, safer and more secure. |
@salrashid123 My understanding was that the point of this PR was to avoid having custom code for situations where the default application credentials are not service account based. It's in everyone's interest to keep the security code as minimal as possible to reduce the attack surface. |
ADC based credentials uses a different client_id/secret pair than gcloud (i.,e an id token derived from ADC usercreds isn't permissiable into cloud run so...if/until that happens, this PR woudn't really help (since the aud field is immutable)... interms of why the tokenflow isn't working in anycase, change google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/UserCredentials.java Line 72 in 9d1b142
to "client_id": "764086051850-6qr4p6gpi6hn506pt8ejuq83di341hur.apps.googleusercontent.com",
"client_secret": "d-FL95Q19q7MQmFpd7hHD0Ty", ...that shoudl return an id_token eg # use refresh_token from
gcloud auth application-default login
cat ~/.config/gcloud/application_default_credentials.json
export REFRESH_TOKEN=...
export CLOUDSDK_CLIENT_ID=764086051850-6qr4p6gpi6hn506pt8ejuq83di341hur.apps.googleusercontent.com
export CLOUDSDK_CLIENT_NOTSOSECRET=d-FL95Q19q7MQmFpd7hHD0Ty
curl -s -X POST -H "Content-type:application/x-www-form-urlencoded" "https://oauth2.googleapis.com/token?client_id=$CLOUDSDK_CLIENT_ID&client_secret=$CLOUDSDK_CLIENT_NOTSOSECRET&refresh_token=$REFRESH_TOKEN&grant_type=refresh_token" then use that id token agains run as the btw, the client_id and secret when used in installed application mode are not considered secrets.. |
@salrashid123, the id_token generated with the user credential, user client_id and user client_secret generate an id_token, but it is not accepted by Cloud Run and Cloud Function. I guess it's because the audience is not correct. That's why it's important to use the client_id and the client_id allowlisted on GFE to allow this. Even if the audience isn't correct, the special Google sauce makes the call accepted. Anyway, I confirm, IAP don't work with this token. |
correct, Clodu Run and GCF has a special setting to allow those gcloud cli based id_tokens...it wont' work on other services like IAP at all. Run and GCF doesn't have the same allow for ADC based creds (i do not know why ADC isn't on that list but i suspect gcloud cli was done not for operational use but to just quickly directly test from cli) digression: anyway, we spoke about a proposal to enhance ADC such that it will delegate to some arbitrary external provider for credentials. If its acceptabl in this scenario for you to set the standared env var it'd be something like this create a config file that defines what to execute to get credentials $ cat /path/to/external_cred.json
{
"type": "external_command",
"command": ["/path/to/some/cred_binary", "get_id_token"]
} then you set the standard env var and run your code as normal with unifor GoogleCredentials $ GOOGLE_APPLICATION_CREDENTIALS=/path/to/external_cred.json
$ ./myapplication in your case, the command would be something like "command": ["gcloud", "auth", "print-identity-token"] the intent is for this to populate a GoogleCredential automatically with the output of the binary (eg, id_token)...from there you need to use that credential to make an httpcall here's variation of that idea but baked directly into an unsupported library used for accesstokens. |
Can you provide me a status on this PR? If it is not accepted, I have another idea. |
help: I'm not a super star in Git and I don't know why my commit comments are rejected. |
Hi @guillaumeblaquiere , I have some luck using user account credentials to obtain an id_token to authenticate to IAP, without using google cloud sdk or it's client_id/secret. My use case is that I need to develop a The following (in python) is working for App Engine behind IAP but not for Cloud Run (I haven't tried Cloud Function):
However, when I use the same steps for authenticating to Cloud Run, I get error in the last step when I set audience to cloud run url: RefreshError: ('invalid_audience: Audience is not a valid client ID.', '{\n "error": "invalid_audience",\n "error_description": "Audience is not a valid client ID."\n}') Finally, here's what I'm trying to say/guess:
This solution only works for IAP and requires user interaction, which suits my use case because the tool I'm developing is for interactive use cases. Do hope Cloud Run team can support user account auth in the future (As of now, I think using client_id/secret of the googlecloudsdk is more of an irregularity as @salrashid123 pointed out, the client_id of googlecloudsk is specifically added to the special whitelist, also it feels like blurring the identities of auth lib vs googlecloudsdk) PS. this flow is described in programmatic authentication Also, I'm not a security person in anyway and very happy to be corrected. |
yeah, its kinda irregular what cloud run, gcf did (i.,e allowlisted this specific the flow you have would allow you to take a further step and echange for an audience of your choosing. if you want to do that, its a the flow you described above here: https://github.com/salrashid123/oauth2oidc << note, thats unsupported by google! re: using the token for cloud run... |
Hi @salrashid123, re your comments:
Did you manage to set arbitrary audience value with user account? With service account, auth-library supports setting audience to cloud run url. But if I use user account, I could only exchange for an audience that is a valid client_id within the same GCP project (as you also pointed out in your oauth2oidc repo), but not any audience of my choosing. If I set audience to my cloud run url, I get I guess I could go further by copy/paste googlecloudsdk client id and use them in my own code, but it feels like phishing, it doesn't feel right... |
aribtrary isn't the right term i shoudve picked there. for the user-creds, you can exchange it using the client flow for just audiences that'd share the same project (i.,e use usercreds for an idtoken that you can use on IAP within the same project). Meaning its a very specific capability here of limited applications. |
What's the final decision on this? I'm still unclear what's the best practice from security standpoint when running tests in services that invoke both GCP APIs and Cloud Run services...
What does Google suggest here? I still claim that the ideal option would be:
Until something like this PR is implemented, I don't see how that could work. |
Hey folks, gentle ping :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections. I would like @silvolu to make a decision about this.
as above, the clientId,secret isnt' a problem..but the use of these idtokens is for a unique usecase that cloud run happens to support: it alllows a user's idtokens derived from gcloud for access regardless of the audience value. Its not a standard thing that'd be supported elsewhere (in a way, its misusing the idtoken thats for user-auth 3LO flow). IMO, i do not think the pr should be committed (even if it makes local testing of a bit easier) |
@guillaumeblaquiere IdToken support has been added to UserCredentials with another PR: #650 If some scenario is not yet addressed, please open a separate issue. |
@TimurSadykov I don't understand the PR #650 . As described here, the idToken is not compliant with Cloud Run (and I'm sure with Cloud Functions and also with IAP). You have now an id_token, but you can't use it. Great feature!! |
the last bit isn't really helpful and i'd say neither that specific error message. I'm unaware of what condition would give rise to not getting an that pr provides the the
both of which you can use to call cloud run package com.test;
import com.google.auth.oauth2.GoogleCredentials;
import com.google.auth.oauth2.IdTokenCredentials;
import com.google.auth.oauth2.IdTokenProvider;
import com.google.auth.oauth2.UserCredentials;
import com.google.auth.oauth2.ServiceAccountCredentials;
import com.google.api.client.http.GenericUrl;
import com.google.api.client.http.HttpRequest;
import com.google.api.client.http.HttpResponse;
import com.google.api.client.http.HttpTransport;
import com.google.auth.http.HttpCredentialsAdapter;
import com.google.api.client.http.javanet.NetHttpTransport;
import java.io.FileInputStream;
import java.io.File;
public class Main {
public static void main(String[] args) {
Main tc = new Main();
}
public Main() {
try
{
//FileInputStream inputStream = new FileInputStream(new File("/path/to/svc_account.json"));
//ServiceAccountCredentials credentials = ServiceAccountCredentials.fromStream(inputStream);
FileInputStream userStream = new FileInputStream(new File("/home/srashid/.config/gcloud/application_default_credentials.json"));
UserCredentials credentials = UserCredentials.fromStream(userStream);
String audience_cloud_run_url = "https://myapp-6w42z6vi3q-uc.a.run.app/";
// setTargetAudience value is ignored with UserCredentials.
// for ADC based user creds, its always "aud": "764086051850-6qr4p6gpi6hn506pt8ejuq83di341hur.apps.googleusercontent.com",
IdTokenCredentials tokenCredential = IdTokenCredentials.newBuilder().setIdTokenProvider(credentials).setTargetAudience(audience_cloud_run_url).build();
tokenCredential.refresh();
System.out.println("Cred: " + tokenCredential.getAccessToken());
GenericUrl genericUrl = new GenericUrl(audience_cloud_run_url);
HttpCredentialsAdapter adapter = new HttpCredentialsAdapter(tokenCredential);
HttpTransport transport = new NetHttpTransport();
HttpRequest request = transport.createRequestFactory(adapter).buildGetRequest(genericUrl);
request.setThrowExceptionOnExecuteError(false);
HttpResponse response = request.execute();
String r = response.parseAsString();
System.out.println(r);
}
catch (Exception ex) {
System.out.println("Error: " + ex);
}
}
} |
@salrashid123 I'm gonna fix the message removing the last bit, we didn't know the ADC Client ID was also accepted for Run/GCF auth. The only reason devs would see that error message is if they create a representation of an ADC user credentials without requiring one of the scopes that would add an id token (openid or userinfo.profile), and we thought that instead of explaining the theory, the best thing we could do was telling them potential solutions. If you have suggestions on how to reformulate it, let me know :) @guillaumeblaquiere the user credentials in this library are meant to generate credentials from user credentials acquired from the gcloud CLI, using the application default credentials flow. As @salrashid123 explained, to use with Run/GCF you'd need to run "gcloud auth login --update-adc" (or now we know "gcloud auth application-default login" as well :) ), and then just acquire default credentials from the client library. |
Thanks @silvolu and @salrashid123. I will have a try and let you know |
Fixes #468
Test on method UserCredential.idTokenWithAudience(...) hasn't been done (I don't know how to do it useful)
Required googleapis/google-http-java-client#1100 to work properly (UrlEncodedContent need to be updated to be compliant with the API call performed)