-
Notifications
You must be signed in to change notification settings - Fork 193
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: service get account private key for JWT encode #557
Conversation
Hello @razvanphp , thanks for your contribution Can you tell me what the purpose of this PR is? These libraries already support Self-signed JWTs, there's no need to pull the private key out of the credentials to do the signing yourself. |
Sure, there is no way currently to use the PHP SDK nicely for google wallet for creating the signed JWT, please check the linked code here: https://github.com/google-wallet/rest-samples/blob/82f5e79e5f490b5b5175392d3935e4ebbc369a04/php/demo_generic.php#L740-L756 The Walletobjects API need a ServiceAccount, not a temporary access key. I think it's not secure and cool to do something like this:
We should promote the usage of the |
@razvanphp I am still confused. Can you explain to me why you need access to the private key (or why you need to sign JWTs) in the first place? This is an operation that should be handled by the auth library, and the user should never need to do it.
I don't know what a temporary access key is. The Walletobjects client library currently uses service accounts when generating an access token. |
Did you check the linked issues and code? Currently the official wallet samples use the service account json file directly, instead of the
By temporary access key I mean that here: google-auth-library-php/src/OAuth2.php Line 548 in bff9f2d
the library will create a JWT token that expires. Also, the auth lib does not know anything about Origin/CORS, which should be embedded in the JWT signed token. PS: since all other languages expose this method, why not have it in PHP as well? |
@razvanphp I just left a comment on your issue (see google-wallet/rest-samples#112 (comment)). The wallet samples are not using this library correctly. As for creating a JWT as they're doing here, it seems like they're doing custom signing of JWTs for some of their API functionality. This seems fine how they have it. But why do you want to pull the private key from the credentials class instead of how they have it? |
By "they" you mean still google team, just another department, right? 🙂 I would love to educate people on using the SDKs in the correct way, but it seems there is no other way currently without the proposed change in this PR: the custom signing cannot be used without the private key being exposed, so one cannot create valid "Add to wallet" JWT signed links, it's not only about communicating with the google API for wallet, those encoded links are shown to the end user. To answer your question on to "why", is because what you also mentioned, the auth lib is not used correctly, the Wallet service should be passed an instance of |
so all google SDKs do expose the private key, like Java, Go, dotnet, etc but you think PHP should not have it? Why? Is it less secure than in other languages? I thought an SDK is about consistency and convenience... if you are not interested in helping the community using google products, at least do not block the open source vibe of trying to maintain and improve the code. |
I don't have an issue merging this feature - I only want to make sure that I understand what it is you're trying to accomplish to ensure that this feature is actually what is needed for you to do so. |
Ok, I've moved the get private key method in I've also investigated signing the JWT token with it, but in fetchAuthToken, OAuth2::toJwt() method is called without any arguments, which means one cannot add the custom claims like This is how it needs to look: {
"iss": "[email protected]",
"aud": "google",
"origins": [
"www.example.com"
],
"typ": "savetowallet",
"payload": {
"eventTicketClasses": [
{}
]}
} This is how it looks from {
"iss": "[email protected]",
"exp": 1717500734,
"iat": 1717497074,
"scope": "https://www.googleapis.com/auth/wallet_object.issuer",
"sub": "[email protected]"
} Should I add a new method for this in there that does JWT encoding? Or... maybe we can do a breaking change in |
How you had it before (on For example, a user might use this method because they received service account credentials from ADC $credentials = ApplicationDefaultCredentials::getCredentials($scope);
if ($credentials instanceof ServiceAccountCredentials) {
$privateKey = $credentials->getPrivateKey();
// sign the JWT
} But if a user is creating WRT breaking changes, we cannot take a breaking change without cutting a new major release of this library, so we only would do that if it was VERY justifiable. |
hmm, ok, then let's have it in both then:
Next step indeed is to provide a way to generate the JWT tokens without knowing about private key, RS256, etc, but we need to:
What do you think? |
This is already possible using |
@bshaffer please check this PR I just opened: google-wallet/rest-samples#124 What I think it's still missing from the auth lib is a way to create the JWT tokens with custom claims like HERE. Those tokens are needed for google-auth-library-php/src/OAuth2.php Line 548 in e10dc3f
|
as discussed in google-wallet/rest-samples#112 and on par with what the Java SDK already does, we need this to streamline the usage of JWT tokens encoding in the google wallet SDK.
Please tag the release after merge.
Thank you!
BEGIN_COMMIT_OVERRIDE
feat: private key getters on service account credentials (#557)
END_COMMIT_OVERRIDE