Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 3 additions & 7 deletions cmd/litcli/sessions.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ var addSessionCommand = cli.Command{
Usage: "The session type to be created which will " +
"determine the permissions a user has when " +
"connecting with the session; options " +
"include readonly|admin|account|custom.",
"include readonly|admin|custom.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Removing the account type from the CLI is a breaking change for users who rely on this preset. While the new --account_id flag allows for more granular control, the account type still serves as a useful preset for the standard set of permissions defined in accounts.MacaroonPermissions. It is recommended to keep it as an option.

include readonly|admin|account|custom.",

Value: "readonly",
},
cli.StringSliceFlag{
Expand All @@ -87,10 +87,8 @@ var addSessionCommand = cli.Command{
},
cli.StringFlag{
Name: "account_id",
Usage: "The account id that should be used for " +
"the account session. Note that this flag " +
"will only be used if the 'type' flag is " +
"set to 'account'.",
Usage: "An optional account ID to restrict the " +
"session macaroon to a specific account.",
},
},
}
Expand Down Expand Up @@ -146,8 +144,6 @@ func parseSessionType(sessionType string) (litrpc.SessionType, error) {
return litrpc.SessionType_TYPE_MACAROON_ADMIN, nil
case "readonly":
return litrpc.SessionType_TYPE_MACAROON_READONLY, nil
case "account":
return litrpc.SessionType_TYPE_MACAROON_ACCOUNT, nil
case "custom":
return litrpc.SessionType_TYPE_MACAROON_CUSTOM, nil
Comment on lines 147 to 148

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since the account session type is still supported by the RPC server and provides a specific set of permissions, it should remain available in the CLI parser to avoid breaking existing scripts and to provide a convenient preset for users.

Suggested change
case "custom":
return litrpc.SessionType_TYPE_MACAROON_CUSTOM, nil
case "account":
return litrpc.SessionType_TYPE_MACAROON_ACCOUNT, nil
case "custom":
return litrpc.SessionType_TYPE_MACAROON_CUSTOM, nil

default:
Expand Down
42 changes: 26 additions & 16 deletions session_rpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -314,17 +314,13 @@
// the macaroons are baked correctly when creating the session.
case session.TypeMacaroonAdmin, session.TypeMacaroonReadonly:

// For account based sessions we just add the account ID caveat, the
// permissions are added dynamically when creating the session.
// For account based sessions we require an account ID to be set.
case session.TypeMacaroonAccount:
id, err := accounts.ParseAccountID(req.AccountId)
if err != nil {
return nil, fmt.Errorf("invalid account ID: %v", err)
if req.AccountId == "" {
return nil, fmt.Errorf("account_id must be set for " +
"the account session type")
}

caveats = append(caveats, accounts.CaveatFromID(*id))
accountID = fn.Some(*id)

// For the custom macaroon type, we use the custom permissions specified
// in the request. For the time being, the caveats list will be empty
// for this type.
Expand Down Expand Up @@ -389,6 +385,18 @@
"using AddAutoPilotSession method")
}

// If an account ID was provided, bind the macaroon to that account
// regardless of session type.
if req.AccountId != "" {
id, err := accounts.ParseAccountID(req.AccountId)
if err != nil {
return nil, fmt.Errorf("invalid account ID: %v", err)
}

caveats = append(caveats, accounts.CaveatFromID(*id))
accountID = fn.Some(*id)
}

// Collect the de-duped permissions.
var uniquePermissions []bakery.Op
for entity, actions := range permissions {
Expand Down Expand Up @@ -473,8 +481,9 @@
readOnly = sess.Type == session.TypeMacaroonReadonly
)
switch sess.Type {
// For the default session types we use empty caveats and permissions,
// the macaroons are baked correctly when creating the session.
// For the default session types, permissions are baked correctly when
// creating the session. Any stored caveats (e.g. account binding) are
// applied below after the switch.
case session.TypeMacaroonAdmin, session.TypeMacaroonReadonly:
permissions = s.cfg.permMgr.ActivePermissions(readOnly)

Expand All @@ -486,26 +495,27 @@
"recipe to be set")
}

caveats = sess.MacaroonRecipe.Caveats
permissions = accounts.MacaroonPermissions

// For custom session types, we use the caveats and permissions that
// were persisted on session creation.
case session.TypeMacaroonCustom, session.TypeAutopilot:
if sess.MacaroonRecipe == nil {
break
if sess.MacaroonRecipe != nil {
permissions = sess.MacaroonRecipe.Permissions
}
Comment on lines +503 to 505

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For TypeMacaroonCustom and TypeAutopilot, the session's permissions are stored within the MacaroonRecipe. If the recipe is missing, the session is in an invalid state and should fail to resume, similar to how TypeMacaroonAccount is handled above. Proceeding with empty permissions would result in a session that cannot perform any actions.

Suggested change
if sess.MacaroonRecipe != nil {
permissions = sess.MacaroonRecipe.Permissions
}
if sess.MacaroonRecipe == nil {
return fmt.Errorf("invalid session, expected " +
"recipe to be set")
}
permissions = sess.MacaroonRecipe.Permissions


permissions = sess.MacaroonRecipe.Permissions
caveats = append(caveats, sess.MacaroonRecipe.Caveats...)

// No other types are currently supported.
default:
log.Debugf("Not resuming session %x with type %d", pubKeyBytes,
sess.Type)
return nil
}

// Apply any stored caveats (e.g. account binding) for all session types.

Check failure on line 514 in session_rpcserver.go

View workflow job for this annotation

GitHub Actions / lint

the line is 81 characters long, which exceeds the maximum of 80 characters. (ll)

Check failure on line 514 in session_rpcserver.go

View workflow job for this annotation

GitHub Actions / lint

the line is 81 characters long, which exceeds the maximum of 80 characters. (ll)
if sess.MacaroonRecipe != nil {
caveats = append(caveats, sess.MacaroonRecipe.Caveats...)
}

// Add the session expiry as a macaroon caveat.
macExpiry := checkers.TimeBeforeCaveat(sess.Expiry)
caveats = append(caveats, macaroon.Caveat{
Expand Down
Loading