googleapis / google-auth-library-php

Google Auth Library for PHP
http://googleapis.github.io/google-auth-library-php/
Apache License 2.0
1.34k stars 191 forks source link

feat: service get account private key for JWT encode #557

Closed razvanphp closed 4 months ago

razvanphp commented 6 months ago

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 (https://github.com/googleapis/google-auth-library-php/pull/557) END_COMMIT_OVERRIDE

bshaffer commented 6 months ago

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.

razvanphp commented 6 months ago

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:

$serviceAccount = json_decode(file_get_contents($this->keyFilePath), true);

We should promote the usage of the ServiceAccountCredentials instance, but we can't without this change, as @stephenmcd mentioned here.

bshaffer commented 6 months ago

@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.

The Walletobjects API need a ServiceAccount, not a temporary access key.

I don't know what a temporary access key is. The Walletobjects client library currently uses service accounts when generating an access token.

razvanphp commented 6 months ago

Did you check the linked issues and code? Currently the official wallet samples use the service account json file directly, instead of the ServiceAccountCredentials class to create the JWT tokens.

    // The service account credentials are used to sign the JWT
    $serviceAccount = json_decode(file_get_contents($this->keyFilePath), true);

    // Create the JWT as an array of key/value pairs
    $claims = [
      'iss' => $serviceAccount['client_email'],
      'aud' => 'google',
      'origins' => ['www.example.com'],
      'typ' => 'savetowallet',
      'payload' => $objectsToAdd
    ];

    $token = JWT::encode(
      $claims,
      $serviceAccount['private_key'],
      'RS256'
    );

    print "Add to Google Wallet link\n";

    return "https://pay.google.com/gp/v/save/{$token}";

By temporary access key I mean that here: https://github.com/googleapis/google-auth-library-php/blob/bff9f2d01677e71a98394b5ac981b99523df5178/src/OAuth2.php#L548 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?

bshaffer commented 6 months ago

@razvanphp I just left a comment on your issue (see https://github.com/google-wallet/rest-samples/issues/112#issuecomment-2145867845). 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?

razvanphp commented 6 months ago

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 ServiceAccountCredentials instead of just a file path. but then one cannot sign the links anymore because they don't have acces to the private key, hence the chicken-egg-problem.

razvanphp commented 6 months ago

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.

bshaffer commented 6 months ago

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.

razvanphp commented 5 months ago

Ok, I've moved the get private key method in ServiceAccountJwtAccessCredentials instead, as it makes more sense to be there and limits the scope of the change.

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 aud, origins, typ and payload. Also, we need a way to disable token expiration (since default is 3600); add to wallet links should not expire & aud + scope should not be both present.

This is how it needs to look:

{
  "iss": "blabla@my-user.iam.gserviceaccount.com",
  "aud": "google",
  "origins": [
    "www.example.com"
  ],
  "typ": "savetowallet",
  "payload": {
    "eventTicketClasses": [
      {}
    ]}
}

This is how it looks from toJwt():

{
  "iss": "blabla@my-user.iam.gserviceaccount.com",
  "exp": 1717500734,
  "iat": 1717497074,
  "scope": "https://www.googleapis.com/auth/wallet_object.issuer",
  "sub": "blabla@my-user.iam.gserviceaccount.com"
}

Should I add a new method for this in there that does JWT encoding? Or... maybe we can do a breaking change in ServiceAccountJwtAccessCredentials constructor (since it anyway does not respect the order from ServiceAccountCredentials) to pass a config array, merged & passed to OAuth2 constructor.

bshaffer commented 5 months ago

How you had it before (on ServiceAccountCredentials instead of ServiceAccountJwtAccessCredentials was more appropriate, as the end-user never really has access to ServiceAccountJwtAccessCredentials (these are created and used internally by ServiceAccountCredentials because they're thought of a "subset" of the same credential).

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 ServiceAccountJwtAccessCredentials directly, I don't see the advantage to calling getPrivateKey over just using the supplied key that was passed into the constructor for the credentials. Either way the user has to have created it beforehand.

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.

razvanphp commented 5 months ago

hmm, ok, then let's have it in both then:

  1. both of them are instances of CredentialsLoader, people can instantiate them
  2. Java, Go and all other languages SDK have it too
  3. at least this way we (as in wallet API users) don't have to use the keyPath JSON array directly, and we can make use of the ServiceAccountCredentials object instead.

Next step indeed is to provide a way to generate the JWT tokens without knowing about private key, RS256, etc, but we need to:

  1. adjust as I said above, we can make use of getAdditionalClaims, but we need to expose those in constructor and/or setter of ServiceAccountCredentials
  2. this way we can add the payload, typ and origins to the assertions.
  3. we need a way to disable expiration in oAuth2::toJwt(), maybe by doing setExpiry(-1) and checking this to exclude exp and iat from the assertions.
  4. expose OAuth2 instance in ServiceAccountCredentials or provide a vanity toJwt() function.

What do you think?

bshaffer commented 4 months ago

Next step indeed is to provide a way to generate the JWT tokens without knowing about private key, RS256, etc

This is already possible using ServiceAccountCredentials and ServiceAccountJwtAccessCredentials. I am once again not sure what your use-case is that you'd need to do this. See my comments in the wallet sample.

razvanphp commented 2 months ago

@bshaffer please check this PR I just opened: https://github.com/google-wallet/rest-samples/pull/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 Add to Wallet buttons and they need to be without expiration (which is now default in the OAuth2::toJwt() now)

https://github.com/googleapis/google-auth-library-php/blob/e10dc3fb43b6f76c717d10f553104431181a7686/src/OAuth2.php#L548