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.

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

Enable secret content in the nix-store. #329

Closed
wants to merge 20 commits into from
Closed

Enable secret content in the nix-store. #329

wants to merge 20 commits into from

Conversation

nbp
Copy link
Member

@nbp nbp commented Aug 25, 2014

This set of patches add a SecretMode class. This class is used to prevent doing permissive chmod on the content of the nix-store. The instances of this class are initialized with a few functions which are used to collect the name of the user which will own the file.

In order to test this features, these patches are adding builtins.toSecretFile primitive, and a special attribute named "secret" to the attribute set argument of derivations. The "secret" attribute expect a boolean value. Both the "secret" attribute and the "toSecretFile" primitives make the derivation/output only readable by the owner of the nix-store.

@shlevy
Copy link
Member

shlevy commented Aug 26, 2014

See also #8

@aszlig
Copy link
Member

aszlig commented Aug 26, 2014

The main problem I see here is that any other user which knows the store path is able to use the Nix daemon to read the contents of the file.

So I guess in order to prevent that entirely on the permission level, the daemon needs to authenticate the socket client and request those store paths from the client. Or, as mentioned by people in #8, if the exact name/path of the secret store path is only known by the "right" user.

But would have an even larger attack vector, especially on daemons running without chroot and/or derivations using __noChroot (you can simply ls -R the whole store to find out the secret path).

@nbp
Copy link
Member Author

nbp commented Aug 26, 2014

The main problem I see here is that any other user which knows the store path is able to use the Nix daemon to read the contents of the file.

Are you referring to the exportPath() function? As I am not familiar much with the code base, I am mostly doing test-driven development.

So I guess in order to prevent that entirely on the permission level, the daemon needs to authenticate the socket client and request those store paths from the client.

Asking Eelco, unix sockets are providing a way to authenticate the user which establishes a connection, so I guess I could use this to check if the given user has right to create/access these files.

@aszlig
Copy link
Member

aszlig commented Aug 26, 2014

Asking Eelco, unix sockets are providing a way to authenticate the user which establishes a connection, so I guess I could this to check if the given user has right to create/access these files.

Yeah, that's something which is also used by PostgreSQL with the peer authentication method. So essentially if you have a .drv which refers to secret paths, the Nix-daemon will refuse access, that's good so far. I'd also make sure that if a drv refers to a secret path, the result should be secret as well.

For example: public -> secret -> secret -> ...
But not: public -> secret -> public -> ...

Because the latter would mean, that through the references on the last derivation, the secret path could be exposed to the builder.

Also, speaking of the builder, we still have the problem that with non-chroot builds (which is also the case for __noChroot and fixed output derivations), the secrets can be accessed.

So for example consider this:

(import <nixpkgs> {}).stdenv.mkDerivation {
  name = "foo";
  buildCommand = ''
    id
    ls -l /nix/store/ | head
  '';
  outputHashAlgo = "sha256";
  outputHash = "0000000000000000000000000000000000000000000000000000";
}

The build will fail obviously, but you're free to access any secret store path and print its contents to stdout/stderr.

@nbp
Copy link
Member Author

nbp commented Aug 26, 2014

For example: public -> secret -> secret -> ...
But not: public -> secret -> public -> ...

Because the latter would mean, that through the references on the last derivation, the secret path could be exposed to the builder.

I don't see any issue with having public derivation / output depending on secret output. Of-course you the access of the secret output should be restricted, thus restricting the functions such as exporting closures.

But I do not understand why a user should not be able to read the configuration files which are not containing secrets. One question I have is, how would you handle a /etc/profile which starts a session daemon which has a config file specified on the command line? If this configuration is secret, I would not expect the /etc/profile script to be secret:

  if test $USER = "root"; then
    myDaemon -c /nix/store-…-conf
  fi

we still have the problem that with non-chroot builds

Then we should probably enforce chroot builds if the store contains any secret.
Also I am not worried about being able to list files of the store, but more reading the content of the files.

Aren't we using non-root build users for making the builds?

@aszlig
Copy link
Member

aszlig commented Aug 26, 2014

But I do not understand why a user should not be able to read the configuration files which are not containing secrets. One question I have is, how would you handle a /etc/profile which starts a session daemon which has a config file specified on the command line? If this configuration is secret, I would not expect the /etc/profile script to be secret:

Okay, you're right, that makes no sense.

However, going from secret -> public introduces the need for additional checks (which are likely to be forgotten), like for what you mentioned regarding export of closures.

Also I am not worried about being able to list files of the store, but more reading the content of the files.

Listing the files was just an example, you can actually access them as well, which is my main point here, just replace ls -l /nix/store/ | head with cat /nix/store/super-secret-path and you should get the contents.

Enabling chroot for the builds doesn't set up chroots for fixed output derivations, which are for example things like fetchurl. Plus as mentioned, you can still use __noChroot = true for any derivation to prevent chrooting.

Getting rid of __noChroot is however not fixing anything, because the fixed output derivations remain without chroot.

@aszlig
Copy link
Member

aszlig commented Aug 29, 2014

A possible way that comes in mind to fix the non-chroot issue would be to use separate build users that only have access to the input store paths. Not sure however how to solve this, except with something like seccomp BPF, selinux or grsecurity.

@nbp
Copy link
Member Author

nbp commented Sep 28, 2014

Update: The current status of the branch is almost finished. NAR files are handled and tested. The last remaining part is on how to handle build substitutes. The problem of build substitute is that they do not always(*) have a corresponding derivation and the derivation is used to find what are the access rights of the output. I am thinking of taking the access right from the derivation if it exists, and otherwise default to a public visibility.

(*) I failed to understand why this was not always the case, as somebody which is doing a build should have a derivation which correspond to the path. I guess this might be for derivations which are including the outPath without context or for made-up user environment in which we write the outPath on the command line.

@edolstra , should we consider sharing secrets through the substitute? Is it fine to ignore it if we have no derivation associated?

@wmertens
Copy link
Contributor

Question: Why is secret content stored in the store at all? Why not only
store a reference to the data somewhere else in the filesystem, protected
by the proper Unix file permissions?

On Sun, Sep 28, 2014 at 7:50 PM, Nicolas B. Pierron <
[email protected]> wrote:

Update: The current status of the branch is almost finished. NAR files are
handled and tested. The last remaining part is on how to handle build
substitutes. The problem of build substitute is that they do not always(*)
have a corresponding derivation and the derivation is used to find what are
the access rights of the output. I am thinking of taking the access right
from the derivation if it exists, and otherwise default to a public
visibility.

  • I failed to understand why this was not always the case, as somebody
    which is doing a build should have a derivation which correspond to the
    path. I guess this might be for derivations which are including the outPath
    without context or for made-up user environment in which we write the
    outPath on the command line.

@Niksnut, should we consider sharing secrets through the substitute? Is it
fine to ignore it if we have no derivation associated?


Reply to this email directly or view it on GitHub
#329 (comment).

@nbp
Copy link
Member Author

nbp commented Sep 29, 2014

Question: Why is secret content stored in the store at all? Why not only store a reference to the data somewhere else in the filesystem, protected by the proper Unix file permissions?

1/ I want to stop warning users that putting password in configuration ...
a/ Is a security issue.
b/ Is different than the way they expect it to be. (not intuitive)
2/ We want it handle by Nix because
a/ nix-store locations can easily be distributed.
b/ impurity are not desirable.
3/ I want it in the Nix language, such as we can warn about world readable configuration files which are containing passwords.

The way we handle password so far is unsafe, weird and complex, and we should just do it properly as any other options.

@wmertens
Copy link
Contributor

Well I agree that it's a security issue, I just think passwords shouldn't (EDIT: was should) be
in the store at all.

For example, if you're configuring a service which has an admin password
(wether plaintext or crypted), right now NixOS will build the configuration
file and store it world-readable in /nix/store.

Your proposal, as I understand it, will store the the file in the nix
store, with special permissions and file ownership. This makes the simple
store model (all files are world-readable) complex and means that nix
stores are no longer NFS mountable if the userids don't match, for example.

What I propose is to put the files with sensitive content in
/nix/var/secret/$somehash-name owned by the appropriate user, still
read-only of course. The nix store then contains symlinks to there, and its
simple model is retained. The only added complexity is contained under
/nix/var/secret, and those files do not get copied along into NAR archives.
Copying the sensitive files to other systems has to be out-of-band, or by
recreating based on NixOS configuration.

On Tue, Sep 30, 2014 at 1:16 AM, Nicolas B. Pierron <
[email protected]> wrote:

Question: Why is secret content stored in the store at all? Why not only
store a reference to the data somewhere else in the filesystem, protected
by the proper Unix file permissions?

1/ I want to stop warning users that putting password in configuration ...
a/ Is a security issue.
b/ Is different than the way they expect it to be. (not intuitive)
2/ We want it handle by Nix because
a/ nix-store locations can easily be distributed.
b/ impurity are not desirable.
3/ I want it in the Nix language, such as we can warn about world readable
configuration files which are containing passwords.

The way we handle password so far is unsafe, weird and complex, and we
should just do it properly as any other options.


Reply to this email directly or view it on GitHub
#329 (comment).

@nbp nbp changed the title WIP: Enable secret content in the nix-store. Enable secret content in the nix-store. Oct 9, 2014
@nbp
Copy link
Member Author

nbp commented Oct 9, 2014

@edolstra, The feature should be complete from the nix-store perspective. Feel free to review the code and leave comments.

The basic idea is that the ownership is either provided as argument when making a derivation, or inferred from the store paths. The SecretMode structure is annotating the canonicalise function, such as we can maintain the security aspect of the store path. One store path is either fully readable or fully secret, we cannot have one secret file within a publicly readable store path. NAR files are encoding the ownership of the store path. The builders and the substituters should maintain the expected ownership to prevent leaking secrets.

Currently, we only have public / secret modes. The current model is made such as it can be made future proof, by using ACL stored in a string. The public mode is using the mask 777, and the secret mode is using the mask 700. This means that the file is only readable by the owner of the Nix store (and root).

I still have to add documentation and add more tests to make sure this implementation is robust enough.

@nbp
Copy link
Member Author

nbp commented Oct 10, 2014

@wmertens Can you explain me how having special directory in /nix/var/secret owned by an appropriate user versus having directories in /nix/store owned by the appropriate users will change anything to a supposed NFS issue?

So far, I do not intent to support having secret for any user except the owner of the nix store. On most system, root share the same uid, so I do not see any issue with NFS here.

Copying files from one computer to another is at your own risk, if you are unable to make wise security choices for the transport, Nix cannot help you with that. But Nix should not prevent you from doing so either, as this is a useful use case for deploying configurations.

@wmertens
Copy link
Contributor

Ok, I thought that you were handling multiple users here. Over NFS the mapping is by uid so if the uids don't match, the files aren't owned by the same user.

Anyway I still think that by adding this you add a lot of complexity to the nix-store code, whereas there are only a few files that have to be private and they could be handled on a case-by-case basis (as now) or by referring to them at a different location outside the store.

I really love the fact that the whole store is world-readable and shareable and your proposal changes that.

You obviously put a lot of work in this and I respect and admire that, but I don't like this change.

@nbp
Copy link
Member Author

nbp commented Oct 12, 2014

Using a different location for storing files implies that this would be an impurity to the /nix/store directory. I guess we could image a per-user store, but then we would have additional issues for maintaining invariants such as completeness and correctness, as the nix store is scanning for paths by their hashes which implies that if we are looking for a realization then we would have to look at the /nix/store and the per-user store too.

Then if you have one user installing another user software from a path within the nix store, on the same computer, then we should make a copy of the realization of the other user content? Or let him try to access other users content?

This sounds way harder to properly handle a separate /nix/store than adding access policies to the /nix/store. And as I learned from practice, a good security design is defined by its surface of attack (aka. code-size). To be honest, I thought about removing toSecretFile prim-op, but the backend is also used for writing derivation files into the /nix/store, and this is a convenient function to have for passwords.

@wmertens
Copy link
Contributor

Good points!

I was thinking that storing the files in a different location would be done with local builds, and transporting them to other locations is either out of scope or can be done with a small, separate script. For nix-store, the secret files are symlinks to locations outside the store, and it doesn't care really about their targets after their initial creation.

The secret files could be stored with their content hash in the filename, so they are self-verifying as well as verified from the store symlink.

To tackle the other-user-with-exact-same-configuration problem, the secret file's content hash should also include the permissions, which would lead to different hashes and would also enable verification of the permissions.

So at a first glance (you know the code better than me) this approach would mean some code to generate the secret files and a script for transferring the files. I think that should yield less changes than your current approach.

@nbp
Copy link
Member Author

nbp commented Dec 21, 2014

@edolstra when would you be able to provide feedback on this implementation? What do you need more before considering this pull request?

We need to solve this issue urgently! This is a Nix issue, which hit both NixOS and NixOps, we can do as much work as we want on NixOps to work-around #8, this will never solve the underlying problem!

@offlinehacker
Copy link
Contributor

What is the state with this pull request?

My issue is a bit different, i want to share store with multiple (docker) containers, and i would like that each of the containers would be allowed to access only files that it's supposed to access(so only files in its closure). I was thinking about apparmor profiles, but i don't know what the performance impact would be?

@wmertens
Copy link
Contributor

@offlinehacker you could use a union mount to overwrite all paths that the Docker containers aren't allowed to read?

@wmertens
Copy link
Contributor

wmertens commented Jan 5, 2015

See also #8 which has a patch that does what I describe in #329 (comment)

@copumpkin
Copy link
Member

How's this going?

@nbp
Copy link
Member Author

nbp commented Nov 20, 2015

This was waiting on @edolstra review, but I am no longer sure I should expect any reviews now.

I can only guess that this solution is not perfect enough to be commented on and that we prefer to have no solution than a root-only solution.

I am probably not going to work on this pull-request again, unless I got more feedback. This is the second pull request I do on the Nix project and I think this project lacks responsive maintainers.

If I keep having no response, I guess I should fork the Nix project and start maintaining my own fork of it.

@copumpkin
Copy link
Member

I think it might make sense to spread out the responsibility a bit on this repo (and other non-nixpkgs repos). I get that not many people outside of @edolstra are "experts" on this codebase, but what's the worst that can happen if we add a handful more core Nix people to the repo? If nothing else, it'll spread the sense of ownership over the project and encourage people to give more feedback and learn more about it.

@stale
Copy link

stale bot commented Feb 13, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Feb 13, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

I closed this issue due to inactivity. → More info

@stale stale bot closed this Apr 16, 2022
@gador
Copy link
Member

gador commented Oct 22, 2022

This is kinda sad that all that work that went into this PR didn't even get a comment from @edolstra. Maybe the new "Teams" will help with the large amount of PR's and issues so effort like this isn't wasted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants