Console API: sessions + static assets #384

david-crespo · 2021-11-12T00:54:32Z

Serve the console from Nexus. Closes #356

For routes that go to console pages, we check auth server-side — if the client is authed, we will send down an HTML response that loads the JS bundle in a script tag. If the client is not, we redirect to whatever IdP is configured. For testing/demo purposes this will probably be a provisional login page served by Nexus.

What's in here

Add console related endpoints to external API
- Placeholder POST /login create session in DB and sets session cookie
- Placeholder GET /login serves console bundle, but without auth gate, so we can show a login form
- POST /logout deletes session in DB and clears session cookie in browser
- GET /orgs/* serves console pages, e.g., if you go to console.oxide.rack/projects or whatever
  - When authed this responds with the HTML that then downloads the bundle
  - When unauthed this redirects to login
- GET /assets/* for serving assets from a configurable directory. no auth: static assets MUST NOT be sensitive
Add [console] section to config for assets dir and cache time
- Also move existing session idle and absolute timeouts into there
Cookie helpers
- Extractor impl so we can get the session token out of the request in order to delete the session during logout
- Helper to generate cookie value to send to the browser
Test helper things
- Make body field on TestResponse public (so I can assert about it as bytes)
- Rename response_body() to parsed_body()
- Add expect_response_header to TestResponse

Places I especially want feedback

Configuring the asset directory
- Currently relative to nexus dir + asserts at startup that it exists
- Should it be absolute?
- Other safety checks at startup to avoid grave errors?
- It's at top level in the config rather than specific to console API because currently external, internal, and console servers all have the same config type. Would be nice to have NormalConfig & ConsoleConfig, but intersection types aren't clean in Rust like they are in, e.g., TypeScript
How to handle the fact that we want a fake login endpoint for now that we extremely do not want to be there in production
Should we auth gate assets? I lean no, especially not for now — we can do it later
File placement of things like the Cookies extractor and the cookie header value helper
I'm using PathBuf in place where it seems like I could be using Path, but I couldn't get it to compile. Does it matter?

There will be more to add to this list.

Things left to do

Serve HTML from "console page" routes (for now we can serve the index.html that's in the console bundle)
Test with real console assets
Update tests to use want better interface for making test HTTP requests #403 helpers once that's merged

Open questions to punt on

Route prefixes for API or for console routes
- It would be nice to serve the console without a prefix
- We will probably punt on this
How to deploy the static assets along with Nexus
- I really want to avoid checking them in to this repo

See it in action

david-crespo · 2021-11-12T01:00:00Z

nexus/src/http_entrypoints_console.rs

+        // but we don't return it from the scheme. The result of that scheme
+        // only tells us the Actor, i.e., the user ID. We can't just delete all
+        // sessions for that user ID because a user could have sessions going in
+        // multiple browsers and only want to log out one.


@davepacheco Interesting question here. We could just stick the token in the authn context, but it would only be there when the session cookie scheme is the one being used. It would be very simple but a little weird to just make it an optional field on the actor.

or we could write a Cookies extractor and just pull it again. prototype here 490a1e5

It makes sense to me to pull it out again, at least for now. I like the idea of keeping the extractor in Omicron for now. What did you need from Dropshot to do that?

just this

diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 66c516e..c6e6f3d 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -532,6 +532,7 @@ pub use config::ConfigDropshot; pub use error::HttpError; pub use error::HttpErrorResponseBody; pub use handler::Extractor; +pub use handler::ExtractorMetadata; pub use handler::HttpResponse; pub use handler::HttpResponseAccepted; pub use handler::HttpResponseCreated;

davepacheco · 2021-11-15T19:32:41Z

How to deploy the static assets along with Nexus
I really want to avoid checking them in to this repo

Agreed. My first thought was to use something like ci_download_cockroachdb to download them as-needed at build time. One difference though is that with CockroachDB and Clickhouse, we expect those to already be running in a real environment, so these tools are really aimed at developers. This thing, however, needs to become part of the Nexus build that we ship. I think that means maybe omicron-package needs to either do this or else it needs to be done by the time somebody runs omicron-package.

Where do these files come from today? Is there going to be a circular dependency (assets shipped by console, so Nexus depends on console, but console also depends on Nexus)?

davepacheco

Cool.

nexus/tests/config.test.toml

 [dropshot_external]
 bind_address = "127.0.0.1:0"

-#


nexus/examples/config.toml

 # IP address and TCP port on which to listen for the internal API
 bind_address = "127.0.0.1:12221"

+[dropshot_console]


davepacheco · 2021-11-15T19:38:31Z

nexus/src/http_entrypoints_console.rs

+        // but we don't return it from the scheme. The result of that scheme
+        // only tells us the Actor, i.e., the user ID. We can't just delete all
+        // sessions for that user ID because a user could have sessions going in
+        // multiple browsers and only want to log out one.


It makes sense to me to pull it out again, at least for now. I like the idea of keeping the extractor in Omicron for now. What did you need from Dropshot to do that?

david-crespo · 2021-11-15T19:42:12Z

Today on GCP these files are pushed around by the console's Dockerfile. For the last milestone demo I sent @jclulow a tarball of the files and he manually dropped them in the right place to be served by nginx.

From #356:

For local console dev, we currently spin up Nexus and then proxy API requests to it through the Vite dev server with an /api prefix. When we deploy to GCP for QA and demo purposes, we use nginx to do roughly the same thing. In both cases, something sits in front of both Nexus and the static bundle and serves both.

david-crespo · 2021-11-18T22:46:40Z

nexus/src/console_api/http_entrypoints.rs

+    }
+
+    Ok(current)
+}


This is based on the file server example @ahl added to Dropshot for me.

david-crespo · 2021-11-18T22:47:24Z

nexus/src/context.rs

+            assets_directory.exists(),
+            "assets directory must exist at start time"
+        );
+


Don't feel confident about this, it's just meant to provisionally show the whole thing works.

I don't think we want to blow an assertion for this condition.

We could have this function return Result. It looks like the caller already does so it's not hard to propagate the error.

Maybe a little friendlier would be to have the caller construct an object from this path that can be used to resolve or serve file names within that tree. e.g., FileLoader or something. Then we store that in the context, instead of just the path that gets re-resolved every time we access it. In an ideal world, this might store an open fd and subsequent calls would use openat. I'm not sure that Rust's built-in fs supports this -- there's this, which sounds right, but the docs aren't clear.

It'd also be neat to sanity-check some well-known assets, but I'm not sure if that's too much knowledge to bake in here.

I keep wondering too if we should bake these assets into the Nexus binary, but I guess that's more trouble than it's worth.

Minimal version done in 33ec251, no fancy FileLoader but I'll add a to-do comment about it

Made what I think is an improvement — using CARGO_MANIFEST_DIR, which consistently points at the nexus dir. Otherwise you get different results depending on the current working directory you're running the command from.

nexus/tests/test_console_api.rs

+
+    // TODO: responses with set-cookie in them won't work until this uses the
+    // new test helpers that don't lock you into a particular set of allowed
+    // headers. See https://github.com/oxidecomputer/omicron/pull/403


just-be-dev · 2021-11-18T23:16:43Z

nexus/src/console_api/http_entrypoints.rs

+// TODO: /c/ prefix is def not what we want long-term but it makes things easy for now
+#[endpoint {
+     method = GET,
+     path = "/c/{path:.*}",
+ }]


I'm fine with this for now, but for the future state: do we have route ranking mechanism? I don't (yet) have a solid understanding of how we know which api gets a request that comes into Nexus.

Right now, no. You can’t define a wildcard route that overlaps another route.

just-be-dev · 2021-11-18T23:24:37Z

nexus/src/console_api/http_entrypoints.rs

+    // we *could* auth gate the static assets but it would be kind of weird.
+    // prob not necessary. one way would be to have two directories, one that
+    // requires auth and one that doesn't. I'd rather not


Yeah, I'm with you. I definitely don't think we should do this. There should be nothing sensitive in our static assets. Also, we'd have a really hard time rendering a login or error page for the user if we couldn't send them the bundle to be able to render said page 😅

davepacheco

I'm not sure if you were looking for another round.

davepacheco · 2021-11-23T00:14:35Z

nexus/src/context.rs

+            assets_directory.exists(),
+            "assets directory must exist at start time"
+        );
+


I don't think we want to blow an assertion for this condition.

We could have this function return Result. It looks like the caller already does so it's not hard to propagate the error.

Maybe a little friendlier would be to have the caller construct an object from this path that can be used to resolve or serve file names within that tree. e.g., FileLoader or something. Then we store that in the context, instead of just the path that gets re-resolved every time we access it. In an ideal world, this might store an open fd and subsequent calls would use openat. I'm not sure that Rust's built-in fs supports this -- there's this, which sounds right, but the docs aren't clear.

It'd also be neat to sanity-check some well-known assets, but I'm not sure if that's too much knowledge to bake in here.

I keep wondering too if we should bake these assets into the Nexus binary, but I guess that's more trouble than it's worth.

davepacheco · 2021-11-23T00:21:53Z

nexus/tests/common/http_testing.rs

+            });
+        match (header_name, header_value) {
+            (Ok(name), Ok(value)) => {
+                self.expected_response_headers.append(name, value);


Presumably we could commonize these?

davepacheco · 2021-11-23T00:22:23Z

nexus/tests/common/http_testing.rs

+            let actual_value = headers.get(header_name).unwrap();
+            ensure!(
+                actual_value == expected_value,
+                "response contained expected header {:?}, but with value {:?} instead of expected {:?}",


Love the detailed message!

david-crespo · 2021-11-24T16:46:26Z

nexus/src/lib.rs

+            let msg = format!("errors shutting down: ({})", errors.join(", "));
+            Err(msg)
+        } else {
+            Ok(())


this is left over from when I added the third dropshot server. it's no longer necessary, but it's an improvement and makes adding a third one later trivial, so I'm leaving it in

david-crespo commented Nov 12, 2021

View reviewed changes

draft console server

785adc5

david-crespo force-pushed the console-api branch from 4f42add to 785adc5 Compare November 12, 2021 01:12

david-crespo added 2 commits November 11, 2021 23:03

stub out asset and console_page routes

e83e39c

Cookies extractor

490a1e5

davepacheco reviewed Nov 15, 2021

View reviewed changes

david-crespo added 3 commits November 17, 2021 14:47

logout is closer to correct, add fake login, session cookie value helper

9476cfd

working tests for session create and session auth, that's cool

e020e46

fix config files

3bc6880

david-crespo changed the title ~~Stub console-related API~~ Console API Nov 18, 2021

david-crespo added 10 commits November 17, 2021 23:28

logout makes session token stop working

ae1a19d

file server and login redirect pretty much work

5fae933

fix confusing variable names

fdc0d31

move session cookie header generator into session cookie auth scheme

b765a4d

clean up file serving

18802ef

Merge main into console-api

b864e64

stop using local dropshot

af72045

make console_api match the new dir structure

98d5f89

real internal error message for file not found errors

efe80eb

make assets directory configurable

356cf7c

david-crespo force-pushed the console-api branch from 6111086 to 356cf7c Compare November 18, 2021 22:38

david-crespo commented Nov 18, 2021

View reviewed changes

david-crespo marked this pull request as ready for review November 18, 2021 22:50

david-crespo commented Nov 18, 2021

View reviewed changes

david-crespo changed the title ~~Console API~~ Console API: sessions + static assets Nov 18, 2021

just-be-dev reviewed Nov 18, 2021

View reviewed changes

david-crespo added 2 commits November 22, 2021 17:13

whoops

7a533e2

premature optimization

c3bff0c

davepacheco reviewed Nov 23, 2021

View reviewed changes

david-crespo added 2 commits November 23, 2021 00:21

extract common header parsing code in test helpers

89fdc9a

handle nonexistent assets dir at startup with error instead of panic

33ec251

david-crespo force-pushed the console-api branch from 8dd134a to 88df393 Compare November 23, 2021 22:27

login form, shenanigans necessary to serve console

9332861

david-crespo force-pushed the console-api branch from 88df393 to 9332861 Compare November 23, 2021 22:51

david-crespo added 2 commits November 23, 2021 17:01

Merge main into console-api

3c0a6a3

response_body -> parsed_body merge issue

ddf642d

david-crespo force-pushed the console-api branch from 4aafb58 to d0a871d Compare November 23, 2021 23:40

include login and logout post endpoint in openapi spec

92a19be

david-crespo force-pushed the console-api branch 2 times, most recently from df69e5f to ebb6303 Compare November 24, 2021 00:45

update tests

eeaefc4

david-crespo force-pushed the console-api branch from ebb6303 to eeaefc4 Compare November 24, 2021 02:11

david-crespo mentioned this pull request Nov 24, 2021

Shenanigans required for serving console from nexus oxidecomputer/console#532

Merged

david-crespo and others added 3 commits November 23, 2021 21:12

we don't need the /c/ prefix! all the console routes start with /orgs !

6500f8c

cache-control headers, comment why we're not doing asset checks on start

c50476a

Merge branch 'main' into console-api

5e022ca

david-crespo commented Nov 24, 2021

View reviewed changes

david-crespo added 3 commits November 24, 2021 12:00

update comments

1a5eefe

make cache-control header tunable, rearrange config a bit

24604a1

assets dir relative to CARGO_MANIFEST_DIR for more stable results

159c4d0

david-crespo force-pushed the console-api branch from f3809e8 to 159c4d0 Compare November 24, 2021 19:43

david-crespo mentioned this pull request Nov 24, 2021

Distinguish API routes and console routes #430

Closed

Merge branch 'main' into console-api

a1ab95d

david-crespo merged commit 07d35c9 into main Nov 24, 2021

david-crespo deleted the console-api branch November 24, 2021 21:03

david-crespo added the console Web console label Jan 10, 2022

Console API: sessions + static assets #384

Console API: sessions + static assets #384

Uh oh!

Conversation

david-crespo commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in here

Places I especially want feedback

Things left to do

Open questions to punt on

See it in action

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco commented Nov 15, 2021

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo commented Nov 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

just-be-dev Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

david-crespo commented Nov 12, 2021 •

edited

Loading

david-crespo Nov 23, 2021 •

edited

Loading

david-crespo Nov 24, 2021 •

edited

Loading

just-be-dev Nov 18, 2021 •

edited

Loading

just-be-dev Nov 18, 2021 •

edited

Loading