multi: Autopilot session linking #621

ellemouton · 2023-08-08T11:53:17Z

With this PR, we add the ability for a client to link a new autopilot session with
a previous set of revoked ones. If this is done, then the same privacy mapper is used
for the linked sessions & the actions and rules db is also shared across sessions.

PR Flow:

First some edits are made to the Session store: mainly, we create separate methods for creating a new session vs updating the remote pub key of an existing session.
2.. Add a new session ID to session local pub key index to the session store. This makes possible to fetch session by ID. A migration to backfill this index is also added.
Update autopilotserver.proto to have the new fields required to link sessions.
These include the session group key along with a sig to prove ownership of the linked
session.
Update the autopilot server client to send the new fields if they are provided.
initially we do not provide them.
Add GroupID to the Session store. The group ID is the ID of the very first
session in the group of linked sessions.
Update litrpcs Session and AddAutopilotSessionRequest messages to include
a linked group ID.
Marshal the new fields in the Session proto message
Add new indexes to the session store which map from session ID to group ID and group ID
to session IDs.
a migration to backfill the above indexes. This is easy since currently the mapping is
one-to-one both ways. Only after this PR will the group-to-session index be one-to-many.
New methods to the session store to query via the new index.
add a new ListGroupActions method to the firewall DB that allows us to query the DB
for all actions of a specific group.
Let the firewall query group actions instead of session actions.
Allow ListActions rpc call to specify groupID.
Let privacy map conversion rpc call specify groupID.
let firewall get the correct privacy mapper by group ID on interception of calls.
Finally, allow the client to link sessions via the cli.

TODO:

add pagination to the new ListGroupActions method (might not need to do this in this PR...).

bitromortac

First round done, it's a very impressive PR 🥇, nice commit structure! Will also do manual tests in the next round.

session/tlv_test.go

session/tlv.go

firewalldb/session_id_index.go

firewalldb/migtest/migtest.go

firewalldb/migration1/session_id_index_test.go

firewalldb/migtest/raw_db.go

firewalldb/actions.go

cmd/litcli/actions.go

session_rpcserver.go

ViktorTigerstrom

First round of code review looks good 🔥 🚀!! Really nice PR.

Will also do more deep manual testing for next round :)!

session_rpcserver.go

ViktorTigerstrom · 2023-08-23T11:35:45Z

session_rpcserver.go

+			return nil, err
+		}
+
+		if prevSess.State != session.StateRevoked &&


Shouldn't we check here that there's no active session for whole group, and not just that the session for the prevPub isn't active? Else this will allow people to set the PrevLocalPub to an old session in the group that has already been replaced by a new session, and this will instead fail in on the AutoPilot server side. I think we should have checks on both the client + server side?

really good point.

The only thing stopping me from adding this is that currently, to do this, we would need to iterate through and load all of our sessions into memory to figure out which ones belong to the group. We currently dont even store the session ID in the session DB. So it could potentially be a very slow query if we have 100s of sessions.

So

option 1 is: we leave it like this and rely on autopilot to return an error (not ideal since we already persisted some things before calling autopilot)

option 2: we build an index from session pubkey to session ID.

option 3: we bite the bullet and migrate Lit to use a sqlite db

I gueeeesss the best thing to do for now is option 2

ok i've added option 2 in the commit before this one 🤓

Awesome, thanks! Agree that this is the best option for now :)!
I guess the only case this can happen now is if a user tries to link a session to a group where all sessions are expired, but then doesn't link the latest session in the group, but a previous one. That will succeed here, but fail on the AutoPilot server side. Though I think that's a fair tradeoff to not add checks for that in LiT and instead just error on the AutoPilot server side.

cool - this should no longer be an issue with the latest update and I think we no longer need to check the whole group. Let me know if you agree.

Really like the last update 🔥!

Though as we send the request to register the session to the Autopilot server before we store the session, we need to also need to add a check before this, that checks the linkedSessionIndex to see that no other session has linked a session using that id, so that that check fails before we send a request to the autopilot server.

I think we should keep this linkedSessionIndex check on LiT even if we change how the Autopilot server checks if a session can be linked corresponding to my other comment :)! I think the linkedSessionIndex way of checking it is really neat for LiT and I don't think that should any cause any issues similar to the ones I've described in my other comment 🚀!

firewalldb/session_id_index.go

firewalldb/session_id_index_test.go

session_rpcserver.go

bitromortac

Nice, this looks very close 🚀. Tested out the migration manually, also tried to go back to an older code version, which was prevented as expected.

firewalldb/migtest/raw_db.go

firewalldb/session_id_index.go

firewalldb/migtest/migtest.go

firewalldb/migration1/session_id_index.go

firewalldb/actions.go

bitromortac · 2023-08-23T11:30:48Z

firewall/rule_enforcer.go

@@ -30,6 +30,7 @@ var _ mid.RequestInterceptor = (*RuleEnforcer)(nil)
 type RuleEnforcer struct {
 	ruleDB            firewalldb.RulesDB
 	actionsDB         firewalldb.ActionReadDBGetter
+	sessionIDIndexDB  firewalldb.SessionIDIndex


suggestion: I feel like the previous commit may be easier to understand if squashed with this one

session_rpcserver.go

ellemouton

thanks for the review so far @ViktorTigerstrom & @bitromortac !!

I've updated accordingly.

I've added one new commit called session: add session ID to Key index

session_rpcserver.go

firewalldb/session_id_index.go

firewalldb/session_id_index_test.go

session_rpcserver.go

firewalldb/migtest/raw_db.go

session_rpcserver.go

ellemouton · 2023-08-23T13:50:35Z

session_rpcserver.go

+			return nil, err
+		}
+
+		if prevSess.State != session.StateRevoked &&


really good point.

The only thing stopping me from adding this is that currently, to do this, we would need to iterate through and load all of our sessions into memory to figure out which ones belong to the group. We currently dont even store the session ID in the session DB. So it could potentially be a very slow query if we have 100s of sessions.

So

option 1 is: we leave it like this and rely on autopilot to return an error (not ideal since we already persisted some things before calling autopilot)

option 2: we build an index from session pubkey to session ID.

option 3: we bite the bullet and migrate Lit to use a sqlite db

I gueeeesss the best thing to do for now is option 2

ellemouton · 2023-08-23T14:45:18Z

session_rpcserver.go

+			return nil, err
+		}
+
+		if prevSess.State != session.StateRevoked &&


ok i've added option 2 in the commit before this one 🤓

ViktorTigerstrom

New changes looks great! Still manually not done with second round of reviewing, but reviewing the changes first :)

Leaving one comment regarding an issue introduced with the latest push.

ViktorTigerstrom · 2023-08-23T17:40:34Z

session_rpcserver.go

+			prevSess, err := s.db.GetSessionByID(id)
+			if err != nil {
+				return nil, err
+			}


Sorry now we just introduced a case of the issue I comment about before 😅:
#621 (comment)

If we can't find the session in the db, we should just move on to the next id of the prevSessionIDs :).

ah! yeah true 😂 good catch! will update tomorrow.

think this is now fine with latest updates :)

ellemouton · 2023-08-24T12:02:59Z

gonna add something else shortly 🤓

ellemouton · 2023-08-24T12:37:06Z

Ok ya'll! thanks for your patience! The PR has been updated quite a bit. Here are the changes:

a session ID to session local pub key index is added to the session store.
the session ID to group ID and group ID index that was originally in the firewall DB is now in the session store. This makes things a lot simpler and more idempotent.
an index from previous session ID to linking session ID is added to the session store. This makes it quick to check that no other session has linked to the one you are trying to link to.
finally, to link a session from the CLI, you now specify the prev session ID (instead of the prev local pub)

ellemouton · 2023-08-24T15:42:59Z

ok I tacked on a temporary commit here now labeled temp:... i'll squash it in in a nicer way tomorrow

ViktorTigerstrom

Really nice updates!! Thinks became so much cleaner once the indexes are stored in the same file as the sessions 🚀 🔥 🚀 !!

Unfortunately though, I realised an issue that exists that'll likely require some redesigning... Sorry to find this one out so late in the review cycle. I've explained the issue in my first comment of this review.

session_rpcserver.go

ViktorTigerstrom · 2023-08-24T23:45:23Z

session_rpcserver.go

+			return nil, err
+		}
+
+		if prevSess.State != session.StateRevoked &&


Really like the last update 🔥!

Though as we send the request to register the session to the Autopilot server before we store the session, we need to also need to add a check before this, that checks the linkedSessionIndex to see that no other session has linked a session using that id, so that that check fails before we send a request to the autopilot server.

I think we should keep this linkedSessionIndex check on LiT even if we change how the Autopilot server checks if a session can be linked corresponding to my other comment :)! I think the linkedSessionIndex way of checking it is really neat for LiT and I don't think that should any cause any issues similar to the ones I've described in my other comment 🚀!

session_rpcserver.go

session/store.go

This commit does a few things: 1. Instead of deriving IDs using the first 4 bytes of the session's serialised local pub key, we instead use bytes [1:5] in order to skip the first byte which is either 0x02 or 0x03. This results in a greater entropy set. 2. We also add a new index from ID to key and we write to this index each time a new session is added. 3. We add a `ReserveNewSessionID` method to the session store which will grind through private keys until it finds one that does not clash with the current ID set. 4. A migration is added to back-fill the ID-to-key index. If any old sessions are found that _do_ have a colliding ID, they are sorted by created time and all but the newest session is revoked. Only an entry for the newest session will be added to the ID-to-key index.

Update the autopilot rpc protos to include the new fields that will allow the client to link autopilot sessions.

Update the autopilot server client to send the new session linking fields if they are provided. Currently, they are not yet provided.

Add the new `GroupID1` field to the `Session` struct and ensure that the new fields are properly serialised and deserialised. The `GroupID` is the ID of the first Session in a set of linked sessions.

Update the Autopilot service AddAutopilotSessionRequest message to include a new `group_id` field. Users may set this field to the ID of a sessino group in order to link the new session to the older ones. The Session message is also updated with new `group_id` field which will be populated by the group a session.

Marshal the new Session fields into the proto Session struct.

This commit adds two new indexes to the session store. One from session ID to group ID and one from groupID to a set of session IDs. This commit only adds the new logic and starts writing the the new indexes from the `StoreSession` method. The next commit will add a migration to back-fill the new indexes.

This commit adds a migration to the session store to back-fill the new session ID to group ID and group ID to session IDs indexes.

This commit adds new getters: `GetGroupID` and `GetSessionIDs` to the session store which can be used to query the newly added indexes to get the associated group ID for a session ID or the associated set of session IDs for a group ID.

Change all comments and variable names mentioning "session ID" to "group ID" instead. As of this commit, session linking is not yet possible and so what ever session IDs are passed in are also group IDs.

This method can be used to list actions in a session Group. Note that actions are still stored under session IDs.

This method can be used to check that each session in a group passes for a given predicate.

This commit adds logic to the CreateSession method that checks that a all the past sessions in a linked set are no longer active.

guggero

aACK, things around the session ID collision look good, very nice work 💯

bitromortac

tACK 🚀🚀 strong work 💪!

ellemouton requested review from bitromortac and ViktorTigerstrom August 15, 2023 11:42

ellemouton force-pushed the sessionLinking branch from ca48d39 to f6fa1e3 Compare August 17, 2023 10:13

bitromortac reviewed Aug 22, 2023

View reviewed changes

ellemouton force-pushed the sessionLinking branch 2 times, most recently from 9f6b95b to b289546 Compare August 22, 2023 14:27

ellemouton requested review from bitromortac August 22, 2023 14:55

ViktorTigerstrom reviewed Aug 23, 2023

View reviewed changes

bitromortac reviewed Aug 23, 2023

View reviewed changes

ellemouton force-pushed the sessionLinking branch from b289546 to b355746 Compare August 23, 2023 14:45

ellemouton commented Aug 23, 2023

View reviewed changes

ellemouton requested a review from ViktorTigerstrom August 23, 2023 14:55

ViktorTigerstrom reviewed Aug 23, 2023

View reviewed changes

ellemouton force-pushed the sessionLinking branch from b355746 to 4972e85 Compare August 24, 2023 11:23

ellemouton force-pushed the sessionLinking branch from 4972e85 to 1b47fae Compare August 24, 2023 12:22

ellemouton force-pushed the sessionLinking branch 4 times, most recently from d9622b8 to 2b4006b Compare August 24, 2023 13:18

ellemouton requested a review from ViktorTigerstrom August 24, 2023 15:45

ViktorTigerstrom reviewed Aug 25, 2023

View reviewed changes

ellemouton added 2 commits August 25, 2023 14:20

session: update the session Store interface

92575c8

session: separate methods for creating vs updating a session

d3a2626

ellemouton force-pushed the sessionLinking branch 2 times, most recently from 200790f to 296c281 Compare August 28, 2023 07:05

ellemouton requested a review from ViktorTigerstrom August 28, 2023 07:05

ellemouton added 4 commits August 30, 2023 11:46

autopilotserverrpc: update with session linking fields

9098253

Update the autopilot rpc protos to include the new fields that will allow the client to link autopilot sessions.

autopilotserver: update to send linked session info

c9c396d

Update the autopilot server client to send the new session linking fields if they are provided. Currently, they are not yet provided.

session: add GroupID field to Session

4bd8a5c

Add the new `GroupID1` field to the `Session` struct and ensure that the new fields are properly serialised and deserialised. The `GroupID` is the ID of the first Session in a set of linked sessions.

ellemouton force-pushed the sessionLinking branch from cb54bb0 to 8284d97 Compare August 30, 2023 09:46

ellemouton requested a review from guggero August 30, 2023 09:49

ellemouton force-pushed the sessionLinking branch from 8284d97 to b87c625 Compare August 30, 2023 10:32

ellemouton added 17 commits August 30, 2023 12:34

session_rpcserver: marshal new Session fields

477a511

Marshal the new Session fields into the proto Session struct.

session: migration to populate group ID to session ID indexes

e90df06

This commit adds a migration to the session store to back-fill the new session ID to group ID and group ID to session IDs indexes.

session: add GetGroupID and GetSessionIDs methods

cc95a01

This commit adds new getters: `GetGroupID` and `GetSessionIDs` to the session store which can be used to query the newly added indexes to get the associated group ID for a session ID or the associated set of session IDs for a group ID.

firewalldb: update comments and var names

7c7e467

Change all comments and variable names mentioning "session ID" to "group ID" instead. As of this commit, session linking is not yet possible and so what ever session IDs are passed in are also group IDs.

multi: give firewallDB access to session ID index

73110b6

firewalldb: add ListGroupActions method

c12e643

This method can be used to list actions in a session Group. Note that actions are still stored under session IDs.

firewall: update ActionsReadDB to get group actions

9218746

firewall: map session ID to group ID

3c837a9

multi: allow querying actions by group ID

bd9a99c

multi: use group ID for PrivacyMapConversion query

60c10bd

firewall: map session ID to group ID in privacy mapper interceptor

a14d7ae

session: add a CheckSessionGroupPredicate method

71cac86

This method can be used to check that each session in a group passes for a given predicate.

multi: allow client to link autopilot sessions

5882a21

app+proto: run make protos

c27f4eb

session: ensure past linked sessions are not active

3b5007c

This commit adds logic to the CreateSession method that checks that a all the past sessions in a linked set are no longer active.

ellemouton force-pushed the sessionLinking branch from b87c625 to 3b5007c Compare August 30, 2023 10:34

guggero approved these changes Aug 30, 2023

View reviewed changes

bitromortac approved these changes Aug 30, 2023

View reviewed changes

ellemouton merged commit 7f0f385 into master Aug 30, 2023
12 checks passed

ellemouton deleted the sessionLinking branch August 30, 2023 16:21

bitromortac mentioned this pull request Aug 31, 2023

autopilot: feature configuration #629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: Autopilot session linking #621

multi: Autopilot session linking #621

ellemouton commented Aug 8, 2023 •

edited

bitromortac left a comment

ViktorTigerstrom left a comment

ViktorTigerstrom Aug 23, 2023

ellemouton Aug 23, 2023

ellemouton Aug 23, 2023

ViktorTigerstrom Aug 23, 2023

ellemouton Aug 24, 2023

ViktorTigerstrom Aug 24, 2023

bitromortac left a comment

bitromortac Aug 23, 2023

ellemouton left a comment

ellemouton Aug 23, 2023

ellemouton Aug 23, 2023

ViktorTigerstrom left a comment

ViktorTigerstrom Aug 23, 2023 •

edited

ellemouton Aug 23, 2023

ellemouton Aug 24, 2023

ellemouton commented Aug 24, 2023

ellemouton commented Aug 24, 2023

ellemouton commented Aug 24, 2023

ViktorTigerstrom left a comment

ViktorTigerstrom Aug 24, 2023

guggero left a comment

bitromortac left a comment

multi: Autopilot session linking #621

multi: Autopilot session linking #621

Conversation

ellemouton commented Aug 8, 2023 • edited

PR Flow:

bitromortac left a comment

Choose a reason for hiding this comment

ViktorTigerstrom 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

bitromortac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

ViktorTigerstrom Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton commented Aug 24, 2023

ellemouton commented Aug 24, 2023

ellemouton commented Aug 24, 2023

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

bitromortac left a comment

Choose a reason for hiding this comment

ellemouton commented Aug 8, 2023 •

edited

ViktorTigerstrom Aug 23, 2023 •

edited