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

Clean up Service Account support #211

Closed
dhermes opened this Issue Jul 1, 2015 · 13 comments

Comments

Projects
None yet
3 participants
@dhermes
Contributor

dhermes commented Jul 1, 2015

See the discussion in #207. Though not directly related, it revealed a few things.

  1. There should just be a single service account class with factory constructors to handle PKCS12/P12 vs. JSON keys
  2. It seems that usable auth created a shadowed service_account._ServiceAccountCredentials class that does essentially the same thing that client.SignedJwtAssertionCredentials does
  3. We should better document the difference between a PKCS12/P12 key and a JSON key (and hopefully there is somewhere on the Google official OAuth docs explaining the difference as well)

@dhermes dhermes changed the title from Clean up Service Account Support to Clean up Service Account support Jul 1, 2015

@sr-murthy

This comment has been minimized.

Show comment
Hide comment
@sr-murthy

sr-murthy Jul 1, 2015

Hi,

would you like to me to do the change and make a pull request for this?

Sandeep

sr-murthy commented Jul 1, 2015

Hi,

would you like to me to do the change and make a pull request for this?

Sandeep

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jul 1, 2015

Contributor

No I or some other maintainer can handle it. I just wanted to give a source for the issue. Thanks though!

Contributor

dhermes commented Jul 1, 2015

No I or some other maintainer can handle it. I just wanted to give a source for the issue. Thanks though!

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jul 6, 2015

Contributor

I played around a bit and it seems like P12->JSON is possible but JSON->P12 is not.

The reason is that a P12 key is made up of a PEM certificate and a PEM key, which can be seen via:

from OpenSSL import crypto
with open('keyfile.p12', 'rb') as fh:
    p12 = crypto.load_pkcs12(fh.read(), 'notasecret')
print crypto.dump_privatekey(crypto.FILETYPE_PEM, p12.get_privatekey())
print crypto.dump_certificate(crypto.FILETYPE_PEM, p12.get_certificate())

On the other hand, the JSON key file has a private_key field, but no corresponding certificate field. It may be that the private_key_id field corresponds to a certificate stored internally within Google (or something similar).

This manifests in the first section of the JWT sent (the header). For the P12

{
  "alg": "RS256", 
  "typ": "JWT"
}

vs. for a JSON key (with a private_key_id which for the purposes of demonstration we'll say is DEADBEEF):

{
  "alg": "RS256", 
  "kid": "DEADBEEF", 
  "typ": "JWT"
}

This may not matter if we never use the certificate from the P12 key.

Contributor

dhermes commented Jul 6, 2015

I played around a bit and it seems like P12->JSON is possible but JSON->P12 is not.

The reason is that a P12 key is made up of a PEM certificate and a PEM key, which can be seen via:

from OpenSSL import crypto
with open('keyfile.p12', 'rb') as fh:
    p12 = crypto.load_pkcs12(fh.read(), 'notasecret')
print crypto.dump_privatekey(crypto.FILETYPE_PEM, p12.get_privatekey())
print crypto.dump_certificate(crypto.FILETYPE_PEM, p12.get_certificate())

On the other hand, the JSON key file has a private_key field, but no corresponding certificate field. It may be that the private_key_id field corresponds to a certificate stored internally within Google (or something similar).

This manifests in the first section of the JWT sent (the header). For the P12

{
  "alg": "RS256", 
  "typ": "JWT"
}

vs. for a JSON key (with a private_key_id which for the purposes of demonstration we'll say is DEADBEEF):

{
  "alg": "RS256", 
  "kid": "DEADBEEF", 
  "typ": "JWT"
}

This may not matter if we never use the certificate from the P12 key.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jul 6, 2015

Contributor

As it turns out, JSON->P12 can be done as well (at least for getting an access token). It's as easy as

  1. Creating a PKCS12 object
  2. Adding the JSON private key to the object
  3. Exporting the object to a file with the given passphrase
p12_obj = crypto.PKCS12()  # 1
p12_obj.set_privatekey(crypto.load_privatekey(  # 2
    crypto.FILETYPE_PEM, private_key))
with open(JSON_AS_P12, 'wb') as fh:  # 3
    fh.write(p12_obj.export(passphrase=PASSPHRASE))

See my gist at the latest revision to see all cases covered.

Contributor

dhermes commented Jul 6, 2015

As it turns out, JSON->P12 can be done as well (at least for getting an access token). It's as easy as

  1. Creating a PKCS12 object
  2. Adding the JSON private key to the object
  3. Exporting the object to a file with the given passphrase
p12_obj = crypto.PKCS12()  # 1
p12_obj.set_privatekey(crypto.load_privatekey(  # 2
    crypto.FILETYPE_PEM, private_key))
with open(JSON_AS_P12, 'wb') as fh:  # 3
    fh.write(p12_obj.export(passphrase=PASSPHRASE))

See my gist at the latest revision to see all cases covered.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jul 6, 2015

Contributor

Some notes on differences and similarities between client.SignedJwtAssertionCredentials and service_account._ServiceAccountCredentials.

Similarities:

  • Both define class constant MAX_TOKEN_LIFETIME_SECS = 3600 # 1 hour in seconds
  • Both inherit directly from client.AssertionCredentials
  • Both use (aud, scope, iat, exp, iss) and anything from kwargs as the keys in the dictionary used for the body of the JWT

Differences

  • Constructors are very different
  • Most positional (P) and keyword (K) arguments map to one another
```
service_account_id(1P)     ==
service_account_email(2P)  == service_account_name(1P)
private_key_id(3P)         ==
private_key_pkcs8_text(4P) == private_key(2P)
scopes(5P)                 == scope(3P)
                           == private_key_password(4K)
user_agent(6K)             == user_agent(5K)
token_uri(7K)              == token_uri(6K)
revoke_uri(8K)             == revoke_uri(7K)
kwargs                     == kwargs
```
  • Arguments that are novel
    - private_key_password is unique to a PKCS12, but always seems to be 'notasecret'
    - private_key_id is unique to a JSON key (see above)
    - service_account_id is unique to a JSON key (client_id in the payload) and seems to just be the email remixed. For example, the svc. acct. email FOO@developer.gserviceaccount.com seems to typically correspond to FOO.apps.googleusercontent.com.
  • Crypto used is different. P12 uses _RequireCryptoOrDie() (pyCrypto or OpenSSL) while only pyasn1 / pyasn1_modules / rsa are used for JSON keys
  • The first segment (header) of the JWT in a P12 key only uses {"alg": "RS256", "typ": "JWT"} while the JSON key also sends kid (the private_key_id) (It's unclear if it is necessary)
  • _ServiceAccountCredentials.service_account_email is a property (getter only, no setter) while SignedJwtAssertionCredentials.service_account_name is just an attribute
  • _ServiceAccountCredentials defines sign_blob while SignedJwtAssertionCredentials does not (though crypt provides plenty of methods for signing blobs)
  • SignedJwtAssertionCredentials.from_json provides serialization while _ServiceAccountCredentials.serialization_data provides a non-standard serialization interface
  • SignedJwtAssertionCredentials._generate_assertion doesn't opaquely generate the JWT, the header is generated by the call to oauth2client.crypt.make_signed_jwt while _ServiceAccountCredentials._generate_assertion creates the JWT in place (it needs to since it adds a custom kid key to the first segment)
  • _ServiceAccountCredentials defines create_scoped and create_scoped_required while SignedJwtAssertionCredentials relies on the do-nothing subclass

Noteworthy

  • Since service_account._ServiceAccountCredentials is not public (i.e. the class name starts with _) we have no obligation to keep it around.
  • If we wanted to just treat all inputs like a JSON key file it would require that
  1. PyCrypto (or similar) is installed to convert from P12 to the PEM key
  2. The private_key_id for the key is required (listed as "Certificate fingerprints" in Cloud Console). Though it's possible that kid is not needed in the JWT.

ISTM the "best" path forward is to do one of the following two things

  1. Beef up SignedJwtAssertionCredentials to handle both P12 and JSON keyfiles and make an alias with a better name in the module
  2. Leave SignedJwtAssertionCredentials as is and define a more usable subclass with factory constructors for each type of
Contributor

dhermes commented Jul 6, 2015

Some notes on differences and similarities between client.SignedJwtAssertionCredentials and service_account._ServiceAccountCredentials.

Similarities:

  • Both define class constant MAX_TOKEN_LIFETIME_SECS = 3600 # 1 hour in seconds
  • Both inherit directly from client.AssertionCredentials
  • Both use (aud, scope, iat, exp, iss) and anything from kwargs as the keys in the dictionary used for the body of the JWT

Differences

  • Constructors are very different
  • Most positional (P) and keyword (K) arguments map to one another
```
service_account_id(1P)     ==
service_account_email(2P)  == service_account_name(1P)
private_key_id(3P)         ==
private_key_pkcs8_text(4P) == private_key(2P)
scopes(5P)                 == scope(3P)
                           == private_key_password(4K)
user_agent(6K)             == user_agent(5K)
token_uri(7K)              == token_uri(6K)
revoke_uri(8K)             == revoke_uri(7K)
kwargs                     == kwargs
```
  • Arguments that are novel
    - private_key_password is unique to a PKCS12, but always seems to be 'notasecret'
    - private_key_id is unique to a JSON key (see above)
    - service_account_id is unique to a JSON key (client_id in the payload) and seems to just be the email remixed. For example, the svc. acct. email FOO@developer.gserviceaccount.com seems to typically correspond to FOO.apps.googleusercontent.com.
  • Crypto used is different. P12 uses _RequireCryptoOrDie() (pyCrypto or OpenSSL) while only pyasn1 / pyasn1_modules / rsa are used for JSON keys
  • The first segment (header) of the JWT in a P12 key only uses {"alg": "RS256", "typ": "JWT"} while the JSON key also sends kid (the private_key_id) (It's unclear if it is necessary)
  • _ServiceAccountCredentials.service_account_email is a property (getter only, no setter) while SignedJwtAssertionCredentials.service_account_name is just an attribute
  • _ServiceAccountCredentials defines sign_blob while SignedJwtAssertionCredentials does not (though crypt provides plenty of methods for signing blobs)
  • SignedJwtAssertionCredentials.from_json provides serialization while _ServiceAccountCredentials.serialization_data provides a non-standard serialization interface
  • SignedJwtAssertionCredentials._generate_assertion doesn't opaquely generate the JWT, the header is generated by the call to oauth2client.crypt.make_signed_jwt while _ServiceAccountCredentials._generate_assertion creates the JWT in place (it needs to since it adds a custom kid key to the first segment)
  • _ServiceAccountCredentials defines create_scoped and create_scoped_required while SignedJwtAssertionCredentials relies on the do-nothing subclass

Noteworthy

  • Since service_account._ServiceAccountCredentials is not public (i.e. the class name starts with _) we have no obligation to keep it around.
  • If we wanted to just treat all inputs like a JSON key file it would require that
  1. PyCrypto (or similar) is installed to convert from P12 to the PEM key
  2. The private_key_id for the key is required (listed as "Certificate fingerprints" in Cloud Console). Though it's possible that kid is not needed in the JWT.

ISTM the "best" path forward is to do one of the following two things

  1. Beef up SignedJwtAssertionCredentials to handle both P12 and JSON keyfiles and make an alias with a better name in the module
  2. Leave SignedJwtAssertionCredentials as is and define a more usable subclass with factory constructors for each type of
@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jul 6, 2015

Contributor

Changed

diff --git a/oauth2client/service_account.py b/oauth2client/service_account.py
index d1d1d89..8120c6c 100644
--- a/oauth2client/service_account.py
+++ b/oauth2client/service_account.py
@@ -62,7 +62,6 @@ class _ServiceAccountCredentials(AssertionCredentials):
     header = {
         'alg': 'RS256',
         'typ': 'JWT',
-        'kid': self._private_key_id
     }

     now = int(time.time())

and tested that minting a token worked, and not sending a kid made no difference at all.

Contributor

dhermes commented Jul 6, 2015

Changed

diff --git a/oauth2client/service_account.py b/oauth2client/service_account.py
index d1d1d89..8120c6c 100644
--- a/oauth2client/service_account.py
+++ b/oauth2client/service_account.py
@@ -62,7 +62,6 @@ class _ServiceAccountCredentials(AssertionCredentials):
     header = {
         'alg': 'RS256',
         'typ': 'JWT',
-        'kid': self._private_key_id
     }

     now = int(time.time())

and tested that minting a token worked, and not sending a kid made no difference at all.

@sr-murthy

This comment has been minimized.

Show comment
Hide comment
@sr-murthy

sr-murthy Jul 7, 2015

I'm actually using service_account._ServiceAccountCredentials in my code to create credentials with which to authorized and get a gspread client. Why couldn't this just be made "public" as well?

What is missing is a simple public method for creating service account credentials using only five pieces of data: the client ID, client email, private key ID, private key, and authentication scopes. If you can make such a method then this would help greatly, and this information can often be loaded not just from raw JSON files but via via internal custom config-type objects (brand in our case). In my case I had to do this by explicitly calling _ServiceAccountCredentials because I realised it was the most direct and shortest way of getting service credentials for gspread clients.

The complexity of loading authentication information from a file is obvious from the code for _get_application_default_credential_from_file(https://google.github.io/oauth2client/_modules/oauth2client/client.html) - too many things can go wrong with the file before you can even starting decoding the JSON to Python.

sr-murthy commented Jul 7, 2015

I'm actually using service_account._ServiceAccountCredentials in my code to create credentials with which to authorized and get a gspread client. Why couldn't this just be made "public" as well?

What is missing is a simple public method for creating service account credentials using only five pieces of data: the client ID, client email, private key ID, private key, and authentication scopes. If you can make such a method then this would help greatly, and this information can often be loaded not just from raw JSON files but via via internal custom config-type objects (brand in our case). In my case I had to do this by explicitly calling _ServiceAccountCredentials because I realised it was the most direct and shortest way of getting service credentials for gspread clients.

The complexity of loading authentication information from a file is obvious from the code for _get_application_default_credential_from_file(https://google.github.io/oauth2client/_modules/oauth2client/client.html) - too many things can go wrong with the file before you can even starting decoding the JSON to Python.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jul 7, 2015

Contributor

@sr-murthy Thanks for letting us know. The plan right now is to make one class with two easy to use factory methods. For example:

p12_creds = ServiceAccountCredentials.from_p12(client_email, p12_key_path)
json_creds = ServiceAccountCredentials.from_json(json_key_path)

Also, please don't depend on private / protected methods, functions, or classes as they may be deprecated / removed in future releases.

Contributor

dhermes commented Jul 7, 2015

@sr-murthy Thanks for letting us know. The plan right now is to make one class with two easy to use factory methods. For example:

p12_creds = ServiceAccountCredentials.from_p12(client_email, p12_key_path)
json_creds = ServiceAccountCredentials.from_json(json_key_path)

Also, please don't depend on private / protected methods, functions, or classes as they may be deprecated / removed in future releases.

@anthmgoogle

This comment has been minimized.

Show comment
Hide comment
@anthmgoogle

anthmgoogle Jul 8, 2015

Some context.

The "private_key_id" can in theory apply to p12 keys also, but you would have to provide it as a separate input, e.g. as an optional 3rd parameter to "from_p12" because it is not in that file. As you discovered, nothing breaks if you remove it, but we were advised to put this in by security experts. Apparently it can help performance and diagnostics on the server side, as it can identify which of N keys you are talking about and can be used to do things like revoke the key in the UI.

The pattern suggested here of making _ServiceAccountCredentials public and adding P12 support sounds good here.

I would suggest the following factories:

from_p12file(client_email, p12_key_path);
from_jsonFile(json_key_path);
from_json(json)

The from_json would be a dictionary in memory.

One other change that I would add here is that we should remove all reference to the "service account id". This is the "client id" of the service account and this is being removed from the console UI and there is a desire to eventually remove this from the json file as well.

anthmgoogle commented Jul 8, 2015

Some context.

The "private_key_id" can in theory apply to p12 keys also, but you would have to provide it as a separate input, e.g. as an optional 3rd parameter to "from_p12" because it is not in that file. As you discovered, nothing breaks if you remove it, but we were advised to put this in by security experts. Apparently it can help performance and diagnostics on the server side, as it can identify which of N keys you are talking about and can be used to do things like revoke the key in the UI.

The pattern suggested here of making _ServiceAccountCredentials public and adding P12 support sounds good here.

I would suggest the following factories:

from_p12file(client_email, p12_key_path);
from_jsonFile(json_key_path);
from_json(json)

The from_json would be a dictionary in memory.

One other change that I would add here is that we should remove all reference to the "service account id". This is the "client id" of the service account and this is being removed from the console UI and there is a desire to eventually remove this from the json file as well.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jul 8, 2015

Contributor

Thanks for chiming in! That's fine with me. Re from_json, that is problematic since this name is already used throughout the oauth2client library for de-serialization (UPDATE: though the concept works, just needs a different name).

Contributor

dhermes commented Jul 8, 2015

Thanks for chiming in! That's fine with me. Re from_json, that is problematic since this name is already used throughout the oauth2client library for de-serialization (UPDATE: though the concept works, just needs a different name).

@anthmgoogle

This comment has been minimized.

Show comment
Hide comment
@anthmgoogle

anthmgoogle Jul 9, 2015

OK. We should find another name like from_dictionary for this, but the function is needed. Code that reads a file that could be either user or a service account needs to forward the dictionary it parses on to this method.

anthmgoogle commented Jul 9, 2015

OK. We should find another name like from_dictionary for this, but the function is needed. Code that reads a file that could be either user or a service account needs to forward the dictionary it parses on to this method.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Jul 9, 2015

Contributor

Totally agree 👍

Contributor

dhermes commented Jul 9, 2015

Totally agree 👍

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Feb 1, 2016

Contributor

@nathanielmanistaatgoogle I'm close to "done" here. Will send a non-merge-able PR and ping you on it by EOD. Then we can discuss how to break it up for review.

Contributor

dhermes commented Feb 1, 2016

@nathanielmanistaatgoogle I'm close to "done" here. Will send a non-merge-able PR and ping you on it by EOD. Then we can discuss how to break it up for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment