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

Added self signed jwt service account credentials #503

Merged
merged 1 commit into from May 18, 2016

Conversation

Projects
None yet
8 participants
@kpayson64
Contributor

kpayson64 commented Apr 22, 2016

Initial self signed jwt credential support #252. Here is my thinking with this initial API:

-SelfSignedJWTCredentials class should not be on the OAuth2Client inheritance hierarchy, there are many oauth2 specific methods (like refresh) that don't apply to self signed credentials.
-SelfSignedJWTCredentials can be placed in the service_account module, SelfSignedJWTCredentials are highly coupled with service account credentials, self signed jwts can only be signed using service account credentials
-get_access_token() can be the external interface for generating tokens, it is consistent with the GoogleCredentials API, and there is nothing OAuth2 specific about the naming.
-The fields contained in the JWT are Google specific as far as I can tell, so this should be considered a Google specific class

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Apr 22, 2016

Member

@kpayson64 Thanks for contributing this! :) 👍

Member

theacodes commented Apr 22, 2016

@kpayson64 Thanks for contributing this! :) 👍

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64
Contributor

kpayson64 commented Apr 22, 2016

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Apr 22, 2016

Member

@anthmgoogle @dhermes @nathanielmanistaatgoogle Care to chime in on API and naming conventions here?

I'd personally prefer this API (which closely matches pyjwt's):

service_account = oauth2client.service_account.ServiceAccount.from_json_keyfile_name('service-account.json')
jwt = oauth2client.jwt.encode(service_account)

Additional claims:

jwt = oauth2client.jwt.encode(service_account, {'sub': 'user@example.com'})

@nathanielmanistaatgoogle would probably be happy to see less classes. :D

Member

theacodes commented Apr 22, 2016

@anthmgoogle @dhermes @nathanielmanistaatgoogle Care to chime in on API and naming conventions here?

I'd personally prefer this API (which closely matches pyjwt's):

service_account = oauth2client.service_account.ServiceAccount.from_json_keyfile_name('service-account.json')
jwt = oauth2client.jwt.encode(service_account)

Additional claims:

jwt = oauth2client.jwt.encode(service_account, {'sub': 'user@example.com'})

@nathanielmanistaatgoogle would probably be happy to see less classes. :D

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Apr 22, 2016

Member

We could also expose this as ServiceAccount.encode_jwt, since a lot of this logic is already in ServiceAccount._generate_assertion. ¯_(ツ)_/¯

Member

theacodes commented Apr 22, 2016

We could also expose this as ServiceAccount.encode_jwt, since a lot of this logic is already in ServiceAccount._generate_assertion. ¯_(ツ)_/¯

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Apr 22, 2016

Contributor

@jonparrott That misses the mark. These credentials need to be able to be used as a swap-in replacement for credentials.

Contributor

dhermes commented Apr 22, 2016

@jonparrott That misses the mark. These credentials need to be able to be used as a swap-in replacement for credentials.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Apr 22, 2016

Member

@dhermes interesting. Can you expand more on that? JWTs are not credentials, they are signed claims that can be used for authorization. The service account private key is the credential.

Member

theacodes commented Apr 22, 2016

@dhermes interesting. Can you expand more on that? JWTs are not credentials, they are signed claims that can be used for authorization. The service account private key is the credential.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Apr 22, 2016

Member

Is the desire to do something like this?

credentials = JwtCredentials(service_account_credentials)
credentials.authorize(http)
# http now sends the JWT in the Authorization header

If so, I don't see how this is distinct from just making the existing ServiceAccount accept a claims arg in its constructor.

Member

theacodes commented Apr 22, 2016

Is the desire to do something like this?

credentials = JwtCredentials(service_account_credentials)
credentials.authorize(http)
# http now sends the JWT in the Authorization header

If so, I don't see how this is distinct from just making the existing ServiceAccount accept a claims arg in its constructor.

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Apr 22, 2016

Contributor

For APIs like Cloud Bigtable, the JWT can be used as the access token (so-called scopless credentials). Hence, we want a class to allow this, and maybe have some built-in retry behavior which regenerates the local token (this PR doesn't quite get there either).

Contributor

dhermes commented Apr 22, 2016

For APIs like Cloud Bigtable, the JWT can be used as the access token (so-called scopless credentials). Hence, we want a class to allow this, and maybe have some built-in retry behavior which regenerates the local token (this PR doesn't quite get there either).

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes Apr 22, 2016

Contributor

@jonparrott Sorry we posted at essentially the same moment.

The difference is that ServiceAccountCredentials uses the JWT to exchange for a bearer token (which requires a round trip).

Contributor

dhermes commented Apr 22, 2016

@jonparrott Sorry we posted at essentially the same moment.

The difference is that ServiceAccountCredentials uses the JWT to exchange for a bearer token (which requires a round trip).

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Apr 22, 2016

Member

Ah, I see, I should get more sleep before discussing auth.

So, I think I'd prefer "both". We should separate the logic of generating a JWT vs using the jwt.

I suggest we:

  • Add oauth2client.jwt.encode that accepts a signer and set of claims and returns a JWT.
  • Make this new class use it to generate its access token.
  • Make ServiceAccount use it to generate its assertion.

This covers all use-cases well and prevents duplicate code.

Member

theacodes commented Apr 22, 2016

Ah, I see, I should get more sleep before discussing auth.

So, I think I'd prefer "both". We should separate the logic of generating a JWT vs using the jwt.

I suggest we:

  • Add oauth2client.jwt.encode that accepts a signer and set of claims and returns a JWT.
  • Make this new class use it to generate its access token.
  • Make ServiceAccount use it to generate its assertion.

This covers all use-cases well and prevents duplicate code.

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 Apr 22, 2016

Contributor

@jonparrott
I think the oauth2client.jwt.encode you are suggesting is essentially crypt.make_signed_jwt.

@dhermes
Do we want the new class to implement client.Credentials? Could you be more specific with the functionality that is missing?

Contributor

kpayson64 commented Apr 22, 2016

@jonparrott
I think the oauth2client.jwt.encode you are suggesting is essentially crypt.make_signed_jwt.

@dhermes
Do we want the new class to implement client.Credentials? Could you be more specific with the functionality that is missing?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Apr 22, 2016

Member

Okay, after chatting with @jboeuf the API should shake out like this:

class JWTAccessCredentials(object):
    def __init__(self, service_account, additional_claims=None)
         pass

    def get_access_token(self, additional_claims=None)
          pass

    def create_with_claims(claims):
          pass

This allows for it to satisfy the oauth2client.client.Credentials interface as well as work for gRPC by allowing gRPC to explicitly call get_access_token(additional_claims={'aud': service_url}) without having to create tons of instances.

Member

theacodes commented Apr 22, 2016

Okay, after chatting with @jboeuf the API should shake out like this:

class JWTAccessCredentials(object):
    def __init__(self, service_account, additional_claims=None)
         pass

    def get_access_token(self, additional_claims=None)
          pass

    def create_with_claims(claims):
          pass

This allows for it to satisfy the oauth2client.client.Credentials interface as well as work for gRPC by allowing gRPC to explicitly call get_access_token(additional_claims={'aud': service_url}) without having to create tons of instances.

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 Apr 26, 2016

Contributor

This latest commit has the new API. Also, I have made JWTAccessCredentials inherit GoogleCredentials . I'm holding off on unit tests/documentation until I have an API nailed down.

Contributor

kpayson64 commented Apr 26, 2016

This latest commit has the new API. Also, I have made JWTAccessCredentials inherit GoogleCredentials . I'm holding off on unit tests/documentation until I have an API nailed down.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes Apr 26, 2016

Member

API surface looks good, I completed a round of reviews. Please ping when you're ready for another review.

Side note: your file seems to have mixed indentation, please use 4 spaces.

Member

theacodes commented Apr 26, 2016

API surface looks good, I completed a round of reviews. Please ping when you're ready for another review.

Side note: your file seems to have mixed indentation, please use 4 spaces.

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 Apr 26, 2016

Contributor

@jonparrott
I've updated the pr from the feedback.

Contributor

kpayson64 commented Apr 26, 2016

@jonparrott
I've updated the pr from the feedback.

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 May 5, 2016

Contributor

@jonparrott
Is the following use case of this library deprecated?

http = httplib2.Http()
creds.authorize(http)

If so, is there any expected use case of JWT tokens outside of gRPC I should consider?

Contributor

kpayson64 commented May 5, 2016

@jonparrott
Is the following use case of this library deprecated?

http = httplib2.Http()
creds.authorize(http)

If so, is there any expected use case of JWT tokens outside of gRPC I should consider?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes May 5, 2016

Member

@kpayson64 no, but in general we want to move away from httplib2 (or rather, support urllib3) as soon as possible. If you're writing a net-new project and want thread-safety and proper https, urllib3 is a much more sane way to go. see #128 and gcloud-python#1364.

Member

theacodes commented May 5, 2016

@kpayson64 no, but in general we want to move away from httplib2 (or rather, support urllib3) as soon as possible. If you're writing a net-new project and want thread-safety and proper https, urllib3 is a much more sane way to go. see #128 and gcloud-python#1364.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle May 5, 2016

Contributor

I think I really like the API change in the current draft - we're adding one new method to one existing class and that's it for changes to the public surface area of the entire library?

Is this the shape this change should have going forward or are there still design pressures pushing this in the direction of "no, a new public class is the right thing for this feature"?

Contributor

nathanielmanistaatgoogle commented May 5, 2016

I think I really like the API change in the current draft - we're adding one new method to one existing class and that's it for changes to the public surface area of the entire library?

Is this the shape this change should have going forward or are there still design pressures pushing this in the direction of "no, a new public class is the right thing for this feature"?

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 May 5, 2016

Contributor

@jonparrott
Sounds good, I can try to produce a POC using urllib3.

A side note, AFAICT credentials are not thread safe, and I assumed this was because the underlying httplib2 is not thread safe. Should the new code be thread safe?

Contributor

kpayson64 commented May 5, 2016

@jonparrott
Sounds good, I can try to produce a POC using urllib3.

A side note, AFAICT credentials are not thread safe, and I assumed this was because the underlying httplib2 is not thread safe. Should the new code be thread safe?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes May 5, 2016

Member

Credentials are thread-safe as long as their storage is thread-safe.

On Thu, May 5, 2016 at 11:24 AM kpayson64 notifications@github.com wrote:

@jonparrott https://github.com/jonparrott
Sounds good, I can try to produce a POC using urllib3.

A side note, AFAICT credentials are not thread safe, and I assumed this
was because the underlying httplib2 is not thread safe. Should the new code
be thread safe?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#503 (comment)

Member

theacodes commented May 5, 2016

Credentials are thread-safe as long as their storage is thread-safe.

On Thu, May 5, 2016 at 11:24 AM kpayson64 notifications@github.com wrote:

@jonparrott https://github.com/jonparrott
Sounds good, I can try to produce a POC using urllib3.

A side note, AFAICT credentials are not thread safe, and I assumed this
was because the underlying httplib2 is not thread safe. Should the new code
be thread safe?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#503 (comment)

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 May 6, 2016

Contributor

It seems like we have converged on an API. I think the one unresolved issue is if JWTAccessCredentials should be public. It should be a trivial change to make JWTAccessCredentials public in the future, so I think we should leave it private for now.

If we are comfortable with this API, could @jonparrott @nathanielmanistaatgoogle do a final review?

Contributor

kpayson64 commented May 6, 2016

It seems like we have converged on an API. I think the one unresolved issue is if JWTAccessCredentials should be public. It should be a trivial change to make JWTAccessCredentials public in the future, so I think we should leave it private for now.

If we are comfortable with this API, could @jonparrott @nathanielmanistaatgoogle do a final review?

"""
new_kwargs = dict(self._kwargs)
new_kwargs.update(claims)
result = self.__class__(self._service_account_email,

This comment has been minimized.

@nathanielmanistaatgoogle

nathanielmanistaatgoogle May 11, 2016

Contributor

self.__class__ is a little surprising here - I guess I thought this would have thought this would always return instances of one specific class. Am I just missing the larger picture?

@nathanielmanistaatgoogle

nathanielmanistaatgoogle May 11, 2016

Contributor

self.__class__ is a little surprising here - I guess I thought this would have thought this would always return instances of one specific class. Am I just missing the larger picture?

This comment has been minimized.

@kpayson64

kpayson64 May 11, 2016

Contributor

For ServiceAccountCredentials(Oauth2), create_with_claims() should provide additional claims when attempting to get an access token. For JWT service account credentials, create_with_claims() should directly add additional claims to the AccessJWT.

In essence, a ServiceAccountCredentials and JWTAccessCredentials are both allowed to specify additional claims, but the behavior is different.

@kpayson64

kpayson64 May 11, 2016

Contributor

For ServiceAccountCredentials(Oauth2), create_with_claims() should provide additional claims when attempting to get an access token. For JWT service account credentials, create_with_claims() should directly add additional claims to the AccessJWT.

In essence, a ServiceAccountCredentials and JWTAccessCredentials are both allowed to specify additional claims, but the behavior is different.

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 May 11, 2016

Contributor

I have addressed the feedback.

Contributor

kpayson64 commented May 11, 2016

I have addressed the feedback.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle May 12, 2016

Contributor

Change content looks good!

Please squash commits.

Please compose a commit message with at least a paragraph explaining what's going on. Is this a Google-proprietary feature? Is this a Google-specific feature? Is there a publicly available open and unencumbered standard being implemented in this change that you can cite and to which you can link?

Contributor

nathanielmanistaatgoogle commented May 12, 2016

Change content looks good!

Please squash commits.

Please compose a commit message with at least a paragraph explaining what's going on. Is this a Google-proprietary feature? Is this a Google-specific feature? Is there a publicly available open and unencumbered standard being implemented in this change that you can cite and to which you can link?

Added JWTAccessCredentials.
Newer Google APIs can accept JWTs signed using ServiceAccountCredentials
for authentication. (See https://jwt.io/).  The new behavior for
GoogleCredentials.get_application_default() will attempt to use a signed JWT
if ServiceAccountCredentials are available and no scope is specified.
Upon specifying a scope, OAuth2 authentication will be used.
@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 May 16, 2016

Contributor

I've squashed the commits and updated the commit message.

Contributor

kpayson64 commented May 16, 2016

I've squashed the commits and updated the commit message.

@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle

nathanielmanistaatgoogle May 18, 2016

Contributor

I'm satisfied; @dhermes, @jonparrott, and @jboeuf: please either also make a final review or explicitly decline to do so?

Contributor

nathanielmanistaatgoogle commented May 18, 2016

I'm satisfied; @dhermes, @jonparrott, and @jboeuf: please either also make a final review or explicitly decline to do so?

@dhermes

This comment has been minimized.

Show comment
Hide comment
@dhermes

dhermes May 18, 2016

Contributor

I decline 😀 I trust you

Contributor

dhermes commented May 18, 2016

I decline 😀 I trust you

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes May 18, 2016

Member

I'm satisfied.

On Wed, May 18, 2016, 8:26 AM Danny Hermes notifications@github.com wrote:

I decline [image: 😀] I trust you


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#503 (comment)

Member

theacodes commented May 18, 2016

I'm satisfied.

On Wed, May 18, 2016, 8:26 AM Danny Hermes notifications@github.com wrote:

I decline [image: 😀] I trust you


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#503 (comment)

@jboeuf

This comment has been minimized.

Show comment
Hide comment
@jboeuf

jboeuf May 18, 2016

LGTM. Thanks! Please CC me on the PR that will do the integration with grpc.

jboeuf commented May 18, 2016

LGTM. Thanks! Please CC me on the PR that will do the integration with grpc.

@nathanielmanistaatgoogle nathanielmanistaatgoogle merged commit f0ba603 into googleapis:master May 18, 2016

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 100.0%
Details
@nathanielmanistaatgoogle

This comment has been minimized.

Show comment
Hide comment
@nathanielmanistaatgoogle
Contributor

nathanielmanistaatgoogle commented May 18, 2016

Woohoo!

@kpayson64

This comment has been minimized.

Show comment
Hide comment
@kpayson64

kpayson64 May 18, 2016

Contributor

@jonparrott
What is the process for releasing a new version?

Contributor

kpayson64 commented May 18, 2016

@jonparrott
What is the process for releasing a new version?

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes May 18, 2016

Member

I probably won't have time to cut a release this week. @nathanielmanistaatgoogle may be able to.

Member

theacodes commented May 18, 2016

I probably won't have time to cut a release this week. @nathanielmanistaatgoogle may be able to.

@theacodes

This comment has been minimized.

Show comment
Hide comment
@theacodes

theacodes May 19, 2016

Member

I'd like to see #506 merged before we release.

Member

theacodes commented May 19, 2016

I'd like to see #506 merged before we release.

@ensonic

This comment has been minimized.

Show comment
Hide comment
@ensonic

ensonic Sep 7, 2017

Can you make _JWTAccessCredentials public? Or how else would/show I do something like below. Please also not the hack to set 'aud'.

from oauth2client.service_account import _JWTAccessCredentials as JWTAccessCredentials
import httplib2

httplib2.debuglevel = 1

credentials = JWTAccessCredentials.from_json_keyfile_name(
    '/path/to/service-account-credentials.json', scopes=['https://www.googleapis.com/auth/datastore'])
# the class method does not pass through he kwargs to __init__()
# this needs to match the service name in the api-config:
credentials._kwargs['aud'] = 'myapi.endpoints.myproject.cloud.goog'

http_auth = credentials.authorize(httplib2.Http(disable_ssl_certificate_validation=True))
resp, content = http_auth.request('https://example.com/v1/myapi', 'GET')

Or should people manually do this as e.g. in:
https://github.com/GoogleCloudPlatform/python-docs-samples/blob/master/endpoints/getting-started/clients/google-jwt-client.py

ensonic commented Sep 7, 2017

Can you make _JWTAccessCredentials public? Or how else would/show I do something like below. Please also not the hack to set 'aud'.

from oauth2client.service_account import _JWTAccessCredentials as JWTAccessCredentials
import httplib2

httplib2.debuglevel = 1

credentials = JWTAccessCredentials.from_json_keyfile_name(
    '/path/to/service-account-credentials.json', scopes=['https://www.googleapis.com/auth/datastore'])
# the class method does not pass through he kwargs to __init__()
# this needs to match the service name in the api-config:
credentials._kwargs['aud'] = 'myapi.endpoints.myproject.cloud.goog'

http_auth = credentials.authorize(httplib2.Http(disable_ssl_certificate_validation=True))
resp, content = http_auth.request('https://example.com/v1/myapi', 'GET')

Or should people manually do this as e.g. in:
https://github.com/GoogleCloudPlatform/python-docs-samples/blob/master/endpoints/getting-started/clients/google-jwt-client.py

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