bootstrap: allow to give password from a file #1218
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1218 +/- ##
==========================================
- Coverage 86.92% 86.57% -0.36%
==========================================
Files 64 64
Lines 12628 11859 -769
==========================================
- Hits 10977 10267 -710
+ Misses 1651 1592 -59 🚀 New features to boost your workflow:
|
|
Thanks @nitnelave for the very fast review and merge :) |
|
@ibizaman you caught me at an extremely lucky moment, these days I usually only review PRs every few days. You just happened to send this while I was reviewing something else! |
|
Luck is always part of it. Thanks nonetheless :) |
| declare "$field"="$(printf '%s' "$user_config" | jq --raw-output --arg field "$field" '.[$field]')" | ||
| done | ||
| printf -- '\n--- %s ---\n' "$id" | ||
|
|
||
| create_update_user "$id" "$email" "$displayName" "$firstName" "$lastName" "$avatar_file" "$avatar_url" "$gravatar_avatar" "$weserv_avatar" | ||
| redundant_users="$(printf '%s' "$redundant_users" | jq --compact-output --arg id "$id" '. - [$id]')" | ||
|
|
||
| if [[ "$password" != 'null' ]] && [[ "$password" != '""' ]]; then | ||
| if [[ "$password_file" != 'null' ]] && [[ "$password_file" != '""' ]]; then | ||
| "$LLDAP_SET_PASSWORD_PATH" --base-url "$LLDAP_URL" --token "$TOKEN" --username "$id" --password "$(cat $password_file)" |
There was a problem hiding this comment.
This looks like it's leaking the password in process listings (which are visible to all processes). Adding a --password-file option to $LLDAP_SET_PASSWORD_PATH, to read the password directly from the file, should fix that.
There was a problem hiding this comment.
Why not put it in the env instead, then?
There was a problem hiding this comment.
What feature does that bring? (I'm not familiar with the code, so not sure why you'd want that.) I would avoid secrets in env vars if possible, to reduce risk of leakage.
There was a problem hiding this comment.
I added this because I wanted to avoid to embed the password in the config file directly. I wanted the password to be a reference to a file instead.
This comes from wanting to generate the config file and embedding the passwords at generation time leaks the passwords in other parts of the system.
I’m not sure I follow what your proposed fix would be, sorry.
There was a problem hiding this comment.
If we have an env var, we can write PASSWORD=$(cat password_file) set_password --user ...
That's simpler and more flexible
There was a problem hiding this comment.
Oh I see what you mean @bjornfor. You mean having the binary itself read the file. I’ll create a PR to do that. It makes much more sense indeed.
There was a problem hiding this comment.
Of the two, I'd prefer the env solution, unless you have compelling arguments
There was a problem hiding this comment.
No arguments. The only thing is the comment above about avoiding leakage. I can do either.
There was a problem hiding this comment.
I'd need a more specific attack scenario than "avoid leakage". Sure, if you set it in your global env and start other processes they'll see it, but that's not what's proposed here
In the bootstrap script, instead of embedding the password in the config file directly, allow to pass a path to a file which contains the password.
This was tested in a NixOS draft PR NixOS/nixpkgs#425923.