Skip to content

[sql-11] sessions: interface massage #969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 12, 2025
Merged

[sql-11] sessions: interface massage #969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f5f35b
session: do full session assertion during tests
ellemouton Feb 9, 2025
e923fff
session: add NewSession to Store interface
ellemouton Feb 9, 2025
8c4d17c
session: add a clock to the DB
ellemouton Feb 9, 2025
64f7961
session: use clock for times
ellemouton Feb 9, 2025
6239428
session: use clock for created at time
ellemouton Feb 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions session/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,12 @@ type Session struct {
GroupID ID
}

// NewSession creates a new session with the given user-defined parameters.
func NewSession(id ID, localPrivKey *btcec.PrivateKey, label string, typ Type,
expiry time.Time, serverAddr string, devServer bool, perms []bakery.Op,
caveats []macaroon.Caveat, featureConfig FeaturesConfig,
privacy bool, linkedGroupID *ID, flags PrivacyFlags) (*Session, error) {
// buildSession creates a new session with the given user-defined parameters.
func buildSession(id ID, localPrivKey *btcec.PrivateKey, label string, typ Type,
created, expiry time.Time, serverAddr string, devServer bool,
perms []bakery.Op, caveats []macaroon.Caveat,
featureConfig FeaturesConfig, privacy bool, linkedGroupID *ID,
flags PrivacyFlags) (*Session, error) {

_, pairingSecret, err := mailbox.NewPassphraseEntropy()
if err != nil {
Expand All @@ -98,8 +99,8 @@ func NewSession(id ID, localPrivKey *btcec.PrivateKey, label string, typ Type,
Label: label,
State: StateCreated,
Type: typ,
Expiry: expiry,
CreatedAt: time.Now(),
Expiry: expiry.UTC(),
CreatedAt: created.UTC(),
ServerAddr: serverAddr,
DevServer: devServer,
MacaroonRootKey: macRootKey,
Expand Down Expand Up @@ -139,6 +140,18 @@ type IDToGroupIndex interface {
// Store is the interface a persistent storage must implement for storing and
// retrieving Terminal Connect sessions.
type Store interface {
// NewSession creates a new session with the given user-defined
// parameters.
//
// NOTE: currently this purely a constructor of the Session type and
// does not make any database calls. This will be changed in a future
// commit.
NewSession(id ID, localPrivKey *btcec.PrivateKey, label string,
typ Type, expiry time.Time, serverAddr string, devServer bool,
perms []bakery.Op, caveats []macaroon.Caveat,
featureConfig FeaturesConfig, privacy bool, linkedGroupID *ID,
flags PrivacyFlags) (*Session, error)

// CreateSession adds a new session to the store. If a session with the
// same local public key already exists an error is returned. This
// can only be called with a Session with an ID that the Store has
Expand Down
33 changes: 30 additions & 3 deletions session/kvdb_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import (
"time"

"github.com/btcsuite/btcd/btcec/v2"
"github.com/lightningnetwork/lnd/clock"
"go.etcd.io/bbolt"
"gopkg.in/macaroon-bakery.v2/bakery"
"gopkg.in/macaroon.v2"
)

var (
Expand Down Expand Up @@ -77,13 +80,15 @@ const (
// BoltStore is a bolt-backed persistent store.
type BoltStore struct {
*bbolt.DB

clock clock.Clock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you shed a little bit of light on the benefits of injecting a shared clock with the DB? My assumption is it's primarily used to add stability for tests, but not sure if there's more to it than that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it is mainly for deterministic tests so that we can guarantee stable (and incrementing) timestamps

}

// A compile-time check to ensure that BoltStore implements the Store interface.
var _ Store = (*BoltStore)(nil)

// NewDB creates a new bolt database that can be found at the given directory.
func NewDB(dir, fileName string) (*BoltStore, error) {
func NewDB(dir, fileName string, clock clock.Clock) (*BoltStore, error) {
firstInit := false
path := filepath.Join(dir, fileName)

Expand All @@ -106,7 +111,10 @@ func NewDB(dir, fileName string) (*BoltStore, error) {
return nil, err
}

return &BoltStore{DB: db}, nil
return &BoltStore{
DB: db,
clock: clock,
}, nil
}

// fileExists reports whether the named file or directory exists.
Expand Down Expand Up @@ -173,6 +181,25 @@ func getSessionKey(session *Session) []byte {
return session.LocalPublicKey.SerializeCompressed()
}

// NewSession creates a new session with the given user-defined parameters.
//
// NOTE: currently this purely a constructor of the Session type and does not
// make any database calls. This will be changed in a future commit.
//
// NOTE: this is part of the Store interface.
func (db *BoltStore) NewSession(id ID, localPrivKey *btcec.PrivateKey,
label string, typ Type, expiry time.Time, serverAddr string,
devServer bool, perms []bakery.Op, caveats []macaroon.Caveat,
featureConfig FeaturesConfig, privacy bool, linkedGroupID *ID,
flags PrivacyFlags) (*Session, error) {

return buildSession(
id, localPrivKey, label, typ, db.clock.Now(), expiry,
serverAddr, devServer, perms, caveats, featureConfig, privacy,
linkedGroupID, flags,
)
}

// CreateSession adds a new session to the store. If a session with the same
// local public key already exists an error is returned.
//
Expand Down Expand Up @@ -398,7 +425,7 @@ func (db *BoltStore) RevokeSession(key *btcec.PublicKey) error {
}

session.State = StateRevoked
session.RevokedAt = time.Now()
session.RevokedAt = db.clock.Now().UTC()

var buf bytes.Buffer
if err := SerializeSession(&buf, session); err != nil {
Expand Down
81 changes: 59 additions & 22 deletions session/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,28 @@ import (
"time"

"github.com/btcsuite/btcd/btcec/v2"
"github.com/lightningnetwork/lnd/clock"
"github.com/stretchr/testify/require"
)

var testTime = time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC)

// TestBasicSessionStore tests the basic getters and setters of the session
// store.
func TestBasicSessionStore(t *testing.T) {
// Set up a new DB.
db, err := NewDB(t.TempDir(), "test.db")
clock := clock.NewTestClock(testTime)
db, err := NewDB(t.TempDir(), "test.db", clock)
require.NoError(t, err)
t.Cleanup(func() {
_ = db.Close()
})

// Create a few sessions.
s1 := newSession(t, db, "session 1", nil)
s2 := newSession(t, db, "session 2", nil)
s3 := newSession(t, db, "session 3", nil)
s4 := newSession(t, db, "session 4", nil)
s1 := newSession(t, db, clock, "session 1", nil)
s2 := newSession(t, db, clock, "session 2", nil)
s3 := newSession(t, db, clock, "session 3", nil)
s4 := newSession(t, db, clock, "session 4", nil)

// Persist session 1. This should now succeed.
require.NoError(t, db.CreateSession(s1))
Expand Down Expand Up @@ -51,11 +55,11 @@ func TestBasicSessionStore(t *testing.T) {
for _, s := range []*Session{s1, s2, s3} {
session, err := db.GetSession(s.LocalPublicKey)
require.NoError(t, err)
require.Equal(t, s.Label, session.Label)
assertEqualSessions(t, s, session)

session, err = db.GetSessionByID(s.ID)
require.NoError(t, err)
require.Equal(t, s.Label, session.Label)
assertEqualSessions(t, s, session)
}

// Fetch session 1 and assert that it currently has no remote pub key.
Expand Down Expand Up @@ -89,17 +93,18 @@ func TestBasicSessionStore(t *testing.T) {
// TestLinkingSessions tests that session linking works as expected.
func TestLinkingSessions(t *testing.T) {
// Set up a new DB.
db, err := NewDB(t.TempDir(), "test.db")
clock := clock.NewTestClock(testTime)
db, err := NewDB(t.TempDir(), "test.db", clock)
require.NoError(t, err)
t.Cleanup(func() {
_ = db.Close()
})

// Create a new session with no previous link.
s1 := newSession(t, db, "session 1", nil)
s1 := newSession(t, db, clock, "session 1", nil)

// Create another session and link it to the first.
s2 := newSession(t, db, "session 2", &s1.GroupID)
s2 := newSession(t, db, clock, "session 2", &s1.GroupID)

// Try to persist the second session and assert that it fails due to the
// linked session not existing in the DB yet.
Expand All @@ -125,7 +130,8 @@ func TestLinkingSessions(t *testing.T) {
// of the GetGroupID and GetSessionIDs methods.
func TestLinkedSessions(t *testing.T) {
// Set up a new DB.
db, err := NewDB(t.TempDir(), "test.db")
clock := clock.NewTestClock(testTime)
db, err := NewDB(t.TempDir(), "test.db", clock)
require.NoError(t, err)
t.Cleanup(func() {
_ = db.Close()
Expand All @@ -135,9 +141,9 @@ func TestLinkedSessions(t *testing.T) {
// after are all linked to the prior one. All these sessions belong to
// the same group. The group ID is equivalent to the session ID of the
// first session.
s1 := newSession(t, db, "session 1", nil)
s2 := newSession(t, db, "session 2", &s1.GroupID)
s3 := newSession(t, db, "session 3", &s2.GroupID)
s1 := newSession(t, db, clock, "session 1", nil)
s2 := newSession(t, db, clock, "session 2", &s1.GroupID)
s3 := newSession(t, db, clock, "session 3", &s2.GroupID)

// Persist the sessions.
require.NoError(t, db.CreateSession(s1))
Expand All @@ -163,8 +169,8 @@ func TestLinkedSessions(t *testing.T) {

// To ensure that different groups don't interfere with each other,
// let's add another set of linked sessions not linked to the first.
s4 := newSession(t, db, "session 4", nil)
s5 := newSession(t, db, "session 5", &s4.GroupID)
s4 := newSession(t, db, clock, "session 4", nil)
s5 := newSession(t, db, clock, "session 5", &s4.GroupID)

require.NotEqual(t, s4.GroupID, s1.GroupID)

Expand Down Expand Up @@ -192,7 +198,8 @@ func TestLinkedSessions(t *testing.T) {
// method correctly checks if each session in a group passes a predicate.
func TestCheckSessionGroupPredicate(t *testing.T) {
// Set up a new DB.
db, err := NewDB(t.TempDir(), "test.db")
clock := clock.NewTestClock(testTime)
db, err := NewDB(t.TempDir(), "test.db", clock)
require.NoError(t, err)
t.Cleanup(func() {
_ = db.Close()
Expand All @@ -202,7 +209,7 @@ func TestCheckSessionGroupPredicate(t *testing.T) {
// function is checked correctly.

// Add a new session to the DB.
s1 := newSession(t, db, "label 1", nil)
s1 := newSession(t, db, clock, "label 1", nil)
require.NoError(t, db.CreateSession(s1))

// Check that the group passes against an appropriate predicate.
Expand All @@ -227,7 +234,7 @@ func TestCheckSessionGroupPredicate(t *testing.T) {
require.NoError(t, db.RevokeSession(s1.LocalPublicKey))

// Add a new session to the same group as the first one.
s2 := newSession(t, db, "label 2", &s1.GroupID)
s2 := newSession(t, db, clock, "label 2", &s1.GroupID)
require.NoError(t, db.CreateSession(s2))

// Check that the group passes against an appropriate predicate.
Expand All @@ -249,7 +256,7 @@ func TestCheckSessionGroupPredicate(t *testing.T) {
require.False(t, ok)

// Add a new session that is not linked to the first one.
s3 := newSession(t, db, "completely different", nil)
s3 := newSession(t, db, clock, "completely different", nil)
require.NoError(t, db.CreateSession(s3))

// Ensure that the first group is unaffected.
Expand Down Expand Up @@ -279,14 +286,15 @@ func TestCheckSessionGroupPredicate(t *testing.T) {
require.True(t, ok)
}

func newSession(t *testing.T, db Store, label string,
func newSession(t *testing.T, db Store, clock clock.Clock, label string,
linkedGroupID *ID) *Session {

id, priv, err := db.GetUnusedIDAndKeyPair()
require.NoError(t, err)

session, err := NewSession(
session, err := buildSession(
id, priv, label, TypeMacaroonAdmin,
clock.Now(),
time.Date(99999, 1, 1, 0, 0, 0, 0, time.UTC),
"foo.bar.baz:1234", true, nil, nil, nil, true, linkedGroupID,
[]PrivacyFlag{ClearPubkeys},
Expand All @@ -295,3 +303,32 @@ func newSession(t *testing.T, db Store, label string,

return session
}

func assertEqualSessions(t *testing.T, expected, actual *Session) {
expectedExpiry := expected.Expiry
actualExpiry := actual.Expiry
expectedRevoked := expected.RevokedAt
actualRevoked := actual.RevokedAt
expectedCreated := expected.CreatedAt
actualCreated := actual.CreatedAt

expected.Expiry = time.Time{}
expected.RevokedAt = time.Time{}
expected.CreatedAt = time.Time{}
actual.Expiry = time.Time{}
actual.RevokedAt = time.Time{}
actual.CreatedAt = time.Time{}

require.Equal(t, expected, actual)
require.Equal(t, expectedExpiry.Unix(), actualExpiry.Unix())
require.Equal(t, expectedRevoked.Unix(), actualRevoked.Unix())
require.Equal(t, expectedCreated.Unix(), actualCreated.Unix())

// Restore the old values to not influence the tests.
expected.Expiry = expectedExpiry
expected.RevokedAt = expectedRevoked
expected.CreatedAt = expectedCreated
actual.Expiry = actualExpiry
actual.RevokedAt = actualRevoked
actual.CreatedAt = actualCreated
}
6 changes: 3 additions & 3 deletions session/tlv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,8 @@ func DeserializeSession(r io.Reader) (*Session, error) {
session.Label = string(label)
session.State = State(state)
session.Type = Type(typ)
session.Expiry = time.Unix(int64(expiry), 0)
session.CreatedAt = time.Unix(int64(createdAt), 0)
session.Expiry = time.Unix(int64(expiry), 0).UTC()
session.CreatedAt = time.Unix(int64(createdAt), 0).UTC()
Comment on lines +240 to +241
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm can this somehow lead to some discrepancy between current sessions that were created without UTC, which are now deserialised with UTC?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah def - but i think it's ok. worst case the expiry/created_at times now differ by a couple of hours.

I think it's an ok tradeoff and i dont think it is worth the added complexity to make sure we read everything exactly as it was/add a migration etc.

thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did some testing just to make sure no issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess the worst case scenario would be that a user after upgrading has a very recently created session that's now deserialized with a timestamp that's a few hours in the future. Alternatively the same could occur for recently revoked sessions, that's seen as having been revoked in the future.

Other than that such scenarios would be pretty confusing (which I think would be fine), there's one edge case where that could lead to a recently created session being automatically revoked:

lightning-terminal/session_rpcserver.go

Lines 442 to 448 in c0e85e0

deadline := sess.CreatedAt.Add(s.cfg.firstConnectionDeadline)
if deadline.Before(time.Now()) {
log.Debugf("Deadline for session %x has already "+
"passed. Revoking session", pubKeyBytes)
return s.cfg.db.RevokeSession(pubKey)
}

But given that it's such an edge case where the worst case scenario would jsut be that a session gets revoked, we deemed this to be an ok tradeoff offline.

session.ServerAddr = string(serverAddr)
session.DevServer = devServer == 1
session.WithPrivacyMapper = privacy == 1
Expand All @@ -248,7 +248,7 @@ func DeserializeSession(r io.Reader) (*Session, error) {
}

if revokedAt != 0 {
session.RevokedAt = time.Unix(int64(revokedAt), 0)
session.RevokedAt = time.Unix(int64(revokedAt), 0).UTC()
}

if t, ok := parsedTypes[typeMacaroonRecipe]; ok && t == nil {
Expand Down
12 changes: 8 additions & 4 deletions session/tlv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,9 @@ func TestSerializeDeserializeSession(t *testing.T) {
priv, id, err := NewSessionPrivKeyAndID()
require.NoError(t, err)

session, err := NewSession(
session, err := buildSession(
id, priv, test.name, test.sessType,
time.Now(),
time.Date(99999, 1, 1, 0, 0, 0, 0, time.UTC),
"foo.bar.baz:1234", true, test.perms,
test.caveats, test.featureConfig, true,
Expand Down Expand Up @@ -183,8 +184,9 @@ func TestGroupIDForOlderSessions(t *testing.T) {
priv, id, err := NewSessionPrivKeyAndID()
require.NoError(t, err)

session, err := NewSession(
session, err := buildSession(
id, priv, "test-session", TypeMacaroonAdmin,
time.Now(),
time.Date(99999, 1, 1, 0, 0, 0, 0, time.UTC),
"foo.bar.baz:1234", true, nil, nil, nil, false, nil,
PrivacyFlags{},
Expand Down Expand Up @@ -218,8 +220,9 @@ func TestGroupID(t *testing.T) {
require.NoError(t, err)

// Create session 1 which is not linked to any previous session.
session1, err := NewSession(
session1, err := buildSession(
id, priv, "test-session", TypeMacaroonAdmin,
time.Now(),
time.Date(99999, 1, 1, 0, 0, 0, 0, time.UTC),
"foo.bar.baz:1234", true, nil, nil, nil, false, nil,
PrivacyFlags{},
Expand All @@ -232,8 +235,9 @@ func TestGroupID(t *testing.T) {
// Create session 2 and link it to session 1.
priv, id, err = NewSessionPrivKeyAndID()
require.NoError(t, err)
session2, err := NewSession(
session2, err := buildSession(
id, priv, "test-session", TypeMacaroonAdmin,
time.Now(),
time.Date(99999, 1, 1, 0, 0, 0, 0, time.UTC),
"foo.bar.baz:1234", true, nil, nil, nil, false,
&session1.GroupID, PrivacyFlags{},
Expand Down
4 changes: 2 additions & 2 deletions session_rpcserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ func (s *sessionRpcServer) AddSession(ctx context.Context,
return nil, err
}

sess, err := session.NewSession(
sess, err := s.cfg.db.NewSession(
id, localPrivKey, req.Label, typ, expiry, req.MailboxServerAddr,
req.DevServer, uniquePermissions, caveats, nil, false, nil,
session.PrivacyFlags{},
Expand Down Expand Up @@ -1148,7 +1148,7 @@ func (s *sessionRpcServer) AddAutopilotSession(ctx context.Context,
return nil, err
}

sess, err := session.NewSession(
sess, err := s.cfg.db.NewSession(
id, localPrivKey, req.Label, session.TypeAutopilot, expiry,
req.MailboxServerAddr, req.DevServer, perms, caveats,
clientConfig, privacy, linkedGroupID, privacyFlags,
Expand Down
Loading
Loading