terminal: auto-bake super macaroon on startup#1324
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces an automated mechanism for baking a 'super macaroon' during the application startup process. It allows users to specify the desired permission scope (read-only or read-write) via configuration flags. The system ensures that if a macaroon already exists, its permissions are validated against the current configuration, and it is automatically regenerated if discrepancies are detected, ensuring the security and consistency of access credentials. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces configuration options (--bake-super-macaroon and --super-macaroon-path) to automatically bake a super macaroon on startup, along with corresponding validation, integration tests, and release notes. The review feedback highlights several important improvement opportunities: validating that the macaroon path is not empty when baking is enabled, propagating and handling errors from setupSuperMacaroon during startup to prevent silent failures, and adding a nil check when iterating over decoded macaroon operations to avoid potential nil pointer dereferences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if cfg.BakeSuperMacaroon != defaultBakeSuperMacaroon && | ||
| cfg.SuperMacaroonPath != "" { | ||
|
|
||
| cfg.SuperMacaroonPath = lncfg.CleanAndExpandPath( | ||
| cfg.SuperMacaroonPath, | ||
| ) | ||
| dir := filepath.Dir(cfg.SuperMacaroonPath) | ||
| if err := os.MkdirAll(dir, 0700); err != nil { | ||
| return nil, fmt.Errorf("unable to create super "+ | ||
| "macaroon directory %v: %w", dir, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
If bake-super-macaroon is enabled (i.e., not set to "none"), but super-macaroon-path is empty, the configuration validation will pass but the daemon will fail to write the macaroon at runtime, leading to a silent failure. We should validate that SuperMacaroonPath is not empty when baking is enabled.
| if cfg.BakeSuperMacaroon != defaultBakeSuperMacaroon && | |
| cfg.SuperMacaroonPath != "" { | |
| cfg.SuperMacaroonPath = lncfg.CleanAndExpandPath( | |
| cfg.SuperMacaroonPath, | |
| ) | |
| dir := filepath.Dir(cfg.SuperMacaroonPath) | |
| if err := os.MkdirAll(dir, 0700); err != nil { | |
| return nil, fmt.Errorf("unable to create super "+ | |
| "macaroon directory %v: %w", dir, err) | |
| } | |
| } | |
| if cfg.BakeSuperMacaroon != defaultBakeSuperMacaroon { | |
| if cfg.SuperMacaroonPath == "" { | |
| return nil, fmt.Errorf("super-macaroon-path cannot be empty when bake-super-macaroon is enabled") | |
| } | |
| cfg.SuperMacaroonPath = lncfg.CleanAndExpandPath( | |
| cfg.SuperMacaroonPath, | |
| ) | |
| dir := filepath.Dir(cfg.SuperMacaroonPath) | |
| if err := os.MkdirAll(dir, 0700); err != nil { | |
| return nil, fmt.Errorf("unable to create super "+ | |
| "macaroon directory %v: %w", dir, err) | |
| } | |
| } |
Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
448cdb7 to
b9e352b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to automatically bake a super macaroon on startup based on configuration options (--bake-super-macaroon and --super-macaroon-path). It includes logic to verify if an existing macaroon matches the required permissions and regenerates it if necessary, along with integration tests and release notes. The feedback suggests adding upfront validation during configuration loading to ensure super-macaroon-path is not empty when baking is enabled, preventing a failure later during startup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if cfg.BakeSuperMacaroon != defaultBakeSuperMacaroon && | ||
| cfg.SuperMacaroonPath != "" { | ||
|
|
||
| cfg.SuperMacaroonPath = lncfg.CleanAndExpandPath( | ||
| cfg.SuperMacaroonPath, | ||
| ) | ||
| dir := filepath.Dir(cfg.SuperMacaroonPath) | ||
| if err := makeDirectories(dir); err != nil { | ||
| return nil, fmt.Errorf("unable to create super "+ | ||
| "macaroon directory %v: %w", dir, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
If bake-super-macaroon is enabled (i.e., not none), but super-macaroon-path is configured as an empty string, the configuration loading will succeed but the daemon will fail to start later when attempting to write the baked macaroon to an empty path. It is better to validate this upfront and return a clear configuration error.
if cfg.BakeSuperMacaroon != defaultBakeSuperMacaroon {
if cfg.SuperMacaroonPath == "" {
return nil, fmt.Errorf("super-macaroon-path cannot be empty " +
"when bake-super-macaroon is enabled")
}
cfg.SuperMacaroonPath = lncfg.CleanAndExpandPath(
cfg.SuperMacaroonPath,
)
dir := filepath.Dir(cfg.SuperMacaroonPath)
if err := makeDirectories(dir); err != nil {
return nil, fmt.Errorf("unable to create super " +
"macaroon directory %v: %w", dir, err)
}
}
|
I’ll fix the lint error after a follow-up review. |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Awesome, thanks for this @Cyberguru1 🎉! Generally this looks great, but leaving some suggestions below for some additional improvements 🔥
| if g.cfg.statelessInitMode { | ||
| log.Infof("Stateless init mode is active, skipping writing " + | ||
| "super macaroon to disk") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I actually think it make sense to check this in the config validation instead of here, and fail the validation if g.cfg.BakeSuperMacaroon != noneChoice & g.cfg.statelessInitMode.
There was a problem hiding this comment.
I think the check for statelessInitMode wouldn’t be possible during config validation since it isn’t a static config value. It gets determined dynamically at runtime during startup, after connecting to LND.
Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
b9e352b to
ce2927d
Compare
Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
ce2927d to
c5fdd1e
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to automatically bake a super macaroon on startup and keep its permissions in sync with active sub-servers, adding the --bake-super-macaroon and --super-macaroon-path configuration options. It includes helper functions to verify macaroon permissions, integrates the setup into the daemon's startup sequence, and adds comprehensive integration and unit tests. The review feedback suggests a robustness improvement: instead of returning an error and halting startup when a macaroon is corrupted or invalid, the parsing failures should be treated as a mismatch, allowing the daemon to automatically regenerate and overwrite the file.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| mac := &macaroon.Macaroon{} | ||
| if err := mac.UnmarshalBinary(macBytes); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| rawID := mac.Id() | ||
| if len(rawID) == 0 || rawID[0] != byte(bakery.LatestVersion) { | ||
| return false, errors.New("invalid macaroon version") | ||
| } | ||
|
|
||
| decodedID := &lnrpc.MacaroonId{} | ||
| if err := proto.Unmarshal(rawID[1:], decodedID); err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
If the macaroon file on disk is corrupted, has an invalid version (e.g., after a library upgrade), or cannot be unmarshaled, returning an error here will cause the daemon to halt startup and force the user to manually delete the file.
Since the goal of the auto-bake feature is to automatically manage and keep the super macaroon in sync, we should treat any parsing or validation failure as a mismatch (false, nil). This allows the startup process to automatically regenerate and overwrite the invalid macaroon, making the daemon much more robust.
| mac := &macaroon.Macaroon{} | |
| if err := mac.UnmarshalBinary(macBytes); err != nil { | |
| return false, err | |
| } | |
| rawID := mac.Id() | |
| if len(rawID) == 0 || rawID[0] != byte(bakery.LatestVersion) { | |
| return false, errors.New("invalid macaroon version") | |
| } | |
| decodedID := &lnrpc.MacaroonId{} | |
| if err := proto.Unmarshal(rawID[1:], decodedID); err != nil { | |
| return false, err | |
| } | |
| mac := &macaroon.Macaroon{} | |
| if err := mac.UnmarshalBinary(macBytes); err != nil { | |
| return false, nil | |
| } | |
| rawID := mac.Id() | |
| if len(rawID) == 0 || rawID[0] != byte(bakery.LatestVersion) { | |
| return false, nil | |
| } | |
| decodedID := &lnrpc.MacaroonId{} | |
| if err := proto.Unmarshal(rawID[1:], decodedID); err != nil { | |
| return false, nil | |
| } |
|
Thank you for taking the time to review @ViktorT-11 🙏 , I have addressed your feedbacks. |
Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
c5fdd1e to
8c5467a
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Thanks for the updates @Cyberguru1 🙏!
This looks great and I've tested locally that this works as expected 🔥! I think this looks good to go from my end once you've fixed the merge conflicts & addressed some minor comments I've added below.
Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
8c5467a to
8ac18cf
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces configuration options (--bake-super-macaroon and --super-macaroon-path) to automatically bake a super macaroon on startup with permissions matching the active sub-servers, regenerating it if permissions differ. It also includes corresponding integration and unit tests. The feedback suggests two improvements: validating that super-macaroon-path is not empty when baking is enabled to prevent startup failures, and gracefully regenerating the macaroon with a warning if the existing file is corrupted, rather than failing startup entirely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| err = macaroons.ValidateSuperMacaroonPath( | ||
| cfg.SuperMacaroonPath, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
If bake-super-macaroon is enabled, the super-macaroon-path must not be empty. Otherwise, the daemon will pass config validation but fail during startup when attempting to write the macaroon to an empty path. Adding a validation check ensures this configuration error is caught early.
err = macaroons.ValidateSuperMacaroonPath(
cfg.SuperMacaroonPath,
)
if err != nil {
return nil, err
}
if cfg.BakeSuperMacaroon != defaultBakeSuperMacaroon &&
cfg.SuperMacaroonPath == "" {
return nil, fmt.Errorf("super-macaroon-path cannot be empty " +
"when bake-super-macaroon is enabled")
}| if litmac.SuperMacaroonExists(path) { | ||
| matches, err := litmac.MacaroonMatchesPermissions( | ||
| path, activePerms, | ||
| ) | ||
|
|
||
| if err == nil && matches { | ||
| log.Debugf("Super macaroon already exists at "+ | ||
| "%v and matches configuration, "+ | ||
| "skipping bake", path) | ||
|
|
||
| return nil | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf( | ||
| "unable to verify super macaroon permissions: "+ | ||
| "%w; please delete the macaroon at %v "+ | ||
| "if the issue persists", err, path, | ||
| ) | ||
| } | ||
|
|
||
| log.Infof("Super macaroon permissions " + | ||
| "differ from configuration, " + | ||
| "regenerating...") | ||
| } |
There was a problem hiding this comment.
If the existing super macaroon file is corrupted or invalid, MacaroonMatchesPermissions will return an error, causing the daemon to fail startup. Since the goal is to ensure a valid super macaroon exists with the correct permissions, it is more robust to log a warning and automatically regenerate/overwrite the corrupted file instead of failing startup.
if litmac.SuperMacaroonExists(path) {
matches, err := litmac.MacaroonMatchesPermissions(
path, activePerms,
)
if err != nil {
log.Warnf("Unable to verify super macaroon permissions: %v, " +
"regenerating...", err)
} else if matches {
log.Debugf("Super macaroon already exists at " +
"%v and matches configuration, " +
"skipping bake", path)
return nil
} else {
log.Infof("Super macaroon permissions " +
"differ from configuration, " +
"regenerating...")
}
}Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
8ac18cf to
830f547
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to automatically bake a super macaroon on startup and keep its permissions in sync, adding the --bake-super-macaroon and --super-macaroon-path configuration options. It includes helper functions to validate macaroon paths, check if they exist, and verify if their permissions match the active configuration, along with comprehensive integration and unit tests. The review feedback suggests improving error wrapping in terminal.go by placing the error argument last in fmt.Errorf, and adding a constant for the "read-write" choice in config.go to avoid magic strings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| // readOnlyChoice is the read-only choice for the bake-super-macaroon | ||
| // configuration option. | ||
| readOnlyChoice = "read-only" |
There was a problem hiding this comment.
For consistency with noneChoice and readOnlyChoice, and to avoid using a magic string for the "read-write" option, it would be best to add a constant for it.
| readOnlyChoice = "read-only" | |
| readOnlyChoice = "read-only" | |
| // readWriteChoice is the read-write choice for the bake-super-macaroon | |
| // configuration option. | |
| readWriteChoice = "read-write" |
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
Nice! Thanks a lot for this @Cyberguru1, it's really appreciated that you keep contributing 🙏!
Leaving some minor nonblocking feedback below.
tACK LGTM 🔥
bitromortac
left a comment
bitromortac
left a comment
There was a problem hiding this comment.
LGTM, nice addition 🙏🎉
Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
830f547 to
330fd82
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to automatically bake a super macaroon on startup and keep its permissions in sync with the active sub-servers. Feedback focuses on improving robustness and user experience: specifically, validating the macaroon path only when the feature is enabled, gracefully handling corrupted macaroon files during startup by logging a warning and regenerating them, and writing the macaroon file atomically using a temporary file to prevent corruption.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if err != nil { | ||
| return fmt.Errorf( | ||
| "unable to verify super macaroon "+ | ||
| "permissions at %v, please delete "+ | ||
| "it if the issue persists: %w", | ||
| path, err, | ||
| ) | ||
| } |
There was a problem hiding this comment.
If the super macaroon file is corrupted or invalid, blocking the entire daemon startup and requiring manual user intervention to delete the file is a poor user experience. Since the daemon is configured to automatically bake the macaroon, it is much more robust to log a warning and automatically regenerate/overwrite the invalid file.
if err != nil {
log.Warnf("Unable to verify super macaroon "+
"permissions at %v, regenerating: %v",
path, err)
}| if err := os.WriteFile(path, superMacBytes, 0600); err != nil { | ||
| return fmt.Errorf("unable to write super macaroon to %v: %w", | ||
| path, err) | ||
| } |
There was a problem hiding this comment.
Writing directly to the target macaroon file using os.WriteFile is not atomic and can result in file corruption if the write is interrupted (e.g., due to disk full, power loss, or process termination). It is safer to write to a temporary file first and then atomically rename it to the target path. Deleting the target path first ensures cross-platform compatibility on Windows where os.Rename fails if the destination already exists.
tempPath := path + ".tmp"
if err := os.WriteFile(tempPath, superMacBytes, 0600); err != nil {
return fmt.Errorf("unable to write super macaroon to %v: %w",
tempPath, err)
}
_ = os.Remove(path)
if err := os.Rename(tempPath, path); err != nil {
_ = os.Remove(tempPath)
return fmt.Errorf("unable to rename super macaroon to %v: %w",
path, err)
}Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
330fd82 to
c4fba1a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to automatically bake a super macaroon on startup based on active sub-servers, adding the --bake-super-macaroon and --super-macaroon-path configuration options. Feedback on these changes suggests cleaning and expanding the super macaroon path before validating its suffix to avoid unexpected validation failures. Additionally, it is recommended to explicitly call os.Chmod after writing the macaroon file to guarantee strict file permissions are enforced even if the file already exists.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| err = macaroons.HasMacaroonSuffix( | ||
| cfg.SuperMacaroonPath, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Clean and expand the super macaroon path | ||
| cfg.SuperMacaroonPath = lncfg.CleanAndExpandPath( | ||
| cfg.SuperMacaroonPath, | ||
| ) |
There was a problem hiding this comment.
It is highly recommended to clean and expand the super macaroon path using lncfg.CleanAndExpandPath before validating its suffix with macaroons.HasMacaroonSuffix. If the path contains a trailing slash, relative segments, or a tilde (~), validating it before cleaning can lead to unexpected validation failures.
| err = macaroons.HasMacaroonSuffix( | |
| cfg.SuperMacaroonPath, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Clean and expand the super macaroon path | |
| cfg.SuperMacaroonPath = lncfg.CleanAndExpandPath( | |
| cfg.SuperMacaroonPath, | |
| ) | |
| // Clean and expand the super macaroon path | |
| cfg.SuperMacaroonPath = lncfg.CleanAndExpandPath( | |
| cfg.SuperMacaroonPath, | |
| ) | |
| err = macaroons.HasMacaroonSuffix( | |
| cfg.SuperMacaroonPath, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } |
| if err := os.WriteFile(path, superMacBytes, 0600); err != nil { | ||
| return fmt.Errorf("unable to write super macaroon to %v: %w", | ||
| path, err) | ||
| } |
There was a problem hiding this comment.
Using os.WriteFile with 0600 permissions will not modify the permissions of the file if it already exists. Since super macaroons contain highly sensitive credentials, we should explicitly call os.Chmod(path, 0600) after writing to ensure that the file permissions are strictly restricted to the owner.
if err := os.WriteFile(path, superMacBytes, 0600); err != nil {
return fmt.Errorf("unable to write super macaroon to %v: %w",
path, err)
}
if err := os.Chmod(path, 0600); err != nil {
return fmt.Errorf("unable to set permissions on super macaroon: %w", err)
}
Introduce helper functions in the macaroons package to manage super macaroon files and permissions. Add SuperMacaroonExists, MacaroonMatchesPermissions, and BakeAndWriteSuperMacaroon.
Introduce configuration flags to enable baking a super macaroon on startup. Add --bake-super-macaroon (none, read-only, read-write) and --super-macaroon-path. Also add validation logic to ensure that the configured super macaroon path ends with the expected '.macaroon' suffix, rejecting startup early otherwise.
Automatically bake a super macaroon on startup if it doesn't already exist on disk and the `bake-super-macaroon` option is configured. On startup, the node verifies if the super macaroon file exists. If it does, it parses the macaroon, extracts and verifies the version and root key ID, and asserts that the macaroon permissions exactly match the expected active permissions. If there is a mismatch, the macaroon is regenerated and overwritten on disk. If the file does not exist, a new super macaroon is baked and written directly to disk. Also validate that the `bake-super-macaroon` option is not enabled when LND is running in stateless initialization mode, failing startup early if they are used together.
Add testSuperMacaroonOnStartup to verify startup baking logic. The test restarts the node with the auto-baking config flags and asserts the macaroon is baked with read-only or read-write permissions accordingly. Also add validation tests verifying that starting with an invalid path suffix or in stateless-init mode with baking enabled fails as expected. Verify permission addition/expansion by starting the node with sub-servers disabled and then restarting with sub-servers re-enabled. Also verify the none config choice, asserting that no super macaroon file is baked/created on startup.
Add release notes for the new auto-bake config options in release notes for version 0.17.0 and reference pull request lightninglabs#1324.
c4fba1a to
8a89643
Compare
Add release notes for the new auto-bake config options in release notes for version 0.17.1 and reference pull request lightninglabs#1324.
8a89643 to
16a4f79
Compare
|
@Cyberguru1, remember to re-request review from reviewers when ready |
4 participants
closes #910
This PR implements the ability to automatically bake a super macaroon on startup if configured and it doesn't already exist on disk, and validates/regenerates it dynamically if the active permissions change.