Skip to content

[sql-37] firewalldb: more preparations for SQL actions store #1070

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 6 commits into from
May 15, 2025
Merged

[sql-37] firewalldb: more preparations for SQL actions store #1070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
297313e
firewalldb: add clock.Clock to firewalldb DB impls
ellemouton May 12, 2025
8f7312f
firewalldb: introduce AddActionReq
ellemouton May 12, 2025
b60dc29
firewall: do a full variable comparison for Actions test
ellemouton May 12, 2025
3e963c0
multi: rename Action.SessionID to MacaroonIdentifier
ellemouton May 12, 2025
f6e66db
litrpc: add macaroon_identifier to Action message
ellemouton May 13, 2025
9018cd4
firewalldb: ensure that tests can run in parallel
ellemouton May 13, 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
6 changes: 6 additions & 0 deletions app/src/types/generated/firewall_pb.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

56 changes: 55 additions & 1 deletion app/src/types/generated/firewall_pb.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions config_dev.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func NewStores(cfg *Config, clock clock.Clock) (*stores, error) {

acctStore := accounts.NewSQLStore(sqlStore.BaseDB, clock)
sessStore := session.NewSQLStore(sqlStore.BaseDB, clock)
firewallStore := firewalldb.NewSQLDB(sqlStore.BaseDB)
firewallStore := firewalldb.NewSQLDB(sqlStore.BaseDB, clock)

stores.accounts = acctStore
stores.sessions = sessStore
Expand All @@ -123,7 +123,7 @@ func NewStores(cfg *Config, clock clock.Clock) (*stores, error) {

acctStore := accounts.NewSQLStore(sqlStore.BaseDB, clock)
sessStore := session.NewSQLStore(sqlStore.BaseDB, clock)
firewallStore := firewalldb.NewSQLDB(sqlStore.BaseDB)
firewallStore := firewalldb.NewSQLDB(sqlStore.BaseDB, clock)

stores.accounts = acctStore
stores.sessions = sessStore
Expand Down Expand Up @@ -154,7 +154,7 @@ func NewStores(cfg *Config, clock clock.Clock) (*stores, error) {
}

firewallBoltDB, err := firewalldb.NewBoltDB(
networkDir, firewalldb.DBFilename, stores.sessions,
networkDir, firewalldb.DBFilename, stores.sessions, clock,
)
if err != nil {
return stores, fmt.Errorf("error creating firewall BoltDB: %v",
Expand Down
2 changes: 1 addition & 1 deletion config_prod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func NewStores(cfg *Config, clock clock.Clock) (*stores, error) {
stores.closeFns["sessions"] = sessStore.Close

firewallDB, err := firewalldb.NewBoltDB(
networkDir, firewalldb.DBFilename, sessStore,
networkDir, firewalldb.DBFilename, sessStore, clock,
)
if err != nil {
return stores, fmt.Errorf("error creating firewall DB: %v", err)
Expand Down
30 changes: 14 additions & 16 deletions firewall/request_logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"strings"
"sync"
"time"

"github.com/lightninglabs/lightning-terminal/firewalldb"
mid "github.com/lightninglabs/lightning-terminal/rpcmiddleware"
Expand Down Expand Up @@ -183,21 +182,20 @@ func (r *RequestLogger) addNewAction(ctx context.Context, ri *RequestInfo,
withPayloadData bool) error {

// If no macaroon is provided, then an empty 4-byte array is used as the
// session ID. Otherwise, the macaroon is used to derive a session ID.
var sessionID [4]byte
// macaroon ID. Otherwise, the last 4 bytes of the macaroon's root key
// ID are used.
var macaroonID [4]byte
if ri.Macaroon != nil {
var err error
sessionID, err = session.IDFromMacaroon(ri.Macaroon)
macaroonID, err = session.IDFromMacaroon(ri.Macaroon)
if err != nil {
return fmt.Errorf("could not extract ID from macaroon")
}
}

action := &firewalldb.Action{
SessionID: sessionID,
RPCMethod: ri.URI,
AttemptedAt: time.Now(),
State: firewalldb.ActionStateInit,
actionReq := &firewalldb.AddActionReq{
MacaroonIdentifier: macaroonID,
RPCMethod: ri.URI,
}

if withPayloadData {
Expand All @@ -211,19 +209,19 @@ func (r *RequestLogger) addNewAction(ctx context.Context, ri *RequestInfo,
return fmt.Errorf("unable to decode response: %v", err)
}

action.RPCParamsJson = jsonBytes
actionReq.RPCParamsJson = jsonBytes

meta := ri.MetaInfo
if meta != nil {
action.ActorName = meta.ActorName
action.FeatureName = meta.Feature
action.Trigger = meta.Trigger
action.Intent = meta.Intent
action.StructuredJsonData = meta.StructuredJsonData
actionReq.ActorName = meta.ActorName
actionReq.FeatureName = meta.Feature
actionReq.Trigger = meta.Trigger
actionReq.Intent = meta.Intent
actionReq.StructuredJsonData = meta.StructuredJsonData
}
}

locator, err := r.actionsDB.AddAction(ctx, action)
locator, err := r.actionsDB.AddAction(ctx, actionReq)
if err != nil {
return err
}
Expand Down
29 changes: 22 additions & 7 deletions firewalldb/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"time"

"github.com/lightninglabs/lightning-terminal/session"
"github.com/lightningnetwork/lnd/fn"
)

// ActionState represents the state of an action.
Expand All @@ -28,12 +29,21 @@ const (
ActionStateError ActionState = 3
)

// Action represents an RPC call made through the firewall.
type Action struct {
// SessionID is the ID of the session that this action belongs to.
// Note that this is not serialized on persistence since the action is
// already stored under a bucket identified by the session ID.
SessionID session.ID
// AddActionReq is the request that is used to add a new Action to the database.
// It contains all the information that is needed to create a new Action in the
// ActionStateInit State.
type AddActionReq struct {
// MacaroonIdentifier is a 4 byte identifier created from the last 4
// bytes of the root key ID of the macaroon used to perform the action.
MacaroonIdentifier [4]byte
Copy link
Contributor

Choose a reason for hiding this comment

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

it's probably not worth it to have another type alias for [4]byte?

Copy link
Member Author

Choose a reason for hiding this comment

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

currently we dont use this in other places - so i think it's ok for now


// SessionID holds the optional session ID of the session that this
// action was performed with.
//
// NOTE: for our BoltDB impl, this is not persisted in any way, and we
// populate it by casting the macaroon ID to a session.ID and so is not
// guaranteed to be linked to an existing session.
SessionID fn.Option[session.ID]
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's a test anywhere for an intercepted action, where we don't set this (i.e. it's not related to a session request).

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 i'll expand on the tests for SQL (where it actually will matter if it is set or not) once we've added the sql impl here 👍


// ActorName is the name of the entity who performed the Action.
ActorName string
Expand All @@ -59,6 +69,11 @@ type Action struct {

// RPCParams is the method parameters of the request in JSON form.
RPCParamsJson []byte
}

// Action represents an RPC call made through the firewall.
type Action struct {
AddActionReq

// AttemptedAt is the time at which this action was created.
AttemptedAt time.Time
Expand Down Expand Up @@ -181,7 +196,7 @@ func WithActionState(state ActionState) ListActionOption {
// ActionsWriteDB is an abstraction over the Actions DB that will allow a
// caller to add new actions as well as change the values of an existing action.
type ActionsWriteDB interface {
AddAction(ctx context.Context, action *Action) (ActionLocator, error)
AddAction(ctx context.Context, req *AddActionReq) (ActionLocator, error)
SetActionState(ctx context.Context, al ActionLocator,
state ActionState, errReason string) error
}
Expand Down
18 changes: 13 additions & 5 deletions firewalldb/actions_kvdb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/lightninglabs/lightning-terminal/session"
"github.com/lightningnetwork/lnd/fn"
"github.com/lightningnetwork/lnd/tlv"
"go.etcd.io/bbolt"
)
Expand Down Expand Up @@ -53,8 +54,14 @@ var (
)

// AddAction serialises and adds an Action to the DB under the given sessionID.
func (db *BoltDB) AddAction(_ context.Context, action *Action) (ActionLocator,
error) {
func (db *BoltDB) AddAction(_ context.Context,
req *AddActionReq) (ActionLocator, error) {

action := &Action{
AddActionReq: *req,
AttemptedAt: db.clock.Now().UTC(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that the time is set to the clocks .Now().UTC(), while the previous version set it to time.Now() (i.e. not UTC). I think the new implementation is probably more correct, but wanted to note that. Could this cause any issues, anddo we need to update previous inserted actions to represent that in some way?

Copy link
Member Author

@ellemouton ellemouton May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yeah - similarly to sessions/accounts - we're going with "it may just be off by a couple of hours and that's ok" since mostly these timestamps are just metadata for display to the user - we dont use them (much) in logic.

So yeah - i think not worth a whole migration just to fix this

State: ActionStateInit,
}

var buf bytes.Buffer
if err := SerializeAction(&buf, action); err != nil {
Expand All @@ -74,7 +81,7 @@ func (db *BoltDB) AddAction(_ context.Context, action *Action) (ActionLocator,
}

sessBucket, err := actionsBucket.CreateBucketIfNotExists(
action.SessionID[:],
action.MacaroonIdentifier[:],
)
if err != nil {
return err
Expand Down Expand Up @@ -103,7 +110,7 @@ func (db *BoltDB) AddAction(_ context.Context, action *Action) (ActionLocator,
}

locator = kvdbActionLocator{
sessionID: action.SessionID,
sessionID: action.MacaroonIdentifier,
actionID: nextActionIndex,
}

Expand Down Expand Up @@ -543,7 +550,8 @@ func DeserializeAction(r io.Reader, sessionID session.ID) (*Action, error) {
return nil, err
}

action.SessionID = sessionID
action.MacaroonIdentifier = sessionID
action.SessionID = fn.Some(sessionID)
action.ActorName = string(actor)
action.FeatureName = string(featureName)
action.Trigger = string(trigger)
Expand Down
Loading
Loading