Hardening DAC #171

nixbitcoin · 2020-05-04T10:22:00Z

This PR all-around hardens our existing discretionary access control.

All lightning modules (spark-wallet, recurring-donations, lightning-charge) now run under their own user with clightning group membership. They can interact with c-lightning via group-readable/writable socket. I didn't remove the lightning-cli option because I think it's a good idea to always run it under the clightning user (otherwise it's executed with root permissions in nodeinfo), please take a look at this.

bitcoinrpc group is removed and bitcoin group membership is reduced to a minimum. Secrets are gotten via preStart with PermissionsStartOnly.

electrs also has an improved and truly optional TLSProxy.

I wasn't able to reduce the permissions of nodeinfo, onion-chef and create-webindex from root. I couldn't find an elegant way to fulfill their functions as a less-privileged user without introducing excessive complexity.

LND was out-of-scope for this PR, because I don't run it and have no experience with it.

Closes #145
Closes #170

jonasnick

nice cleanup 👍

modules/electrs.nix

jonasnick · 2020-05-10T13:36:40Z

modules/clightning.nix

@@ -111,6 +110,10 @@ in {
        while [[ ! -e ${cfg.dataDir}/bitcoin/lightning-rpc ]]; do
            sleep 0.1
        done
+        # give group "search" access to allow clightning group processes to find lightning-rpc


Do you know who needs that? lightning-rpc is always lightning-rpc - not sure why anyone would need search permissions

spark-wallet and lightning-charge need to be able to "look" into the directory to find lightning-rpc. Otherwise you get

Lightning client connection error { Error: connect EACCES /var/lib/clightning/bitcoin

in spark-wallet for example.

modules/clightning.nix

modules/lightning-charge.nix

jonasnick · 2020-05-10T20:04:13Z

modules/presets/secure-node.nix

@@ -132,8 +132,6 @@ in {
    services.onion-chef.enable = true;
    services.onion-chef.access.operator = [ "bitcoind" "clightning" "nginx" "liquidd" "spark-wallet" "electrs" "sshd" ];

-    # Unfortunately c-lightning doesn't allow setting the permissions of the rpc socket
-    # https://github.com/ElementsProject/lightning/issues/1366
    security.sudo.configFile =
     (optionalString cfg.clightning.enable ''


Yeah I'm fine with keeping the lightning-cli option with sudo. It's simpler than having to deal groups.

@erikarvstedt Re: 0e66ec47489732002ba9ab96dafd3ddd401a5ae8 I thought it would be good to keep, so lightning-cli would never be executed as root by accident. But I guess the simplicity win outweighs that. So ACK.

modules/clightning.nix

modules/nodeinfo.nix

pkgs/default.nix

modules/lightning-charge.nix

jonasnick · 2020-05-17T16:13:10Z

ACK want to squash?

nixbitcoin · 2020-05-17T16:14:36Z

Should I squash all into one or just the jonasnick fixups?

jonasnick · 2020-05-17T16:17:04Z

just fixups

nixbitcoin · 2020-05-17T16:30:34Z

squashed

jonasnick · 2020-05-17T18:47:55Z

Oh I meant squashing the fixups into the corresponding original commits.

nixbitcoin · 2020-05-18T07:55:58Z

Done. Didn't squash clightning module: remove config group read access because it is a separate hardening, but I moved it to a more appropriate place.

erikarvstedt · 2020-05-18T09:56:23Z

I'll review this later today, please wait before merging.

erikarvstedt · 2020-05-18T12:52:01Z

Thanks, @nixbitcoin, for these very sensible changes.

Could you, in the future, add a fixup! prefix to titles of commits that are meant to be squashed?

Also, please use separate commits for larger, orthogonal changes.
For example lightning-charge: Update pkg and run under lightning-charge user consists of these single-topic commits:

lightning-charge: 0.4.14 -> 0.4.19
lightning-charge: add dedicated user
clightning: allow group access to RPC socket

Here are my fixups.

nixbitcoin · 2020-05-18T15:50:30Z

Thank you for the great fixup !'s 😄 I squashed them (except 679b95f2c2abe6560c2c7d4cf9688244e499cf2a) into the appropriate commits.

This was my first try at structuring commits properly. My next PR will be more diligent in separating into single-topic commits and obeying the nix naming conventions.

I separated said commit. I didn't separate 95afba4 because the changes aren't that orthogonal and splitting wasn't that easy.

erikarvstedt · 2020-05-18T20:48:03Z

Thanks! 😺
These fixups remain.

Could you please remove the ephemeral fixup messages from the squashed messages?
The squashed messages should only contain information relevant to the committed content. Historical discussion about ultimately rejected and uncommited changes should not be part of the commit message, unless it provides an insightful rationale for the actual changes.

Example

Problematic message:

nodeinfo: Convert to module and allow alternative operator username

operatorname: make into option in secure-node.nix

@jonasnick fixup

nodeinfo module: fix whitespace & remove unused

@jonasnick fixup

fixup! rename services.operatorname -> nix-bitcoin.operatorName

operatorName is not a service

fixup! improve nodeinfo

rename services.nodeinfo -> program.nodeinfo, following NixOS naming
conventions. (nodeinfo is not a service.)

currently, nodeinfo has presets/secure-node.nix as a strict
dependency as it requires onion-chef and the 'operatorName' option.
and nix-bitcoin-webindex.nix has nodeinfo as a dependecy.

so don't add nodeinfo and webindex to modules.nix because they will fail on standalone use.

Fixed message:

nodeinfo: Convert to module and allow custom operator username

Currently, nodeinfo has presets/secure-node.nix as a strict
dependency as it requires onion-chef and the 'operatorName' option.
Also, nix-bitcoin-webindex.nix has nodeinfo as a dependecy.

So don't add nodeinfo to modules.nix (and remove webindex)
because they both fail on standalone use.

erikarvstedt · 2020-05-18T21:08:55Z

Please rename commit clightning module: remove ... -> clightning: remove ...

If TLSProxy is disabled, bypass nginx by forwarding Tor HS traffic directly to electrs.

Electrs does not need to be a part of "bitcoinrpc" group because preStart electrs.toml creation is handled by PermissionsStartOnly. "bitcoin" group membership is only necessary when cfg.high-memory is enabled and electrs reads blocks directly from the blocks directory.

Secrets are written to clightning config file during preStart with root permissions because of PermissionsStartOnly.

currently, nodeinfo has presets/secure-node.nix as a strict dependency as it requires onion-chef and the 'operatorName' option. and nix-bitcoin-webindex.nix has nodeinfo as a dependecy. so don't add nodeinfo and webindex to modules.nix because they will fail on standalone use.

jonasnick

ACK b8e10af

nixbitcoin · 2020-05-19T12:13:36Z

@erikarvstedt

Could you format your approvals in the future like so: ACK <top commit hash>? That way it shows up in @jonasnick's merge commits.

erikarvstedt · 2020-05-19T12:34:46Z

@nixbitcoin ACK 😃

nixbitcoin force-pushed the hardening-DAC branch from 98869f4 to 89bec3f Compare May 4, 2020 15:47

nixbitcoin mentioned this pull request May 6, 2020

Hardening systemd #174

Merged

nixbitcoin force-pushed the hardening-DAC branch from 89bec3f to 7c1dc84 Compare May 6, 2020 15:54

nixbitcoin force-pushed the hardening-DAC branch from 7c1dc84 to c904b7f Compare May 10, 2020 14:47

jonasnick reviewed May 10, 2020

View reviewed changes

nixbitcoin force-pushed the hardening-DAC branch from a0e579d to 3ee6264 Compare May 15, 2020 08:21

jonasnick closed this May 15, 2020

jonasnick reopened this May 15, 2020

jonasnick reviewed May 16, 2020

View reviewed changes

modules/lightning-charge.nix Show resolved Hide resolved

jonasnick reviewed May 16, 2020

View reviewed changes

modules/lightning-charge.nix Show resolved Hide resolved

nixbitcoin force-pushed the hardening-DAC branch from 3ee6264 to 1165fc6 Compare May 17, 2020 16:06

nixbitcoin force-pushed the hardening-DAC branch from 1165fc6 to f1b81eb Compare May 17, 2020 16:28

nixbitcoin force-pushed the hardening-DAC branch from f1b81eb to 6bada9d Compare May 18, 2020 07:54

nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 18, 2020

Adjust joinmarket module to hardenings in fort-nix#171 and fort-nix#174

f71a8c5

nixbitcoin force-pushed the hardening-DAC branch from 6bada9d to baaf854 Compare May 18, 2020 15:46

nixbitcoin added 2 commits May 19, 2020 11:08

electrs: Make TLSProxy truly optional

4c139a6

If TLSProxy is disabled, bypass nginx by forwarding Tor HS traffic directly to electrs.

electrs: Drop insecure TLS ciphers

7cfae66

nixbitcoin added 11 commits May 19, 2020 11:08

clightning: Remove clightning "bitcoinrpc" membership

04c6936

Secrets are written to clightning config file during preStart with root permissions because of PermissionsStartOnly.

clightning: remove config group read access

304dd29

clightning: allow group access to RPC socket

0ba5575

lightning-charge: 0.4.14 -> 0.4.19

e67a818

lightning-charge: add dedicated user

81a04a4

bitcoind: only make blocksdir group-readable when dataDirReadableByGroup

205fca3

spark-wallet: Run under spark-wallet user

563b210

Remove bitcoinrpc group remnants

95d230d

recurring-donations: Run under recurring-donations user

b8e10af

nixbitcoin force-pushed the hardening-DAC branch from baaf854 to b8e10af Compare May 19, 2020 11:30

jonasnick approved these changes May 19, 2020

View reviewed changes

erikarvstedt approved these changes May 19, 2020

View reviewed changes

jonasnick merged commit 0ac1e49 into fort-nix:master May 19, 2020

nixbitcoin added a commit to nixbitcoin/nix-bitcoin that referenced this pull request May 26, 2020

Adjust joinmarket module to hardenings in fort-nix#171 and fort-nix#174

d2ab71f

nixbitcoin deleted the hardening-DAC branch March 3, 2021 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardening DAC #171

Hardening DAC #171

nixbitcoin commented May 4, 2020

jonasnick left a comment

jonasnick May 10, 2020

nixbitcoin May 11, 2020

jonasnick May 10, 2020

nixbitcoin May 18, 2020

jonasnick commented May 17, 2020

nixbitcoin commented May 17, 2020

jonasnick commented May 17, 2020

nixbitcoin commented May 17, 2020

jonasnick commented May 17, 2020

nixbitcoin commented May 18, 2020

erikarvstedt commented May 18, 2020

erikarvstedt commented May 18, 2020

nixbitcoin commented May 18, 2020 •

edited

Loading

erikarvstedt commented May 18, 2020

erikarvstedt commented May 18, 2020 •

edited

Loading

jonasnick left a comment

nixbitcoin commented May 19, 2020 •

edited

Loading

erikarvstedt commented May 19, 2020

Hardening DAC #171

Hardening DAC #171

Conversation

nixbitcoin commented May 4, 2020

jonasnick left a comment

Choose a reason for hiding this comment

jonasnick May 10, 2020

Choose a reason for hiding this comment

nixbitcoin May 11, 2020

Choose a reason for hiding this comment

jonasnick May 10, 2020

Choose a reason for hiding this comment

nixbitcoin May 18, 2020

Choose a reason for hiding this comment

jonasnick commented May 17, 2020

nixbitcoin commented May 17, 2020

jonasnick commented May 17, 2020

nixbitcoin commented May 17, 2020

jonasnick commented May 17, 2020

nixbitcoin commented May 18, 2020

erikarvstedt commented May 18, 2020

erikarvstedt commented May 18, 2020

nixbitcoin commented May 18, 2020 • edited Loading

erikarvstedt commented May 18, 2020

Example

erikarvstedt commented May 18, 2020 • edited Loading

jonasnick left a comment

Choose a reason for hiding this comment

nixbitcoin commented May 19, 2020 • edited Loading

erikarvstedt commented May 19, 2020

nixbitcoin commented May 18, 2020 •

edited

Loading

erikarvstedt commented May 18, 2020 •

edited

Loading

nixbitcoin commented May 19, 2020 •

edited

Loading