This is based on how Rocket does it — note that the Rocket dev, @SergioBenitez, is also the dev of the cookie crate.

Dropshot currently does not provide this functionality but it easily could. Or not. Doing it here seems fine too.

I figure we should check both cookie expiration time and database? or we could just ignore the one on the cookie since the one we have stored is really the source of truth

The expires set on a cookie is intended for the client. After the expires date lapses from the client's perspective the cookie will be removed. I'd say if we get a cookie we should try to verify it. The expiration we really care about should be managed server side. Typically you might have something like Redis or some other datastore with a TTL setting on a row that'd remove it after it expires. Cockroach doesn't have that though (see bulk deleting expired data).

I'm not sure if this matters, but according to MDN, the key-value pairs in the Cookie header are separated by ; , not just ;.

Along these lines, is there something out there that's already vetted that parses the key-value pairs out of the Cookie header? It looks like Cookie::parse already does this, but we're pre-splitting the header and using it here to parse just a chunk?

Sorry if a lot of this is understood. It's obviously outside my wheelhouse. I'm just nervous because this seems like pretty important code to get right and I'm trying to really convince myself it's correct.

I was following this comment: rwf2/cookie-rs#137 (comment)

I admit I did not look into the reasoning.

I agree that it's odd that the cookie lib doesn't have something that does this, and so do others. @SergioBenitez seems to think it's too simple to warrant a utility for it, or rather any appropriately general utility would be too complex to be worth it. See also https://github.com/SergioBenitez/Rocket/blob/7ffe3a73601aabdfe3190a809e660538acd5bdce/core/lib/src/request/request.rs#L995-L1007.

@davepacheco unlike Spoof, this authn method depends on the rest of Nexus because it depends on the sessions table, so in order to test it I had to add this call to a real Nexus endpoint. Ideally I could test by adding a special test-only endpoint that I can add the auth call to and leave the real endpoints alone, but I'm not sure how to do that. Ideas?

Answering my own question after some sleep, as it often is: the way these often work when they're pluggable middleware type things (kind of like what we have here) is to factor out the session lookup function into something that can be passed in. It would probably have to take a generic argument referring to the server context type, which in the real Nexus case would look like

async fn lookup_session(ctx: &Arc<ServerContext>, token: &str) -> Result<Session, Error> {
    ctx.nexus.session_fetch(token.to_string()).await
}

and in the test case it would ignore the argument and hard code in a map:

async fn lookup_session(_ctx: <something>, token: &str) -> Result<Session, Error> {
    match token {
        "good" => Ok(<good_token>), 
        "expired" => Ok(<expired_token>),
        _ => Err(<not found>),
    }
}

I will try to figure out how to do this.

Which part are you talking about testing? For "spoof", I separated out the guts (simple as it is) so that I could test it with a unit test without a whole HTTP server. I also have an integration test that looks at the external HTTP response for various authn results. That one should cover you here, too.

For what it's worth, while working on "spoof" I considered adding a separate "test" dropshot server with its own API that we could use for stuff like this. But I found I could do it entirely outside with a custom Dropshot server in the integration test rather than Nexus, and that seemed better.

Sean helped me with this (change here: 54282ad )— the scheme is now generic so it can take any kind of context that implements SessionBackend, which just means it has a session lookup function. I need to make the type of Session generic and write some tests and you'll see what I mean

I saw in #299 - and the discussion in the product chat room - that we might be punting on having a user table? FYI @teisenbe , not sure if the alternative will have a UUID

I think we have to store a record of users existing somehow because we need something to reference in the ACLs. I understood the conclusion of those discussions to be about how much additional info to store, besides the basic requirement of an internal UUID plus an external ID representing the user in the customer's auth system.

I understood the conclusion from the product chat to be only that we wouldn't support POST /users and DELETE /users/:user. I think it's an important open question exactly what data is stored and how it's kept in sync with the directory, but I expect it'll be more than nothing.

If you're aiming to cut down on the control flow paths, this is a decent candidate for ok_or_else, to convert the option to a Result (and to use the ? operator more):

let token = cookies.get(SESSION_COOKIE_NAME)
  .map(|c| c.value())
  .ok_or_else(|| SchemeResult::NotRequested)?;

Ultimately it's not too different from what you're already doing, but I've seen some minor preference for "reduce the number of explicit return calls" in Rust functions to make following control flow easier.

nit: store "last_used" instead of "time_expires"? That's just as useful for expiring it, gives the client more flexibility in interpreting it, and is also a potentially useful fact for us to record.

I haven't gotten to where we delete records for expired sessions, but I suspect we'll want an index on whichever field we use for this so that we can quickly enumerate the oldest sessions.

Feel free to punt this to later, but a relatively easy thing I saw in the cheat sheet is binding the session to certain properties of the client, like the source IP and user agent headers. We could add those here and check them on fetch.

That's a good idea. Your IP would change if you went from wi-fi to LTE or something, but that's rare enough that I wouldn't feel bad. Will probably make an issue or add to the tracking list rather than implementing here. (Edit: I see it's already in #341)

suggestion: Call this ConsoleSession? I suspect we're going to wind up with a lot of things called sessions (e.g., TLS session, cloud shell session, VNC session, serial console session, etc.)

Alternatively, maybe better: we could try to keep the console stuff in a separate CockroachDB schema or database (not cluster). That might be good for improved security and smaller blast radius.

Yes, I'll change the name. I'd want to wait to see how much more stuff we have. It might only be this table and maybe some user preferences.

I think this is used for cases where we're reporting something like "object not found: session "foo"". For the usual security reasons, I wonder if we don't want to leak that information, and so maybe don't need this. (If we do, thoughts on "console session"?)

Do you know why this is needed here but not for the the Spoof one?

Sean helped me fix this by moving the type annotation to map: c2a9329#diff-5a2c045a788ce52d2612b2ffa2e163fe66cbee0fb72776cef9df4e9695262213R49

The reason it wasn't a problem with Spoof alone is that the error was about the arms of the match not matching. When there was only Spoof, there was only one arm.

Which part are you talking about testing? For "spoof", I separated out the guts (simple as it is) so that I could test it with a unit test without a whole HTTP server. I also have an integration test that looks at the external HTTP response for various authn results. That one should cover you here, too.

For what it's worth, while working on "spoof" I considered adding a separate "test" dropshot server with its own API that we could use for stuff like this. But I found I could do it entirely outside with a custom Dropshot server in the integration test rather than Nexus, and that seemed better.

What will this return if the 'id' is in use? That seems almost certainly a server bug (500 error).

It returns one of these. I think I'd consider it the job of the nexus wrapper or even the endpoint calling that to turn it into a 500. I just added an entry to #341 to make sure we're not leaking info about the nature of the error when we respond.

Err(ObjectAlreadyExists { type_name: ConsoleSession, object_name: "a_token" })

I'm not sure that's right. ObjectAlreadyExists is already an api::external::Error, right? That's the type we turn directly into an HTTP response and the usual pattern is to just ? and bail early when you get one. It feels pretty error prone to have cases where consumers have to check which one they got and potentially translate it to a different variant.

Yep, you’re right. Now I understand what @smklein meant when he was helping me with this PR and said it wasn’t ideal that we’re using these external errors basically everywhere. The DB Result types should not be using the public-facing error type. I’ll look into it but it probably goes beyond this PR. For now I’ll fix this one so it doesn’t leak info.

I could make them all return 500s on errors for now, which will get ignored by the session cookie scheme either way because I'm collapsing Result to Option in the calling code for the moment. Or I could put in TODOs, add it to the list in #341, and address it after #347.

We may as well just do the better thing, as long we're going to do #347.

I'd suggest putting the expire time in here, and maybe even the time we checked against.

What about trying with one that's actually expired (i.e., use session_create_with with a time in the past)?

that's what I'm doing! these could be more nicely grouped visually, I did think about that

let ago_5_minutes = Utc::now() - Duration::seconds(300);
let _ =
    nexus.session_create_with("expired".into(), user2, ago_5_minutes).await;

Sorry! My mistake. I saw "expired" was the session token itself and thought it was a magic value.

these should probably be in the config — @davepacheco what's a nice way to plumb through the values here? I almost want to stick the entire config on server context so I can do something like

fn absolute_timeout(&self) -> Duration {
    Duration::minutes(self.config.session_absolute_ttl_minutes)
}

That sounds good. I think it's reasonable to expose simple tunables like this on the Context. The thing I worry about is exposing more complicated things that ought to be better encapsulated. It would be easy for someone to mistakenly think they can figure out the server's bound address by grabbing config.dropshot_external.bind_address for example. As long as this is behind a method, that doesn't seem like a problem. We could even put this in a tunables section of the config so that and only store that in the Context.

I'd just make the name more descriptive -- absolute_timeout doesn't say anything to me about being config-related or session-related.

Ah right. I suppose the fact that's it's a method on the SessionStore trait doesn't help when you're just doing context.absolute_timeout() or you see it in an autocomplete menu.

this was an interesting learning experience. because the logic for session expiration requires making multiple calls to the session store, I couldn't extract out a pure function of the request that's easily unit testable, like the Spoof scheme has. Instead I end up testing the full authn method with a fake context that implements SessionStore with a hashmap as the store.

This is not going to stay in, I just need to figure out out to use a custom server for integration tests like WhoamiServer is being used to test the spoof scheme. There are some annoying parts but they're not at all insurmountable.

The main annoying thing is that in order to implement SessionStore on Arc<WhoamiServerState> (source) I have to wrap it in a newtype because I don't own SessionStore or Arc.

Well, it turns out the server state didn't need to be wrapped in Arc anyway, so getting rid of that solves my orphan rule problem. The other problem is that the diff is gnarly as hell and the test is getting messy. So I think what I'll do is still basically this, but move it into its own file so it's not mixed up with all the spoof tests. Unfortunately there's a lot of similar boilerplate between that we'll want to think about cleaning up later.

cookie-auth...cookie-whoami done properly in bcd45d3

this can go away once I stop using the real nexus for the integration tests

had to make this stop being Arc so I could implement SessionStore directly on WhoamiServerState

I'm surprised that worked but that's no problem.

I would think that if you impl'd SessionStore on WhoamiServerState, you could still use Arc<WhoamiServerState> where you need a something that impl'd SessionStore, as long as you took the value by reference and didn't need it to be mutable.

I tried it that way and it complained that Arc<WhoamiServerState> didn't have SessionStore impl'd, but it's very possible I wasn't doing it right.

I followed Tess's lead here adding tests directly to datastore. I find these tests pretty reassuring. Too bad they're slow to spin up, at least on Mac (see #53).

Still seems like this should be time_last_used right?