[auth] Allow use of cloud access tokens for hail batch authorization #13131

daniel-goldstein · 2023-05-31T23:25:54Z

Deprecate hail-minted API keys in favor of using access tokens from the identity providers already associated with user identities. For more context and a high-level overview of the implementation, see this RFC

hail/python/hailtop/hailctl/auth/login.py

daniel-goldstein · 2023-06-07T21:32:06Z

@danking This is far from done, evidenced by the fact that for some reason I'm getting tons of QoB test failures (but some are passing! :/) but I would appreciate a first pass on this if you want to do a high-level review. I do have a rough RFC that is not up to date, so let me know if you'd prefer to start the discussion from such a doc instead of the code.

danking

This all seems broadly what I expected. I didn't carefully review the auth flow yet.

The idea is that we're either using:

latent credentials like google application default or the azure managed identity.
An OAuth2 flow to get authorization to use the user's email/openid.

In either case, we get an OAuth2 access token for email or openid, we send that to Hail which uses it to verify that the caller produced a valid email/openid access token. We accept that as proof of identity. The user sends that along with every request as proof it is still logged in. If the token expires, the client requests a new one and sends that along in the next request.

We are gonna use either a "hail" audience or create our own scopes to ensure that Google doesn't let arbitrary third parties steal these tokens and log into, say, Terra.

Does that all check out?

hail/python/hailtop/auth/auth.py

hail/python/hailtop/hailctl/auth/login.py

danking · 2023-06-09T21:57:01Z

hail/scripts/upload_qob_jar.sh

@@ -22,5 +22,6 @@ else
    JAR_LOCATION="${TEST_STORAGE_URI}/${NAMESPACE}/jars/${TOKEN}/${REVISION}.jar"
 fi

-python3 -m hailtop.aiotools.copy -vvv 'null' '[{"from":"'${SHADOW_JAR}'", "to":"'${JAR_LOCATION}'"}]'
+gcloud storage cp ${SHADOW_JAR} ${JAR_LOCATION}
+# python3 -m hailtop.aiotools.copy -vvv 'null' '[{"from":"'${SHADOW_JAR}'", "to":"'${JAR_LOCATION}'"}]'


:/. Timeout issues, right?

rm -rf and please PR something to back off the default timeout so we can feel confident in copy.

danking · 2023-06-09T21:58:07Z

hail/src/main/scala/is/hail/services/Requester.scala

+      .createScoped(
+        "openid",
+        "https://www.googleapis.com/auth/userinfo.email",
+        "https://www.googleapis.com/auth/userinfo.profile"


Do we need profile or openid?

Good question, I can try to remove them.

danking · 2023-06-09T21:58:32Z

hail/src/main/scala/is/hail/services/Requester.scala

+  override def accessToken(): String = {
+    val context = new TokenRequestContext()
+    // TODO I hope I dont have to give this scope
+    context.addScopes("https://management.azure.com/.default")


openid should be enough no?

Or email? https://learn.microsoft.com/en-us/azure/active-directory/develop/scopes-oidc

Ya that's why I have the TODO, going to make sure I can trim that down.

This is actually replaced by the hail-specific OAuth scope.

danking · 2023-06-09T21:59:39Z

infra/azure/modules/auth/main.tf

+    oauth2_permission_scope {
+      admin_consent_description  = "Allow hailctl to access the Hail Batch service on behalf of the signed-in user."
+      admin_consent_display_name = "hailctl"
+      user_consent_description   = "Allow hailctl to access the Hail Batch service on your behalf."


It's also the hail Python library right?

Hm ya I can change the text here to the Hail Python Library.

danking · 2023-06-09T22:02:05Z

build.yaml

+      if [ "$?" -ne 0 ]
+      then
+          kubectl delete secret -n {{ default_ns.name }} ssl-config-hail-root
+      fi


This is unrelated updates of the root cert, yeah?

Ya this was merged in a different PR already. Dropped this commit.

danking · 2023-06-09T22:02:44Z

hail/python/hailtop/aiocloud/aiogoogle/credentials.py

+        'https://www.googleapis.com/auth/userinfo.email',
+        'https://www.googleapis.com/auth/cloud-platform',
+        'https://www.googleapis.com/auth/appengine.admin',
+        'https://www.googleapis.com/auth/compute',


We can drop the last three, right?

I didn't want to change the defaults in this PR, as these are used for other purposes like the FS which definitely needs at least GCS scopes. I think as a follow-up we should get rid of default scopes in general and require use sites to specify the min set of scopes they ned.

danking

dismiss if ready for another look

hail/python/hailtop/auth/flow.py

danking

Excellent changes. Some conceptual (but, I guess, not RFC-level, more code-level concepts) things we need to iron out and get written down. A few nits. Overall feels good!

danking · 2023-08-07T20:06:32Z

auth/auth/auth.py

@@ -518,6 +518,12 @@ async def rest_login(request: web.Request) -> web.Response:
    )


+@routes.get('/api/v1alpha/oauth2-client')
+async def hailctl_oauth_client(request):  # pylint: disable=unused-argument
+    idp = 'Google' if CLOUD == 'gcp' else 'Microsoft'


Aside: I don't love that gcp is a magic string for Google, but it is what is for now.

Shouldn't you use IdentityProvider.GOOGLE.value and IdentityProvider.MICROSOFT.value for the idp?

Yes you're right, I'll use that.

Bump, the literal strings are still here.

auth/auth/auth.py

danking · 2023-08-07T20:23:50Z

auth/auth/auth.py

+    raise web.HTTPUnauthorized()
+
+
+async def get_userinfo_from_login_id_or_hail_identity_id(request, login_id):


This appears to only be used with a IdP's uid, right? I think I'm a bit confused on hail identity uid vs login id. The login id is the username from the email, right? And hail identity uid is the uid from the cloud provider?

login_id is what we use to identify human users. In GCP it is currently an email, in Azure it is the actual sub of that user in AAD.

hail_identity is the email in GCP or common name in Azure of the robot identity assigned to the user. hail_identity_uid is the actual sub of the robot identity in both of those places.

This appears to only be used with a IdP's uid, right

This is not the case because we do not store UIDs for human users in GCP, only their email.

hail/src/main/scala/is/hail/services/Requester.scala

infra/azure/modules/auth/main.tf

x

danking · 2023-08-21T23:48:38Z

review is in progress, sorry for the delay.

danking · 2023-08-21T23:43:47Z

auth/auth/auth.py

@@ -518,6 +518,12 @@ async def rest_login(request: web.Request) -> web.Response:
    )


+@routes.get('/api/v1alpha/oauth2-client')
+async def hailctl_oauth_client(request):  # pylint: disable=unused-argument
+    idp = 'Google' if CLOUD == 'gcp' else 'Microsoft'


Bump, the literal strings are still here.

auth/auth/auth.py

hail/python/hailtop/aiocloud/aiogoogle/credentials.py

danking

Not finished with review, but here's what I have

hail/python/hailtop/auth/flow.py

hail/python/hailtop/aiocloud/aioazure/credentials.py

batch/batch/cloud/azure/worker/worker_api.py

batch/batch/worker/worker_api.py

hail/python/hailtop/aiocloud/aioazure/credentials.py

hail/python/hailtop/auth/flow.py

hail/python/hailtop/batch_client/aioclient.py

danking · 2023-08-24T19:21:41Z

infra/azure/modules/auth/main.tf

@@ -58,12 +58,53 @@ resource "azuread_application_password" "oauth2" {
  application_object_id = azuread_application.oauth2.object_id
 }

+resource "random_uuid" "hailctl_oauth2_idenfier_uri_id" {}


We have to apply this before we merge, right?

Ya. This is one of the annoying papercuts I was talking about. Unless we stack them, this PR and the ubuntu PR cannot be passing at the same time because they have different terraform changes.

danking · 2023-08-24T19:23:29Z

I think we're very close; lots of little details of course, but we're close.

made issues and edits

danking

A big but important and necessary change.

Great work.

danking · 2023-08-31T23:13:45Z

batch/batch/cloud/azure/driver/create_instance.py

@@ -205,6 +206,7 @@ def create_vm_config(
 DOCKER_ROOT_IMAGE=$(jq -r '.docker_root_image' userdata)
 DOCKER_PREFIX=$(jq -r '.docker_prefix' userdata)
 REGION=$(jq -r '.region' userdata)
+HAIL_AZURE_OAUTH_SCOPE=$(jq -r '.hail_azure_oauth_scope' userdata)


Just a reminder of this before my approval.

hail/python/hailtop/aiocloud/aiogoogle/credentials.py

danking · 2023-08-31T23:16:47Z

I put WIP on in light of the infra ordering issue.

github-advanced-security bot found potential problems May 31, 2023

View reviewed changes

hail/python/hailtop/hailctl/auth/login.py Fixed Show fixed Hide fixed

daniel-goldstein force-pushed the idp branch 2 times, most recently from 6b4be59 to 08b2eec Compare June 1, 2023 01:39

github-advanced-security bot found potential problems Jun 1, 2023

View reviewed changes

hail/python/hailtop/hailctl/auth/login.py Fixed Show resolved Hide resolved

daniel-goldstein force-pushed the idp branch 7 times, most recently from 8ca6896 to e231540 Compare June 5, 2023 17:58

daniel-goldstein assigned danking Jun 7, 2023

daniel-goldstein force-pushed the idp branch 2 times, most recently from 40282f5 to b0dd11e Compare June 9, 2023 20:06

danking reviewed Jun 9, 2023

View reviewed changes

daniel-goldstein force-pushed the idp branch 6 times, most recently from 78bec4b to f832b4d Compare June 16, 2023 15:30

daniel-goldstein force-pushed the idp branch from f832b4d to 49fb891 Compare June 20, 2023 15:02

danking previously requested changes Jun 20, 2023

View reviewed changes

daniel-goldstein force-pushed the idp branch 3 times, most recently from 86e00b3 to 0e1a34a Compare June 21, 2023 20:09

daniel-goldstein added the stacked PR label Jun 21, 2023

github-advanced-security bot found potential problems Jun 21, 2023

View reviewed changes

hail/python/hailtop/auth/flow.py Fixed Show fixed Hide fixed

daniel-goldstein force-pushed the idp branch from 0e1a34a to 6cbe7bd Compare June 22, 2023 12:00

danking previously requested changes Aug 7, 2023

View reviewed changes

daniel-goldstein added 6 commits August 11, 2023 12:04

Merge branch 'main' into idp

39dc367

address some comments

d34f2ae

more cleanup

be7b7a3

Merge branch 'main' into idp

a2a0156

make AnonymousCloudCredentials not subclass CloudCredentials

6b70c06

fixes

a43b75c

danking reviewed Aug 21, 2023

View reviewed changes

danking suggested changes Aug 21, 2023

View reviewed changes

hail/python/hailtop/auth/flow.py Show resolved Hide resolved

danking previously requested changes Aug 24, 2023

View reviewed changes

daniel-goldstein added 3 commits August 31, 2023 11:31

Merge branch 'main' into idp

28927ca

address comments

0ebed3e

ignore msals horrible typing problems

0dc7841

daniel-goldstein mentioned this pull request Aug 31, 2023

Deprecate and remove hail API tokens in favor of OAuth access tokens #13531

Open

lint

06cc2de

danking approved these changes Aug 31, 2023

View reviewed changes

danking added the WIP label Aug 31, 2023

daniel-goldstein added 3 commits September 5, 2023 11:11

Merge branch 'main' into idp

8713d5d

lints

4b02808

fix

0248788

daniel-goldstein removed the WIP label Sep 5, 2023

danking merged commit dc1f086 into hail-is:main Sep 5, 2023
5 checks passed

daniel-goldstein added terraform full-deploy Requires a full deployment at this commit before following commits can be deployed labels Sep 5, 2023

danking added the NIST 800-53 label Oct 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[auth] Allow use of cloud access tokens for hail batch authorization #13131

[auth] Allow use of cloud access tokens for hail batch authorization #13131

daniel-goldstein commented May 31, 2023 •

edited

Loading

daniel-goldstein commented Jun 7, 2023

danking left a comment

danking Jun 9, 2023

daniel-goldstein Jun 12, 2023

danking Aug 7, 2023

danking Jun 9, 2023

daniel-goldstein Jun 12, 2023 •

edited

Loading

danking Jun 9, 2023

danking Jun 9, 2023

daniel-goldstein Jun 12, 2023

daniel-goldstein Jun 20, 2023

danking Jun 9, 2023

daniel-goldstein Jun 12, 2023

danking Jun 9, 2023

daniel-goldstein Jun 12, 2023

danking Jun 9, 2023

daniel-goldstein Jun 20, 2023

danking left a comment

danking left a comment

danking Aug 7, 2023

daniel-goldstein Aug 7, 2023

danking Aug 21, 2023

danking Aug 7, 2023

daniel-goldstein Aug 11, 2023

daniel-goldstein Aug 11, 2023

danking commented Aug 21, 2023

danking Aug 21, 2023

danking left a comment

danking Aug 24, 2023

daniel-goldstein Aug 31, 2023

danking commented Aug 24, 2023

danking left a comment

danking Aug 31, 2023

danking commented Aug 31, 2023

		raise web.HTTPUnauthorized()


		async def get_userinfo_from_login_id_or_hail_identity_id(request, login_id):

[auth] Allow use of cloud access tokens for hail batch authorization #13131

[auth] Allow use of cloud access tokens for hail batch authorization #13131

Conversation

daniel-goldstein commented May 31, 2023 • edited Loading

daniel-goldstein commented Jun 7, 2023

danking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-goldstein Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danking left a comment

Choose a reason for hiding this comment

danking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danking commented Aug 21, 2023

Choose a reason for hiding this comment

danking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danking commented Aug 24, 2023

danking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danking commented Aug 31, 2023

daniel-goldstein commented May 31, 2023 •

edited

Loading

daniel-goldstein Jun 12, 2023 •

edited

Loading