Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change authentication method to use OIDC identity tokens #322

Merged
merged 4 commits into from
Mar 8, 2023
Merged

Change authentication method to use OIDC identity tokens #322

merged 4 commits into from
Mar 8, 2023

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Mar 2, 2023 •
edited
Loading

Context

As of present, authentication is performed by the Merlin and Turing SDKs using Google OAuth 2.0 access tokens (generated after authentication with Google's OAuth server) that are added to the headers of requests sent to the Turing API server. These tokens are then used by an internal proxy which will validate the access token and then retrieve the user profile information from the Google API.

However, in keeping with the move of the Google Identity Services client library from OAuth 2.0 access tokens to Google OpenID Connect (OIDC) identity tokens, we are similarly replacing the use of the former for the latter. These tokens generated would similarly be added to the headers of requests sent to the Turing API server to be validated internally.

A utils module caraml_auth in the package caraml-auth-google has been created in the CaraML SDK repository to provide this new method of authentication.

This PR serves to update the existing authentication method used by the Turing SDK.

Changes

In sdk/turing/session.py, the Turing SDK will import and use the function get_default_id_token_credentials provided by the caraml-auth-google package:

from caraml_auth.id_token_credentials import get_default_id_token_credentials

credentials = get_default_id_token_credentials(target_audience="turing-sdk.caraml")
credentials.refresh(Request())

@deadlycoconuts deadlycoconuts self-assigned this Mar 2, 2023
@deadlycoconuts deadlycoconuts force-pushed the make_sdk_use_openid_connect_id_tokens branch from 8ff79b4 to 7f354db Compare March 6, 2023 02:31
@deadlycoconuts deadlycoconuts changed the title Add utils file to allow SDK to generate id tokens Change authentication method to use OIDC identity tokens Mar 6, 2023
@deadlycoconuts deadlycoconuts requested a review from a team March 7, 2023 06:45
@deadlycoconuts deadlycoconuts marked this pull request as ready for review March 7, 2023 06:45
krithika369
krithika369 reviewed Mar 7, 2023
View reviewed changes
sdk/turing/session.py Outdated
from google.auth.transport.requests import Request
from google.auth.transport.urllib3 import urllib3, AuthorizedHttp

# Load default credentials
credentials, _ = google.auth.default(scopes=TuringSession.OAUTH_SCOPES)
credentials = get_default_id_token_credentials(target_audience="turing-sdk.caraml")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the target_audience expected to be used on the auth server? Or is it currently unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unused right now. But we can set up the RequestAuthentication resource to validate the aud field which this target_audience field would translate into in the jwt token generated for service accounts. CMIIW @mbruner

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.. I wonder if we should simply call it "sdk.caraml" in order to not distinguish each SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with any name to be honest 😅 sdk.caraml it shall be then!

@deadlycoconuts deadlycoconuts force-pushed the make_sdk_use_openid_connect_id_tokens branch from 4926c06 to c7a98b3 Compare March 7, 2023 06:50
deadlycoconuts
deadlycoconuts commented Mar 7, 2023
View reviewed changes
sdk/requirements.txt
@@ -1,7 +1,7 @@
cloudpickle==2.0.0
deprecation==2.1.0
fire
google-auth>=1.11.0
google-auth>=2.16.2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this since this version is used in caraml-auth-google.

krithika369
krithika369 approved these changes Mar 8, 2023
View reviewed changes
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, besides the one pending discussion.

@deadlycoconuts
Copy link
Contributor Author

Thank you for the comments! I've just updated the name of the target audience. Will be merging this soon!

@deadlycoconuts deadlycoconuts merged commit 5cd2278 into caraml-dev:main Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krithika369 krithika369 krithika369 approved these changes

Assignees

@deadlycoconuts deadlycoconuts

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@deadlycoconuts @krithika369