nixos/miniflux: add database.* options #108906
Conversation
|
@GrahamcOfBorg test miniflux |
There was a problem hiding this comment.
This looks very good, thank you for your efforts!
I spotted one issue: Special characters in the password are not escaped properly. Once it has to be quoted for the purposes of protecting it from shell expansion (and to be SQL-safe), the second time so that DATABASE_URL is always a valid url.
Unfortunately, I don't know the recommend nixpkgs way of quoting. Perhaps others can weigh in on this question?
In any case, this problem existed also before this pull request.
Before this change the database password was hardcoded to "miniflux", so there was no need for performing special quoting.
I'm not sure how would we url-encode the characters of a string using just Nix. An alternative that comes to my mind is to add an assertion checking that the password matches a regular expression (such as `[a-zA-Z0-9%]). If the assertion fails, building the configuration should abort and indicate the user to URL-encode the password to prevent problems. |
|
Besides the issue of character escaping, the changes look good to me. They will conflict with my PR #111030 but I can easily fix it if this PR gets merged first. |
aanderse
force-pushed
the
nixos/miniflux
branch
from
e7f8a05
to
0a8c558
Compare
|
@iblech @cript0nauta rebased and added some database escaping. Can you please take another look? Thanks! |
There was a problem hiding this comment.
Anyway, I don't see an attack surface useful to a malicious user. And in the case a password just has special characters, the worst case will be miniflux failing to start. Because the default password doesn't have special characters, it is unlikely that this change will cause a regression. In my opinion, it is good enough for merging.
| ${pgbin}/createdb --owner "${dbUser}" "${dbName}" | ||
| ${pgbin}/psql "${dbName}" -c "CREATE EXTENSION IF NOT EXISTS hstore" | ||
| if ! db_exists "${cfg.database.name}"; then | ||
| ${pgbin}/psql postgres -c "CREATE ROLE \"${cfg.database.user}\" WITH LOGIN NOCREATEDB NOCREATEROLE ENCRYPTED PASSWORD '${cfg.database.password}'" |
There was a problem hiding this comment.
Is this the database escaping you added? In my experience, what causes escaping bugs in the connection string is the password because it might have special characters. Not the username.
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
|
@aanderse oh I was going to test fully removing passwords: symphorien@861ca32 Honestly the miniflux service is sooo simple that if you don't want a local database you are better off rewriting everything from scratch, so I'd favor simplicity with no options. I think I'll still open a PR when I test it for real (the nixos tests works) to drive discussion. |
stale
bot
removed
the
2.status: stale
|
@symphorien ok cool. I don't use the service so have no real interest. Can you please close #108386 with a resolution and we'll call it a day? Thanks! |
Motivation for this change
#108386
From here someone (@bricewge? @nornagon? ❤️) should make a PR to:
database.passwordoption with adatabase.passwordFileoptiondatabase.socketoption and start using socket authentication by default to avoid passwords entirelyNote that while this PR does add a
passwordoption, storing a plain-text password in the nix store, the value was already being placed in the nix store - I'm just making it explicit so no one is mistaken in what they are doing when they use this module. We really need an additional PR to replacedatabase.passwordwithdatabase.passwordFileas mentioned above.If anyone is interested in writing such a PR but needs some advice I'd be more than willing to share my thoughts.
ping @criena for testing.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)