-
Notifications
You must be signed in to change notification settings - Fork 53
fix: set explicit permissions on the LND admin macaroon #890
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.
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
Open
AndreaDiazCorreia
wants to merge
10
commits into
main
Choose a base branch
from
fix/lnd-macaroon-file-permissions
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
36560cf
fix(docker): set restrictive permissions on LND credentials and warn …
AndreaDiazCorreia 985fce8
docs: document admin macaroon permissions and use install(1) for cred…
AndreaDiazCorreia 17a7a91
docs: warn at startup when LND admin macaroon is reachable by other u…
AndreaDiazCorreia 519c5c7
fix(docker): run container as host user instead of requiring chown on…
AndreaDiazCorreia cad7930
docs: clarify admin macaroon permission guidance to preserve group ac…
AndreaDiazCorreia 9ba71b4
docs: restrict settings directory and file to owner-only permissions …
AndreaDiazCorreia fc967ae
fix(config): refuse to follow symlinks when creating settings.toml fr…
AndreaDiazCorreia f45efe6
refactor(config): consolidate settings file creation primitives in pe…
AndreaDiazCorreia 38085ad
docs: clarify symlink handling in settings directory creation vs file…
AndreaDiazCorreia dc24e11
docs: document atomic .env writes and expand permission check coverag…
AndreaDiazCorreia File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # The Docker build context is the repository root (docker/compose.yml sets | ||
| # `context: ..`) and both Dockerfiles pull it into the builder stage with | ||
| # `COPY . .`. `make docker-build` stages the LND cert and admin macaroon in | ||
| # docker/config/lnd/ right before `docker compose build`, so without this entry | ||
| # a spend-capable credential would be shipped into the build context and left | ||
| # in the builder layer and its cache. .gitignore already keeps it out of git, | ||
| # but Docker does not read .gitignore. | ||
| docker/config/ | ||
| docker/.env |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| //! Startup checks on the filesystem permissions of the credential files | ||
| //! mostrod reads. | ||
| //! | ||
| //! Kept in its own module — rather than folded into `config::util` — because | ||
| //! these checks are about the files named by the settings, not about loading | ||
| //! the settings themselves. | ||
|
|
||
| use std::path::Path; | ||
|
|
||
| /// Warn when a credential file has any of its "other" permission bits set, | ||
| /// which puts it within reach of every account on the host. | ||
| /// | ||
| /// The check is advisory: a node whose macaroon is `0644` still starts, it | ||
| /// just says so out loud once per boot. Refusing to start would turn a | ||
| /// hardening gap into an outage on the next upgrade for every operator who | ||
| /// already runs that way. | ||
| /// | ||
| /// Empty paths and files that cannot be stat'ed are ignored: an unset or | ||
| /// unreadable path is not a permissions problem, and the real failure is | ||
| /// reported with far more context by whoever opens the file (for the macaroon, | ||
| /// `LndConnector::new`). | ||
| pub fn warn_if_other_accessible(path: &str, label: &str) { | ||
| if path.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| if let Some(mode) = other_accessible_mode(Path::new(path)) { | ||
| // `chmod o=` rather than `chmod 600`: the file may legitimately be | ||
| // owned by the LND account and read by mostrod through the node's | ||
| // group, and following advice that drops the group bits would leave | ||
| // the daemon unable to authenticate on its next restart. | ||
| tracing::warn!( | ||
| "{label} ({path}) has permissions {mode:04o}: its \"other\" bits are set, so every \ | ||
| account on this host can reach it. Clear them with: chmod o= {path}" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /// The file's permission bits when the "other" class has any of them set, | ||
| /// `None` otherwise. | ||
| /// | ||
| /// Group access is deliberately tolerated: LND itself creates | ||
| /// `admin.macaroon` with mode `0640`, and granting a service account access | ||
| /// through the node's group is a legitimate deployment, not a finding. | ||
| /// | ||
| /// Only the mode bits are read. A POSIX ACL can widen access without touching | ||
| /// them, so a quiet startup means "the mode bits are sane", not "no other | ||
| /// account can read this file" — the check is a cheap guard against the | ||
| /// documented copy-it-into-place flows, not an audit. | ||
| /// | ||
| /// `metadata` follows symlinks on purpose — pointing `lnd_macaroon_file` at a | ||
| /// link is common, and what matters is the mode of the file that is actually | ||
| /// read. | ||
| #[cfg(unix)] | ||
| fn other_accessible_mode(path: &Path) -> Option<u32> { | ||
| use std::os::unix::fs::PermissionsExt; | ||
|
|
||
| let mode = std::fs::metadata(path).ok()?.permissions().mode() & 0o777; | ||
| (mode & 0o007 != 0).then_some(mode) | ||
|
AndreaDiazCorreia marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /// Non-Unix platforms have no POSIX permission bits to inspect. | ||
| #[cfg(not(unix))] | ||
| fn other_accessible_mode(_path: &Path) -> Option<u32> { | ||
| None | ||
| } | ||
|
|
||
| #[cfg(all(test, unix))] | ||
| mod tests { | ||
| use super::*; | ||
| use std::os::unix::fs::PermissionsExt; | ||
| use std::path::PathBuf; | ||
|
|
||
| fn temp_dir(tag: &str) -> PathBuf { | ||
| let dir = | ||
| std::env::temp_dir().join(format!("mostro-permissions-{tag}-{}", std::process::id())); | ||
| std::fs::create_dir_all(&dir).expect("create temp dir"); | ||
| dir | ||
| } | ||
|
|
||
| fn macaroon_with_mode(tag: &str, mode: u32) -> PathBuf { | ||
| let path = temp_dir(tag).join("admin.macaroon"); | ||
| std::fs::write(&path, b"macaroon").expect("write macaroon"); | ||
| std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)) | ||
| .expect("set permissions"); | ||
| path | ||
| } | ||
|
|
||
| #[test] | ||
| fn owner_only_is_accepted() { | ||
| let path = macaroon_with_mode("owner-only", 0o600); | ||
| assert_eq!(other_accessible_mode(&path), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn group_readable_is_accepted() { | ||
| // 0640 is the mode LND writes admin.macaroon with, and reaching it | ||
| // through the node's group is a supported setup. | ||
| let path = macaroon_with_mode("group-read", 0o640); | ||
| assert_eq!(other_accessible_mode(&path), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn world_readable_is_reported() { | ||
| let path = macaroon_with_mode("world-read", 0o644); | ||
| assert_eq!(other_accessible_mode(&path), Some(0o644)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn other_read_without_group_read_is_reported() { | ||
| let path = macaroon_with_mode("other-read", 0o604); | ||
| assert_eq!(other_accessible_mode(&path), Some(0o604)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn world_writable_is_reported() { | ||
| // Not a disclosure by itself, but a local account that can replace the | ||
| // credential mostrod authenticates with is the same class of problem. | ||
| let path = macaroon_with_mode("world-write", 0o602); | ||
| assert_eq!(other_accessible_mode(&path), Some(0o602)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn symlink_reports_the_target_mode() { | ||
| let dir = temp_dir("symlink"); | ||
| let target = dir.join("real.macaroon"); | ||
| std::fs::write(&target, b"macaroon").expect("write macaroon"); | ||
| std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o644)) | ||
| .expect("set permissions"); | ||
|
|
||
| let link = dir.join("linked.macaroon"); | ||
| let _ = std::fs::remove_file(&link); | ||
| std::os::unix::fs::symlink(&target, &link).expect("create symlink"); | ||
|
|
||
| assert_eq!(other_accessible_mode(&link), Some(0o644)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn missing_file_is_ignored() { | ||
| assert_eq!( | ||
| other_accessible_mode(Path::new("/definitely/not/here.macaroon")), | ||
| None | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn empty_and_missing_paths_do_not_panic() { | ||
| warn_if_other_accessible("", "LND admin macaroon"); | ||
| warn_if_other_accessible("/definitely/not/here.macaroon", "LND admin macaroon"); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.