feat: enable token auth for Jupyter notebooks [MD-404] #9452

azhou-determined · 2024-05-30T14:08:22Z

Ticket

Description

Enables access to a remote Jupyter server running as a Determined task outside Determined (use case here is to support VSCode integrations).

Jupyter in VSCode requires an access token, so this change modifies Determined auth logic to accept a token parameter in the proxy URLs.

Test Plan

Testing this requires VSCode with the Jupyter extension installed.

Launch a Jupyter notebook through the CLI:
det notebook start
After the notebook starts up, it should print a URL for connecting to the running remote Jupyter server:
Copy this URL, go to VSCode
Create a new Jupyter notebook in VSCode
In VSCode, click on "Select Kernel" -> "Select another kernel" -> "Existing Jupyter server"
You should see a prompt to enter the URL of a remote Jupyter server
Paste the URL from det notebook start into the prompt, it should contain a ?token= parameter.
Run some commands in the local notebook shell. Verify that VSCode is able to connect to the Determined Jupyter instance.

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-05-30T14:08:39Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`3adaed5`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/667378c6cd8f440008bb3015

codecov · 2024-05-30T14:10:10Z

Codecov Report

Attention: Patch coverage is 2.58621% with 113 lines in your changes missing coverage. Please review.

Project coverage is 49.85%. Comparing base (ea929fc) to head (3adaed5).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9452      +/-   ##
==========================================
- Coverage   49.90%   49.85%   -0.06%     
==========================================
  Files        1245     1246       +1     
  Lines      161903   162007     +104     
  Branches     2887     2886       -1     
==========================================
- Hits        80795    80763      -32     
- Misses      80937    81073     +136     
  Partials      171      171

Flag	Coverage Δ
backend	`43.88% <2.63%> (-0.15%)`	⬇️
harness	`63.81% <0.00%> (-0.01%)`	⬇️
web	`46.24% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
master/pkg/tasks/task_command.go	`0.00% <ø> (ø)`
harness/determined/cli/notebook.py	`28.26% <0.00%> (-0.63%)`	⬇️
master/internal/api_notebook.go	`32.38% <0.00%> (+0.15%)`	⬆️
master/internal/command/command.go	`66.94% <42.85%> (-0.88%)`	⬇️
master/internal/user/service.go	`48.23% <0.00%> (-3.50%)`	⬇️
master/internal/api_auth.go	`41.60% <0.00%> (-7.00%)`	⬇️
master/internal/db/postgres_notebook_sessions.go	`0.00% <0.00%> (ø)`
master/internal/command/command_service.go	`63.20% <0.00%> (-11.24%)`	⬇️

... and 7 files with indirect coverage changes

NicholasBlaskey · 2024-05-30T14:53:29Z

master/internal/command/command.go

 	allo := c.refreshAllocationState()
+	notebookAddress := fmt.Sprintf("%s?token=%s", c.serviceAddress(), c.Base.UserSessionToken)


I'm a little worried about returning user tokens in /api/v1/notebooks for security reasons

is there a way around this?

we have to give the plain text link with the token to the user, so they could copy-paste it into their vscode, right? so probably no way to avoid sending these secrets over the wire.

we could maybe send the base URL and the token separately, so at least the automated URL scanners could not catch it. but that is security by obscurity.

I think my issue is we are exposing Determined users tokens to other users. An non admin could snag an admins token from the api and just start making calls as that user. Or you could impersonate another user.

I have mentioned at least part of this in a DM but an alternative that doesn't expose more access into the system is

Notebooks links in the API stay the same. When connecting from the CLI we add ?token=... to the link we print. This token comes from being logged into the CLI or webui.

The proxy authenticates the request based on ?token if present.

If vscode needs the notebook to be authenticated the proxy can add another layer of auth between itself and the notebook. This is optional to me since we don't assume this connection is authed today. But I don't think the notebook could use the CLI/webui token? as the token it expects because multiple tokens should be valid to access the notebook. Maybe you could rewrite token to a per Notebook credential.

oh, good point.

NicholasBlaskey

lgtm

NicholasBlaskey · 2024-06-18T19:09:31Z

master/internal/db/postgres_notebook_sessions.go

+
+	if _, err := Bun().NewInsert().Model(notebookSession).
+		Returning("id").Exec(ctx, &notebookSession.ID); err != nil {
+		return err


nit: wrap these errors

NicholasBlaskey · 2024-06-18T19:12:43Z

master/internal/command/command_service.go

@@ -131,6 +131,59 @@ func (cs *CommandService) LaunchGenericCommand(
 	return cmd, nil
 }

+// LaunchNotebookCommand creates notebook commands and persists them to the database.
+func (cs *CommandService) LaunchNotebookCommand(


I'm a little worried about this logic diverging from the generic one LaunchGenericCommand

can we instead have LaunchGenericCommand check task type and only create a notebook session if task type is notebook?

yeah, i didn't want this either, but it's hard to isolate, since the notebook launch takes in a session parameter and also sprinkles in additional calls throughout the method.

NicholasBlaskey · 2024-06-18T19:13:50Z

master/internal/api_auth.go

+// in two places:
+// 1. A token query parameter in the request URL.
+// 2. An HTTP Authorization header with a "token" type.
+func extractNotebookTokenFromRequest(r *http.Request) (string, error) {


maybe this doesn't need to return an error?

azhou-determined requested a review from ioga May 30, 2024 14:08

azhou-determined requested review from a team as code owners May 30, 2024 14:08

azhou-determined requested a review from NicholasBlaskey May 30, 2024 14:08

cla-bot bot added the cla-signed label May 30, 2024

NicholasBlaskey reviewed May 30, 2024

View reviewed changes

azhou-determined force-pushed the vscode-notebooks branch 2 times, most recently from bbc95df to db62b72 Compare June 18, 2024 16:43

NicholasBlaskey approved these changes Jun 18, 2024

View reviewed changes

feat: enable token auth for Jupyter notebooks

059a34d

azhou-determined force-pushed the vscode-notebooks branch from 7d1cedc to 32d04ff Compare June 19, 2024 23:52

alternate approach, store notebook tokens in a separate table

3adaed5

azhou-determined force-pushed the vscode-notebooks branch from 32d04ff to 3adaed5 Compare June 20, 2024 00:33

azhou-determined merged commit 553521e into main Jun 20, 2024
83 of 98 checks passed

azhou-determined deleted the vscode-notebooks branch June 20, 2024 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable token auth for Jupyter notebooks [MD-404] #9452

feat: enable token auth for Jupyter notebooks [MD-404] #9452

azhou-determined commented May 30, 2024 •

edited

Loading

netlify bot commented May 30, 2024 •

edited

Loading

codecov bot commented May 30, 2024 •

edited

Loading

NicholasBlaskey May 30, 2024

ioga May 30, 2024

NicholasBlaskey May 31, 2024 •

edited

Loading

ioga May 31, 2024

NicholasBlaskey left a comment

NicholasBlaskey Jun 18, 2024

NicholasBlaskey Jun 18, 2024

azhou-determined Jun 20, 2024

NicholasBlaskey Jun 18, 2024

		allo := c.refreshAllocationState()
		notebookAddress := fmt.Sprintf("%s?token=%s", c.serviceAddress(), c.Base.UserSessionToken)

feat: enable token auth for Jupyter notebooks [MD-404] #9452

feat: enable token auth for Jupyter notebooks [MD-404] #9452

Conversation

azhou-determined commented May 30, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented May 30, 2024 • edited Loading

✅ Deploy Preview for determined-ui canceled.

codecov bot commented May 30, 2024 • edited Loading

Codecov Report

NicholasBlaskey May 30, 2024

Choose a reason for hiding this comment

ioga May 30, 2024

Choose a reason for hiding this comment

NicholasBlaskey May 31, 2024 • edited Loading

Choose a reason for hiding this comment

ioga May 31, 2024

Choose a reason for hiding this comment

NicholasBlaskey left a comment

Choose a reason for hiding this comment

NicholasBlaskey Jun 18, 2024

Choose a reason for hiding this comment

NicholasBlaskey Jun 18, 2024

Choose a reason for hiding this comment

azhou-determined Jun 20, 2024

Choose a reason for hiding this comment

NicholasBlaskey Jun 18, 2024

Choose a reason for hiding this comment

azhou-determined commented May 30, 2024 •

edited

Loading

netlify bot commented May 30, 2024 •

edited

Loading

codecov bot commented May 30, 2024 •

edited

Loading

NicholasBlaskey May 31, 2024 •

edited

Loading