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.
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
[RFC 0143] Nix Store ACLs #143
base: master
Are you sure you want to change the base?
[RFC 0143] Nix Store ACLs #143
Changes from 2 commits
1d0b015
a3e6322
519ca11
bc29a46
bd3f6a6
830cdd3
676f1c9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to care about
nix-serve
for this (unless it's already implemented?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence seems to imply that it's actually not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not matter too much, but shouldn't anyone with access to the path be allowed to share it further?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(it looks like that's what L53 implies too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear at which level these options acts: the first paragraph seems to talk about a specific path (with one access-control list), while the second one seems to be global to the store
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand it correctly:
acl=true
(as a global store option) means that every path MUST have an attached ACL (which is enforced)selective-acl=true
means that paths CAN have an attached ACL, and will be world-readable otherwiseIs that it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably irrelevant here, but this is somewhat reminescent of the mechanism for allowing access to paths in pure eval mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean that it's possible for someone to register a derivation without having access to its closure? How would that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that would be theoretically possible; after all, derivations (
.drv
files) are pretty much just regular files added to the store, and one can add such files without having access to the closure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole section is a bit weird since the
LocalStore
doesn't have any notion of user (nortrusted-users
).Maybe this could be reorganized as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just leave that out. It's pure implementation detail and doesn't matter too much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are POSIX ACLs also available on OSX or is it using a different mechanism there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are available but the API differs a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that attribute have an impact on the derivation hash? It feels like it shouldn't (as it only touches the metadata), but at the same time not doing so wouldn't be properly backwards-compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be worth mentioning (maybe in the “alternatives” section) that changing the “derivation” builtin isn't required as it's possible to just give access to the builder by default. (btw, the first part seems to assume that it's the case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: Should that work recursively? If I'm Alice and have
should
bob
be given access to"${./somePath}"
since it's in the closure of that derivation output?And what if I don't add myself to the
__permissions
field?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably shouldn't. The lack of backwards-compatibility is annoying; I think we should make
__permissions
format non-toString
-able (e.g. by making it an attrset). It would result in a clear failure on older Nix, which is a fine behaviour IMHO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so; IMHO, Permissions are applied only to the object on which they are specified.
Perhaps there should be a check to automatically add the evaluating user to the __permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC attic has a similar problem, and some solution to it. It would be nice to see how they handle it (maybe Cachix has it too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re. my comment about whether
__permissions
should affect the hash: This means that it doesn't. If you go that way it should be spelled out explicitlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be a bit stronger here (or somewhere else), and highlight that this isn't a good mechanism for storing secrets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it depends on what is meant by "secrets"; after all, the only reason to have ACLs in the first place is to have some information which is only accessible to a select group, and thus "secret" in some sense. Of course, it should be highlighted that this is intended for "secret" (proprietary or otherwise protected) source-code and build artifacts, rather than runtime secrets such as credentials.