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

Improve setting permissions on /run/keys. #206

Merged
merged 9 commits into from
Jul 4, 2014
Merged

Improve setting permissions on /run/keys. #206

merged 9 commits into from
Jul 4, 2014

Conversation

aszlig
Copy link
Member

@aszlig aszlig commented Jun 24, 2014

We had NixOS/nixpkgs@4ab5646 so far which introduced the keys group in order to theoretically make it possible to allow access to other users than root, practically for NixOps we need to make sure we're allowing access to the group as well.

So, right now it's only setting the group ownership for /run/keys, but I'd go even further into changing deployment.keys.* into a nested attribute set, which would look like this:

deployment.keys.foobar = {
  text = "blah";
  user = "someuser";
  group = "somegroup";
};

Which then sets the appropriate permissions on the key itself. Unfortunately this will break backwards-compatibility, so I'm waiting for better ideas/suggestions before implementing it that way.

Latest build/evaluation at: https://headcounter.org/hydra/jobset/nixops/keys-permissions/latest-eval

@aszlig
Allow users in group "keys" to access /run/keys.
51cafce 
This makes it at least possible to access the keys directory if a
particular service is in the keys group, which has been introduced by
NixOS/nixpkgs@4ab5646.

However, to let specific users access a particular key, you still need
to work around it by adding an additional systemd service that sets the
right permissions. But at least with this we should have some
consistency with what is actually done in <nixpkgs>.

Signed-off-by: aszlig <[email protected]>
@rbvermaa
Copy link
Member

Allowing more control over the /run/keys using such a attribute set sounds nice indeed. Should be able to make it backwards compatible as well right?

aszlig added 6 commits June 24, 2014 13:23
@aszlig
Turn "keys" option into an attrset of attrsets.
3adc359 
This now provides options for each key and also converts old style
string-only keys into the new format while emitting a warning.

At the moment only the "text" option is actually supported.

For applying the keys for string-values the permissions attribute is set
to "0600" instead of the default value "0640" in keyOptionsType in order
to correctly replicate the old behaviour even when we implement
permissions and ownership.

Signed-off-by: aszlig <[email protected]>
@aszlig
MachineState: Implement permissions for keys.
1a3b226 
This however only implements setting permissions if "storeKeysOnMachine" is
set to false right now, because if the value is set to true the keys are
symlinked from the store and we actually have to find a way to control
permisions on it, which for the store is only possible if NixOS/nix#8 is
implemented.

Also, this ensures that the key filename is properly escaped.

Signed-off-by: aszlig <[email protected]>
@aszlig
nix/keys: Fix keyOptionsType submodule type.
ed2ba39 
First, forgot about specifying options.* and even if it would be there,
the option values wouldn't be merged because keyType was not properly
inheriting the merge function from keyOptionsType.

Signed-off-by: aszlig <[email protected]>
@aszlig
nix/keys: Use 0600 as the default permissions.
b4dfa05 
Only using this for legacy options could introduce unexpected behaviour
when switching to the new configuration if the user just appends a .text
to its configuration values and could possibly lead to services that
refuse to work, especially when it comes to OpenSSH.

Signed-off-by: aszlig <[email protected]>
@aszlig
nix/keys: Fix eval if storeKeysOnMachine is true.
3b16f5d 
This currently only fixes evaluation but actually doesn't apply the
correct permissions on the key files because they're just links to world
readable files in the Nix store.

Signed-off-by: aszlig <[email protected]>
@aszlig
nix/keys: Fix description of storeKeysOnMachine.
fb1307b 
As storeKeysOnMachine no longer is true by default (7ae4b27), we
shouldn't say so in the description. Especially because the default
value is already shown in the generated manual along with the
description.

Signed-off-by: aszlig <[email protected]>
@aszlig
Copy link
Member Author

aszlig commented Jun 24, 2014

@rbvermaa: Implemented so far, but if storeKeysOnMachine is enabled, permissions cannot be set. Should we issue a warning in that case, or do you think it would annoy users who deliberately want this behaviour. OTOH, permissions are world-readable anyway with that option enabled, so I guess we shouldn't need any warning as the behavior without this option enabled would restrict access on these files.

So if that's okay and there are no other issues, it's ready for merge :-)

@rbvermaa
Copy link
Member

I would add assert for it. I wouldn't know a use-case for setting permission that can't be set.

@aszlig
Copy link
Member Author

aszlig commented Jun 24, 2014

OTOH: Isn't storeKeysOnMachine kinda pointless the way it is right now? Because if someone wants to refer to a key (s)he can simply just add it with writeText anyway, without any need to get the long way around using /run/keys. So until we have NixOS/nix#8, I'd even suggest removing that option entirely, or am I forgetting about something?

@aszlig
Copy link
Member Author

aszlig commented Jun 24, 2014

Hm, another issue is that there is no keys group in 14.04, which causes the tests to fail. Going to fix that later.

@aszlig
Copy link
Member Author

aszlig commented Jun 27, 2014

Okay, I was wrong, there is a keys group, but for example the Hetzner backend test fails because the user name isn't yet available while bootstrapping in rescue.

@rbvermaa: My idea would be to use key.uid and key.gid instead of key.user and key.group, or do you have a better idea?

@aszlig
Copy link
Member Author

aszlig commented Jun 27, 2014

Okay, the user/group part is resolved by #207, because it really doesn't make sense to send keys to the rescue system, where we don't even have /run mounted.

aszlig added 2 commits July 4, 2014 11:16
@aszlig
nix/keys: Add a warning for storeKeysOnMachine.
f250e19 
This warning should be annoying enough so people will eventually set it
to false in case they ever used it. Especially if we want to set key
permissions, setting this option is pointless anyway because keys in
/run/keys are just symlinks to the world-readably store paths.

Signed-off-by: aszlig <[email protected]>
@aszlig
nix/keys: Correctly merge/convert deployment.keys.
8b3c27b 
So far we only used the apply function in order to convert keys as
string values into the new format. But if you have several keys defined,
mixing old and new format, the evaluation will fail because the apply
function is only evaluated once everything is merged.

So, we now use the same function we're using for apply to merge the
consecutive option values.

Signed-off-by: aszlig <[email protected]>
rbvermaa added a commit that referenced this pull request Jul 4, 2014
@rbvermaa
Merge pull request #206 from aszlig/keys-permissions
da140d8 
Improve setting permissions on /run/keys.
@rbvermaa rbvermaa merged commit da140d8 into NixOS:master Jul 4, 2014
@aszlig aszlig deleted the keys-permissions branch July 4, 2014 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aszlig @rbvermaa @thedebugger