[sql-14] sessions: atomic session creation #980
Conversation
ellemouton
added
no-changelog
ellemouton
force-pushed
the
sql14Sessions6
branch
from
72d32b0 to
5a28d56
Compare
session_rpcserver.go
Outdated
| req.DevServer, uniquePermissions, caveats, nil, false, nil, | ||
| session.PrivacyFlags{}, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating new session: %v", err) | ||
| } | ||
|
|
||
| if err := s.cfg.db.CreateSession(sess); err != nil { | ||
| sess, err = s.cfg.db.CreateSession(sess.ID) |
There was a problem hiding this comment.
note: in this follow up, we add a way to do this in one go for normal (non-autopilot) sessions
session/interface.go
Outdated
| /---> StateExpired | ||
| StateReserved ---> StateCreated --- | ||
| \---> StateRevoked |
There was a problem hiding this comment.
There is nothing in the database layer preventing a session from being immediately moved from StateReserved to StateRevoked.
I think we should enforce this constraint, either at the database layer or directly in the RevokeSession RPC endpoint. Otherwise, although extremely unlikely with the current code, a user could potentially revoke the session after the NewSession call in AddAutopilotSession but before or during the s.cfg.autopilot.RegisterSession call within the same function. That'd likely create a lot of unintended errors.
There was a problem hiding this comment.
what sort of errors would that create?
but yes, can add a tight-map constraint (above DB layer as it isnt db related). But i think we can make it so that it's fine to always go from any state to "revoked"
There was a problem hiding this comment.
So I think mainly there's 2 errors that would occur that I can think of right now, but there might be more:
-
The issue described in the comment below ([sql-14] sessions: atomic session creation #980 (comment)) would occur, because calling
s.cfg.db.CreateSession(sess.ID)after registering the session with the autopilot server results in an error in theAddAutopilotSessioncall, since the session is not in theReservedstate. -
The actual
s.cfg.autopilot.SessionRevoked(ctx, pubKey)call inRevokeSessionwould not actually revoke the session on the autopilot server, as the session has not yet been registered on the server. It'll then actually get added to server, when the call to register the session to the autopilot server gets executed, after it has already been revoked on the client. I don't know this could lead to unintended consequences that I can't think of right now.
There was a problem hiding this comment.
have added the strict state shift stuff here: #985
this PR now builds on top of that one
session_rpcserver.go
Outdated
| err = s.cfg.db.UpdateSessionRemotePubKey(sess.LocalPublicKey, remoteKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error setting remote pubkey: %v", err) | ||
| } | ||
|
|
||
| // We only activate the session if the Autopilot server registration | ||
| // was successful. | ||
| sess, err = s.cfg.db.CreateSession(sess.ID) | ||
| if err != nil { |
There was a problem hiding this comment.
There's nothing to change here, but just a reminder, as we discussed in the original session-linking PR, that we should allow session revocation based on groupIDs at the autopilot server level. 🤓 Otherwise, this could lead to a scenario where the autopilot server persists the reserved session, but litd removes it on the next startup if an error occurs here. That'd become problematic for linked sessions.
There was a problem hiding this comment.
ah yes, that's a great point
ellemouton
force-pushed
the
sql14Sessions6
branch
from
5a28d56 to
374c93f
Compare
And only allow legal state shifts.
In this commit, we let StateReserved be the new initial state of a session for when NewSession is called. We then do predicate checks for linked sessions along with unique session alias (ID) and priv key derivations all under the same DB transaction in NewSession. ShiftState then moves a session to StateCreated. Only in StateCreated does a session become usable. With this change, we no longer need to ensure atomic session creation by acquiring the `sessRegMu` mutex in the session RPC server.
This was used to check that all linked sessions are no longer active before attempting to register an autopilot session. But this is no longer needed since this is done within NewSession.
ellemouton
force-pushed
the
sql14Sessions6
branch
from
374c93f to
3f94793
Compare
See #979 for more context.
In this PR, we do the refactor to make session creation happen in 1 atomic DB transaction as described in the issue above.