Skip to content
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

Jump to bottom

lightningd/jsonrpc.c: Set JSON-RPC socket permissions by command line. #3437

Merged
merged 1 commit into from
Jan 27, 2020
Merged

lightningd/jsonrpc.c: Set JSON-RPC socket permissions by command line. #3437

merged 1 commit into from
Jan 27, 2020

Conversation

ZmnSCPxj
Copy link
Contributor

@ZmnSCPxj ZmnSCPxj commented Jan 24, 2020 •
edited
Loading

Fixes: #1366

Arguably also works for #3394. It is probably more sensible to set permissions via this patch and --rpc-file-mode, since this will work in a new install or if you place the RPC file in a tmpfs (which might make more sense as well, so that at startup the rpc file does not exist yet, so you can wait on the file existing to determine if you can now connect to it and the correct lightningd already is listening).

@ZmnSCPxj
lightningd/jsonrpc.c: Set JSON-RPC socket permissions by command line.
5c28d1f 
Changelog-Added: Can now set the permissions on the JSON-RPC socket by `--rpc-file-mode`.
@ZmnSCPxj
Copy link
Contributor Author

A comment: JSON, technically speaking, disallows encoding octal in the JSON stream, and the only numbers that can start with 0 are 0 and floating-point numbers that have only 0 before the . sign. Of course, traditionally we express file modes in octal, because tradition. So in listconfigs, the rpc-file-mode setting is a string that is the octal representation of the mode.

@cdecker cdecker requested a review from rustyrussell January 24, 2020 12:04
@cdecker
Copy link
Member

cdecker commented Jan 24, 2020

The implementation looks good to me, however the reason I was reluctant to add a CLI flag was that it'd also get us to --rpc-file-owner and -rpc-file-group. Would it make sense to rather not recreate the socket file on each startup, keeping any manual changes in place? That'd be less unexpected than the file reverting any changes an operator might have done on the file, and free us from having to wire all possible file-related changes through lightningd options. It'd also match the systemd-way-of-doing-things, where a separate socket unit can pre-populate the socket file with the desired ACLs before starting the node.

Then again, I see the point of restoring a known good state on each startup, which is what I think @rustyrussell aimed to do by recreating every time. Just wanted to discuss this briefly before merging. What do you think @rustyrussell @ZmnSCPxj?

@ZmnSCPxj
Copy link
Contributor Author

I intend to set up my own rpc-file in a tmpfs, for the same reason: a clean startup. As well, it allows me to determine if lightningd is ready to listen, by checking if the file exists (since it is in a tmpfs, at system boot the file does not exist yet, then when lightningd is ready to listen, it exists tthen).

Ownership and groupship are generally handled by running daemons in their own unique UID and GID, e.g. Tor is typically run in tor:tor or debian-tor:debian-tor and if you want to let particular users access the Tor control port you do CookieAuthFileGroupReadable 1 and add the tor/debian-tor group to the user ancillary group list. That is how I intend to set up lightningd myself.

@cdecker
Copy link
Member

cdecker commented Jan 27, 2020

Ownership and groupship are generally handled by running daemons in their own unique UID and GID, e.g. Tor is typically run in tor:tor or debian-tor:debian-tor and if you want to let particular users access the Tor control port you do CookieAuthFileGroupReadable 1 and add the tor/debian-tor group to the user ancillary group list. That is how I intend to set up lightningd myself.

Sounds compelling. Would you suggest we change the file mask to allow groups then? It's something that several projects have bumped against, and I like your "add them to the lightningd group" proposal 👍

@cdecker
Copy link
Member

cdecker commented Jan 27, 2020

ACK 5c28d1f

@cdecker cdecker merged commit 7f4ed54 into ElementsProject:master Jan 27, 2020
@ZmnSCPxj
Copy link
Contributor Author

Would you suggest we change the file mask to allow groups then?

This command-line argument allows you to set this...? Just pass in 0660 instead of the default 0600. Or 0666 if you are insane and want to get your node funds stolen. In retrospect a --rpc-file-allow-group might have made better sense, since the only sensible settings are 0600 (no group) and 0660 (allow group).

@ZmnSCPxj ZmnSCPxj mentioned this pull request Jan 27, 2020
Closed
@ZmnSCPxj ZmnSCPxj deleted the rpc-mode branch January 28, 2020 00:46
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 3, 2020
@nixbitcoin
lightning-charge: Run under lightning-charge user
4bdeb60 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.
This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I haven't figured out a good way to migrate an existing
lightning-charge.db to the right ownership and permissions.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 3, 2020
@nixbitcoin
lightning-charge: Update pkg and run under lightning-charge user
6595149 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.
This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I haven't figured out a good way to migrate an existing
lightning-charge.db to the right ownership and permissions.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 3, 2020
@nixbitcoin
lightning-charge: Update pkg and run under lightning-charge user
645e75a 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.
This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I haven't figured out a good way to migrate an existing
lightning-charge.db to the right ownership and permissions.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 4, 2020
@nixbitcoin
lightning-charge: Update pkg and run under lightning-charge user
49e4451 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.

This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 4, 2020
@nixbitcoin
lightning-charge: Update pkg and run under lightning-charge user
ddecd41 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.

This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 6, 2020
@nixbitcoin
lightning-charge: Update pkg and run under lightning-charge user
58b075d 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.

This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 15, 2020
@nixbitcoin
lightning-charge: Update pkg and run under lightning-charge user
10147d2 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.

This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 17, 2020
@nixbitcoin
lightning-charge: Update pkg and run under lightning-charge user
ec747c7 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.

This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.
nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 18, 2020
@nixbitcoin
lightning-charge: Update pkg and run under lightning-charge user
f5f264f 
It has been possible to set JSON-RPC socket permissions since
ElementsProject/lightning#3437.

This commit makes the clightning JSON-RPC socket group readable and
writeable, runs lightning-charge under its own user with clightning
membership, and removes related warnings.

I thought about removing the lightning-cli option, but I think it's
actually not a good idea to run lightning-cli with another user anyway,
I ask for feedback on this.

clightning module: remove conditional in postStart

@jonasnick fixup

clightning module: g+X -> g+x in postStart

@jonasnick fixup

lightning-charge: remove unnecessary clightning-datadir option

lightning-charge module: move lightning-charge.db to own dataDir

@jonasnick fixup

lightning-charge module: Add todo

@jonasnick fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rustyrussell rustyrussell Awaiting requested review from rustyrussell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Umask for creating socket should be available as cmd param
2 participants
@ZmnSCPxj @cdecker