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

Stop storing keys in the nix store for storeKeysOnMachine. #650

Closed
wants to merge 1 commit into from

Conversation

@kevincox
Copy link
Contributor

@kevincox kevincox commented Apr 16, 2017

This patch changes storeKeysOnMachine to work roughly the same way when
true as when false. The only difference is that the keys are stored in
/var/keys (which is usually a real disk) as opposed to /run/keys (which
is RAM).

This is an improvement on the previous version of storeKeysOnMachine
where the keys were stored in the Nix store and linked from /run/keys.
The previous version doesn't allow setting unix permissions on the keys,
meaning any process on the server can read all keys.

This solution has the downside that rolling back doesn't roll back the
keys, however this is consistent with how storeKeysOnMachine=false works
so shouldn't be a major concern. Furthermore if someone wants to have
the keys rollback with the system they can use builtins.toFile instead
of the keys mechanism which works the same way that the previous
storeKeysOnMachine=true worked.

Additionally a symlink /etc/nixos-keys has been added that will be set
to the appropriate key location to make it easier to switch between the
options.

@kevincox kevincox force-pushed the kevincox:keys-no-store branch 2 times, most recently from ae36e61 to 5de6739 Apr 16, 2017
@kevincox
Copy link
Contributor Author

@kevincox kevincox commented Apr 16, 2017

cc: @aszlig @ryanartecona @edolstra as you guys have touched this code last.

Copy link
Contributor

@ryanartecona ryanartecona left a comment

Nice! The existence of storeKeysOnMachine in its current form always made me nervous, and I never understood why it was an option anyway. This makes much more sense to me. I think this is roughly how I would've expected it to work when I first learned about it.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Apr 25, 2017

So really storeKeysOnMachine should be called persistKeysOnMachine?

@kevincox
Copy link
Contributor Author

@kevincox kevincox commented Apr 25, 2017

Yeah, it probably could have a better name. But that is orthogonal to this PR.

Copy link
Member

@aszlig aszlig left a comment

First of all, having /etc/nixops-keys sounds a bit dangerous to me, given that files in /etc are usually generated and might be overwritten if there is an environment.etc."nixops-keys" option definition. So I'd opt to use something in /var/lib instead.

But that aside, having different key directories based on whether storeKeysOnMachine is true or false is also not necessary here, because we can simply bind-mount the keys from let's say /var/lib/nixops-keys to /run/keys - even on a one by one basis.

@kevincox
Copy link
Contributor Author

@kevincox kevincox commented Apr 25, 2017

@aszlig
Copy link
Member

@aszlig aszlig commented Apr 25, 2017

@kevincox: Instead of bind mounts, I guess symlinks are okay as well, since that's what's already done in the existing implementation, although a bind mount would have the advantage that it will work even if programs don't properly dereference symlinks or doing permission checks on the symlink.

Having it in a different directory now means that existing deployments need to check storeKeysOnMachine in order to figure out the right directory, so even if we go that road it might be a good idea to make this available via a module option or even argument.

Another problem I see here (which is problematic for storeKeysOnMachine as well, but it's a ramfs so keys don't persist) is that old keys are not removed.

@kevincox kevincox force-pushed the kevincox:keys-no-store branch from 5de6739 to 27bbc5c Apr 27, 2017
@kevincox
Copy link
Contributor Author

@kevincox kevincox commented Apr 27, 2017

Having it in a different directory now means that existing deployments need to check storeKeysOnMachine in order to figure out the right directory, so even if we go that road it might be a good idea to make this available via a module option or even argument.
I didn't really want to worry about linking from a ramfs but since I'm chmoding them anyways I may as well.

Updated.

Another problem I see here (which is problematic for storeKeysOnMachine as well, but it's a ramfs so keys don't persist) is that old keys are not removed.

I didn't really want to remove the keys on deploy because the new keys are uploaded before the new config is activated. I could probably add it to the chmod activation script but since removing keys is pretty rare I propose pushing this to the next PR.

This patch changes storeKeysOnMachine to work roughly the same way when
true as when false. The only difference is that the keys are stored in
/var/keys (which is usually a real disk) as opposed to /run/keys (which
is RAM).

This is an improvement on the previous version of storeKeysOnMachine
where the keys were stored in the Nix store and linked from /run/keys.
The previous version doesn't allow setting unix permissions on the keys,
meaning any process on the server can read all keys.

This solution has the downside that rolling back doesn't roll back the
keys, however this is consistent with how storeKeysOnMachine=false works
so shouldn't be a major concern. Furthermore if someone wants to have
the keys rollback with the system they can use builtins.toFile instead
of the keys mechanism which works the same way that the previous
storeKeysOnMachine=true worked.

Additionally a symlink /etc/nixops-keys has been added that will be set
to the appropriate key location to make it easier to switch between the
options.
@kevincox kevincox force-pushed the kevincox:keys-no-store branch from 27bbc5c to 1d2530a Apr 27, 2017
@ip1981
Copy link
Contributor

@ip1981 ip1981 commented May 4, 2017

Guys, how about adding an option for keys path? Defaulting to /run/keys. So instead of guessing on storeKeysOnMachine, one can choose path where to put keys.

I used to have services for persisting keys: https://gist.github.com/ip1981/755f7bf71c157ebb1bd8f6d404cae5d3

And I'm really ready for configurable keys path: https://github.com/ip1981/nixsap/blob/49659d55878d09827566902d07d01f051926aff2/modules/deployment/keyrings.nix#L38 :)

@domenkozar
Copy link
Member

@domenkozar domenkozar commented May 10, 2017

We clearly need to document motivations for these options. We are mixing different needs here.

The way I see it:

  1. Storing keys in /nix/store/ is useful for rollbacks. Since that exposes the keys as world readable, this is not the default.

  2. Storing keys in /run/keys/* location could be configurable together with fixing #646

@domenkozar
Copy link
Member

@domenkozar domenkozar commented May 11, 2017

A different take on this: #664

@kevincox
Copy link
Contributor Author

@kevincox kevincox commented May 11, 2017

Guys, how about adding an option for keys path? Defaulting to /run/keys. So instead of guessing on storeKeysOnMachine, one can choose path where to put keys.

This is what the first version did basically. It was binary but changing the target would be trivial. It also added a /etc/nixops-keys symlink to abstract where it was stored from users.

Storing keys in /nix/store/ is useful for rollbacks. Since that exposes the keys as world readable, this is not the default.

If you want to do this simply use builtins.toFile. I don't see any advantage to using the keys system.

Storing keys in /run/keys/* location could be configurable together with fixing #646

I'm okay with that.

@ip1981
Copy link
Contributor

@ip1981 ip1981 commented May 11, 2017

Honestly, I think rollbacks are overestimated. First of all, it means discrepancy between the machine specification and actual state. Second, deploy with previous configuration is in fact a rollback. If you haven't done GC, it will be very fast. Quick rollback in case of f*ck up... should not happen :) Thanks to Nix, I can test configurations and migrations to death before pushing to production.

@ip1981
Copy link
Contributor

@ip1981 ip1981 commented May 11, 2017

I only see some value in "keys in the store" that if key content changes you will get activation/restart. There is a hopefully nice workaround: use new key name for new content, e .g. by appending a number to its name :)

@kevincox
Copy link
Contributor Author

@kevincox kevincox commented May 11, 2017

I think rollbacks aren't commonly useful to have for the key level. I would argue that they are very useful however when you need to get an ssh server running so that you can deploy the new version :) That being said I don't know how many people depend on keys for their ssh server and can't use a newer key.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented May 17, 2017

#664 is ready for review as an alternative implementation. It doesn't change behavior of storeKeysOnMachine = true, but it allows to specify destDir when storeKeysOnMachine = false.

@grahamc
Copy link
Member

@grahamc grahamc commented Mar 26, 2020

Hello!

Thank you for this PR.

In the past several months, some major changes have taken place in
NixOps:

  1. Backends have been removed, preferring a plugin-based architecture.
    Here are some of them:

  2. NixOps Core has been updated to be Python 3 only, and at the
    same time, MyPy type hints have been added and are now strictly
    required during CI.

This is all accumulating in to what I hope will be a NixOps 2.0
release
. There is a tracking issue for that:
#1242 . It is possible that
more core changes will be made to NixOps for this release, with a
focus on simplifying NixOps core and making it easier to use and work
on.

My hope is that by adding types and more thorough automated testing,
it will be easier for contributors to make improvements, and for
contributions like this one to merge in the future.

However, because of the major changes, it has become likely that this
PR cannot merge right now as it is. The backlog of now-unmergable PRs
makes it hard to see which ones are being kept up to date.

If you would like to see this merge, please bring it up to date with
master and reopen it
. If the or mypy type checking fails, please
correct any issues and then reopen it. I will be looking primarily at
open PRs whose tests are all green.

Thank you again for the work you've done here, I am sorry to be
closing it now.

Graham

@grahamc grahamc closed this Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.