nixos/etesync-dav: init module

[matt-snider]

Motivation for this change

etesync-dav was recently packaged in #76886 and since then I've been using this module locally. I think it could be useful to others.

One thing to consider is that the current implementation requires that the user go to the management panel once the service is running (default http://localhost:37358), use their etesync credentials to login, and then copy the generated password for use in other programs that want to use etesync-dav to bridge the connection to etesync. It might be possible to have the credentials generated on startup from some additional module options (e.g. etesyncUsername, etesyncPasswordCommand) similarly to here, but I'm not sure this would work nicely, and the generated password still needs to be passed back to the user securely.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[aanderse]

What a great module 👍 Very clean and simple! A few points:

• regarding the steps required I think that sounds fine, but is there a need to document/describe to the user what to do, or is that obvious for users of this software?
• I'm not familiar with this software but it sounds like something that should be behind ssl if exposed outside of localhost. Is this correct?

[matt-snider]

Hi Aaron, thanks for the review!

regarding the steps required I think that sounds fine, but is there a need to document/describe to the user what to do, or is that obvious for users of this software?

I think that should be obvious from the README of the software itself. I'm still new to the Nix Community though so correct me if I'm wrong.

I'm not familiar with this software but it sounds like something that should be behind ssl if exposed outside of localhost. Is this correct?

As noted above, I'll add support for this via SSL_CERT_FILE and SSL_KEY_FILE

[matt-snider]

@aanderse I updated the PR. The SSL cert handling was modeled after the syncthing module.

Let me know what you think!

[aanderse]

@matt-snider is there an opportunity to write a test for this module? That would really make this PR a home run ⚾

[matt-snider]

@aanderse Great idea on the test! I found a few issues as a result:

• I forgot to add it to the module-list
• In my manual tests the certs happened to be named properly (etesync.key, etesync.crt). The install command should explicitly set the proper names.

Anyways, the test cases are quite simple and basically just check that the login screen is reachable. I don't know if there is much else that can be tested.

Note: I commented out a test that checks for the redirect from http -> https. I noticed this doesn't currently work. There is however already an unreleased fix, so it should work in the next release. @Valodim: Just an FYI about the test since you are the maintainer of the etesync-dav package. It can likely be added in the next release (> 0.16.0)

[matt-snider]

Hey @aanderse can you take a look again when you have some time? EDIT: used the "Re-request review" action - didn't know about that feature :-)

[aanderse]

Sorry @matt-snider this dropped off my radar.
@GrahamcOfBorg test etesync-dav

[matt-snider]

@aanderse No problem.

Hmm tests were passing before, but it looks like etesync-dav was upgraded to 0.16.0 in the meantime (see #85061). I'll have a look and see if I can also uncomment the other test for the https redirect.

EDIT: woops I had thought the https redirect would be in 0.16.0 but as I wrote about it'll be in the next release

[matt-snider]

@aanderse I rebased and fixed the test.

The problem was that I referenced the etesync-dav user and group to copy the keys in ExecStartPre and on the very first run, the user & group weren't created yet (I guess due to DynamicUser=true). I'm not sure why it was working before, but it might have been due to running with -A driver and accidentally reusing existing VM state.

Anyways, I realized I don't need to do this step as root (via ExecStartPre=+...) or use install. Since the script runs as the correct user anyways cp looks like it works just fine.

[aanderse]

@GrahamcOfBorg test etesync-dav
@Valodim are you interested in leaving a review?

[Valodim]

Oh wow, this one kind of slipped past me! At a first glance this looks great! I'll try to give it a full review soon.

Meanwhile I updated etesync-dav #93556, which broke somewhere along the way. Quick review on that would be welcome to bring this back on track.

[matt-snider]

@Valodim A review would be great - thanks! Since you've updated to 0.20, the other test for the http to https redirect should also work now. I'lll try it out soon

[Valodim]

So I started reviewing this, and as noted before things seem mostly straightforward :)

However, I wondered one thing: Is setup for non-local access really a use case we want to support in this module? etesync-dav is intended to be run as a local proxy, so that the dav client becomes the actual cryptographic endpoint, to make things "end to end".
There are a bunch of options in this module (openFirewall, sslCertificate, sslCertificateKey, host) that don't fit this idea. Do either of you use etesync-dav in this way, personally?

As a concrete proposal for discussion: We could drop those options listed above in favor of a generic settings one, which is passed straight into the environment. That gives flexibility to folks with any use case, but also avoids having more than half the options of this module in the "not really the intended way of using it" bucket.

For comparison, I pushed the etesync-dav module I use in home-manager for the same purpose: nix-community/home-manager#1398

[aanderse]

ping @matt-snider

[matt-snider]

@Valodim Thanks for having a look and making some suggestions!

However, I wondered one thing: Is setup for non-local access really a use case we want to support in this module? etesync-dav is intended to be run as a local proxy, so that the dav client becomes the actual cryptographic endpoint, to make things "end to end".
There are a bunch of options in this module (openFirewall, sslCertificate, sslCertificateKey, host) that don't fit this idea. Do either of you use etesync-dav in this way, personally?

That's a good point - it's something I hadn't considered. I certainly don't use etesync-dav this way, but I'm not sure we should lump all of those options into the "non-local access" bucket. SSL is properly supported by etesync-dav and it seems like it might make sense for us to support it too. In the windows instructions it's mentioned that some clients won't work without SSL configured. Although Windows-specific instructions are irrelevant to us, I could foresee other clients having the same quirk.

As a concrete proposal for discussion: We could drop those options listed above in favor of a generic settings one, which is passed straight into the environment. That gives flexibility to folks with any use case, but also avoids having more than half the options of this module in the "not really the intended way of using it" bucket.

If we decide to keep the SSL options, then it's really just host and openFirewall that could potentially be eliminated. If non-local use is, as you say "not really the intended way of using it", then I think openFirewall can be dropped. It's also easy for users to work around if they do end up needing it. I'm not sure which local-only use cases would require changing host, but I'm not certain that there aren't any, and it's a one-liner that sets an env var, so supporting it is easy. My inclination would be to leave it.

What do you think?

[aanderse]

Oh wow... this sure dropped off the radar. @Valodim are you able to provide the feedback requested by @matt-snider?

[matt-snider]

@aanderse I don't know that we will get a response from @Valodim -- do you have an opinion on the above? The main part is this:

If we decide to keep the SSL options, then it's really just host and openFirewall that could potentially be eliminated. If non-local use is, as you say "not really the intended way of using it", then I think openFirewall can be dropped. It's also easy for users to work around if they do end up needing it. I'm not sure which local-only use cases would require changing host, but I'm not certain that there aren't any, and it's a one-liner that sets an env var, so supporting it is easy. My inclination would be to leave it.

Basically, we are just mapping nix options to ETESYNC environment variables for the most part, and I'm inclined to allow setting all of the supported variables. openFirewall is something I could drop - I don't really have an opinion on that since I don't use it.

[matt-snider]

@aanderse Another question: on another PR is submitted, there was a comment about how the tests aren't very useful. The same could probably be said about this one. I still didn't receive an answer on that comment about alternative ways to test. Do you think I should remove or change the tests for this PR too?

[aanderse]

@matt-snider I'll trust your judgement on this one - either way you see fit. Aside from the merge conflict I think this is ready to merge 👍

Possible future PRs against this module include:

• add systemd hardening options from the upstream unit
• utilize LoadCredential to avoid the need for an ExecStartPre, as well as having key files with secure permissions

Thanks for your work (and patience!) on this.

[matt-snider]

@aanderse I will remove the tests, as I think the criticism in the above linked comment holds about them testing relatively little compared to the cost of evaluation.

Those are some interesting suggestions and something I will think about adding going forwards.

In particular, I wasn't aware of the systemd LoadCredential directive - this is very cool! My initial reaction was to ask whether there are any plans to use this for NixOS secret management, but then I searched and came across RFC 59. Thank you for sharing that!

I will look into updating the PR ASAP!

[matt-snider]

@aanderse I removed the test so there are no conflicts anymore. I think we can merge it

[aanderse]

@matt-snider I'm really sorry I missed this before another merge conflict came up. I moved last week so I wasn't able to react as fast as I would have liked. 😞

[matt-snider]

@aanderse Hey thanks for staying on this. Now I'm the one who's sorry for the slow response! I rebased and fixed the conflict

Congrats on the move :-)

[aanderse]

Thanks @matt-snider!

[matt-snider]

Thank you for all your work reviewing this @aanderse !