Debian/Ubuntu: use non-root evcc user #4901
Conversation
|
Ich halte das für eine gute Idee, kann aber nicht beurteilen, ob der PR korrekt ist. Können wir einen unabhängigen Review dafür bekommen? |
andig
changed the title
|
@goebelmeier ist so nett und wird das für mich testen. |
|
Bitte die korrigierte Fassung, mein Fix (copy/paste error) war unsinnig, hab es korrigiert. |
|
@goebelmeier Thanks for testing, I fixed the home dir creation - this might still leave open some very special cornercases (/var/lib/evcc exists but belongs to another user), but I think we are safe to assume someone in that scenario being able to resolve that manually. |
|
Fehlt hier noch was? Sonst wäre jetzt nach Release der richtige Zeitpunkt zum mergen. |
|
Gegentesten könnte ich das noch, weiß aber nicht ob ich das morgen unter kriege |
|
Wie schaut es aus? Fehlt noch was? |
|
@pauxus Zeit das fertig zu machen ;) Aktuell liegt die db ja unter Also wäre die Fragen, wie wir dann wieder an die richtige Location kommen. |
|
@andig Ich verschiebe sie nach /var/lib/evcc (also ~evcc statt ~evcc/.evcc), und natürlich habe ich aus irgendeinem Grund vergessen, die angepasste Config mit zu commiten. Argh. Neues Jahr, neues Glück. Ich rebase dann auch gleich nochmal auf den letzten Release. |
- include preinstall script that stops the service (with flag) and copies over the database - postinstall also recognizes restart flag in above case - unit file uses evcc user Fixes #4900
|
So, nochmal alles durchgetestet. Funktioniert auch soweit alles, Service läuft mit dem richtigen Nutzer, DB ist kopiert. Einen Cornercase gibt es, über den ich bei meinem letzten Test gestolpert bin: ich hatte manuell bei mir einen anderen Nutzer über |
|
Allerdings ist da ein Gedankenfehler drin, muss ich gleich noch korrigieren |
Don't change the default location, we only want this for the systemd service
|
Macht natürlich keinen Sinn, den Pfad zur DB global zu überschreiben, sondern nur für die Systemd Unit, ich habe es jetzt explizit im Unitfile gesetzt. Ich werde noch einen Sanity Check was geänderte Nutzer oder doppelte DBs angeht einbauen |
Das wär mir egal. Wer das kann kann auch die DB wieder finden. |
- Copy database even if run as non root - check if database already exists before copying, if so fail - Fail if existing homedir could not be moved instead of warn only - Warn if there are systemd overrides, in that case, service is not restarted
|
Hab jetzt noch ein paar Sanity checks dazugepackt, ich denke damit sind alle Szenarien erschlagen |
|
@goebelmeier könntest Du nochmal einen Blick drauf werden? Soweit ok? Rein damit? /cc @premultiply ? |
| User=evcc | ||
| Group=evcc |
There was a problem hiding this comment.
Instead of using a statically allocated user, let systemd manage the user dynamically. That should simplify the install script somewhat.
| User=evcc | |
| Group=evcc | |
| DynamicUser=true | |
| User=evcc |
Then let systemd create the state directory at /var/lib/evcc and change the working directory using
StateDirectory=evcc
WorkingDirectory=/var/lib/evcc
From what I remember systemd also manages the StateDirectory migration from/to DynamicUser=, so if /var/lib/evcc already exists it should migrate that data.
There was a problem hiding this comment.
Good point - this would simply the script. I will give it a spin tomorrow.
There was a problem hiding this comment.
I have no idea what you‘re talking about. Staying tuned :)
There was a problem hiding this comment.
I did again look into it (was almost tomorrow...) and would rather not use DynamicUser. The directories would be created inside /var/lib/private/evcc and symlinked to /var/lib/evcc which introduces unnecessary complexity. Also it would no make the script much simpler. We still would need to create the directory, we still need to copy the database and we would still need to chown (systemd only chowns the containing files if the owner of the directory is wrong, so this might lead to effects, if the user/directory already exists).
The only thing we could do would be to set StateDirectory and use the STATE_DIRECTORY env variable in the call, but I don't think this would help very much.
There was a problem hiding this comment.
Yeah, if you have state to migrate in post-install that gets hairy fast. Most of the issues you describe seem to originate from a migration story, but I think they are easily solvable.
if the user/directory already exists
You would need to delete the persistent user.
we would still need to chown (systemd only chowns the containing files if the owner of the directory is wrong
After removing the persistent user the owner/group would certainly mismatch.
The directories would be created inside /var/lib/private/evcc and symlinked to /var/lib/evcc which introduces unnecessary complexity.
I think the complexity you see is that it only gets created, when the unit is first started. But as you noticed you can create it yourself and I think that is reasonable given that you want to migrate old state.
I think systemd is a very neat tool to manage a service's lifetime. Find below my personal systemd unit running on NixOS. It goes beyond the scope of this PR, especially by including hardening options, but maybe you'll find it interesting or even useful.
# /etc/systemd/system/evcc.service
[Unit]
After=network-online.target mosquitto.target mosquitto.service
[Service]
Environment="HOME=/var/lib/evcc"
Environment="LOCALE_ARCHIVE=/nix/store/wqzfxm6fni76h5w9m17q7zkl7h7isk96-glibc-locales-2.35-163/lib/locale/locale-archive"
Environment="PATH=/nix/store/57xv61c5zi8pphjbcwxxjlgc34p61ic9-glibc-2.35-163-bin/bin:/nix/store/a7gvj343m05j2s32xcnwr35v31ynlypr-coreutils-9.1/bin:/nix/store/mydc6f4k2z73xlcz7ilif3v2lcaiqv>
Environment="TZDIR=/nix/store/i16lgq16av602nfyws3ps8dd9yj36dwh-tzdata-2022f/share/zoneinfo"
CapabilityBoundingSet=
DeviceAllow=char-ttyUSB
DevicePolicy=closed
DynamicUser=true
ExecStart=/nix/store/qc0vs3by55v0daf7ii854d56kz3nwsf0-evcc-0.109.2/bin/evcc --config /nix/store/fzbldlqmx5ambi3hmf3jhagb3nvc33qp-evcc.yml
LockPersonality=true
MemoryDenyWriteExecute=true
PrivateTmp=true
PrivateUsers=true
ProcSubset=pid
ProtectClock=true
ProtectControlGroups=true
ProtectHome=true
ProtectHostname=true
ProtectKernelLogs=true
ProtectKernelModules=true
ProtectKernelTunables=true
ProtectProc=invisible
RestrictAddressFamilies=AF_INET
RestrictAddressFamilies=AF_INET6
RestrictAddressFamilies=AF_UNIX
RestrictNamespaces=true
RestrictRealtime=true
StateDirectory=evcc
SystemCallArchitectures=native
SystemCallFilter=@system-service
SystemCallFilter=~@privileged
UMask=0077
User=evcc
WorkingDirectory=/var/lib/evccThere was a problem hiding this comment.
You would need to delete the persistent user.
Since DynamicUser CAN work with preexisting users, this can get fishy rather. If you have a user (log reader or similar) that is part of the group evcc and that group is deleted, you will have unexpected consequences. I would suggest sticking with fixed users. You can always use your really strict (in a positive way, you are correct, I find it one of the most hardened unit files that I have seen so far) unit configuration in an override file.
|
@goebelmeier @premultiply @mweinelt last call- habt ihr noch Anmerkungen? Mir erscheint das sonst vollständig und hoffentlich auch frei von Nebenwirkungen und ich würde es ins Nightly mergen? |
|
LGTM |
|
Es sieht mir gut aus, ich hab relativ viel Fuzzy Testing gemacht und es ist nicht zerbrochen. Also könnte man IMHO reinnehmen |
|
Dann Mut zur Lücke. Danke fürs Feedback! @VolkerK62 im apt Package wandert die SQlite Datenbank jetzt nach https://github.com/evcc-io/evcc/pull/4901/files#diff-fc00ac250b8d002d87e12d46f86302b54999b140a3048acb82f91de09df794e7R13 |
|
Und natürlich Dank an @pauxus |
Ich habe die Kommentierungen gelesen und nix verstanden. |
|
Ne, die sollte per Skript umziehen, liegt aber danach am neuen Ort. Im Logfile sagt evcc, welche verwendet wird |
|
Sollte dieser PR in dem Nightly von vorhin (wegen freeze) enthalten sein? |
|
This error has just popped up in the wild: hatte grade von 0.111.? auf 0.112.5 aktualisiert. nach dem ändern "chown evcc:evcc /bla/bla" vom Verzeichnis in dem die SQlite db ist und die db file - läuft alles wieder rund. -> is that something we need to fix in the packaging? |
|
The change was not part of 112.5. You reverted beforehand and revert-reverted afterwards. Need more details:
|
|
I guess it was root |
|
This was the error or warning I saw during the update. |
|
Ok, this is a corner case. We could add a chown after the creation of the user, but I don't know whether this is really necessary. I assume you created the folder manually? Or do you have any overrides in your systemd module (like stateDirectory)? |
|
No didn't created it but using the nightly and added it to the config file a while ago. Not sure why I had to do it but was related to not saving the chargelogs persistent as I remember correctly. |
|
This is the error on todays update: process (31452) was the previous EVCC running process. |
|
Let's continue in #6162, seems this is more serious. |
|
There is another issue caused by this, which needs to be addressed: #6140 (comment)
|
copies over the database
Fixes #4900
Refs #4878