-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
refactor: CLI commands for database management #2654
refactor: CLI commands for database management #2654
Conversation
These two are very similar, updated together. Restructures logic into functions, additional comments, local vars.
These two are very similar, updated together. Restructures logic into functions, additional comments, local vars.
These two are very similar, updated together. Restructures logic into functions, additional comments, local vars.
Restructures logic into functions, additional comments, local vars. Heavier refactor for this file. Dovecot Quota logic refactored, listing account aliases logic wrapped into it's own method. `echo` output adjusted to not depend on conditional branches, but could be reverted.
These two are very similar, updated together. Restructures logic into functions, additional comments, local vars.
Restructures logic into functions, additional comments, local vars.
Restructures logic into functions, additional comments, local vars.
These two are very similar, updated together. Restructures logic into functions, additional comments, local vars.
Restructures logic into functions, additional comments, local vars.
`MAIL_ACCOUNT` usage and method names for these files were misunderstood, renaming to what they should be.
Should not use white-space as a delimiter in the pattern..
Majority of shared logic extracted into `add-account.sh` for maintenance.
Majority of shared logic extracted into `update-account.sh` for maintenance.
- Added some notes. - Renamed some methods, `_password_hash` now takes args instead of using external vars.
Bring back a common `_validate_paramters()` to command files for now. Have `_account_already_exists` default to `postfix-accounts.cf` as that's the most commonly used DB for this method. Renamed `_mail_account` helper methods to scope name to the context.
- Logic restructured heavily in `listmailuser`, also fixing the quota regression I introduced (when moving the `ENABLE_QUOTA` guard into `_quota_show_for()`). - `addalias` + `delalias` share common validation check in the new helper, their migrated methods is unaltered. - The new helper adds some extra documentation.
Adds a common helper method to perform the DB check and list iterations while delegating a method for each to format the output per line/entry as they see fit. This helper calls an internal method that would be defined earlier by each command. Similar to how `__usage` is called by several methods in `helper.sh` already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional commentary if it's helpful.
Just to clarify. Multi-line values in .cf` files is not supported, nor is resolving recursive aliases. This just brings better consistency and reliability to the current support.
else # Remove target VALUE from entry: | ||
__db_list_already_contains_value || return 0 | ||
|
||
# The delimiter between key and first value may differ from | ||
# the delimiter between multiple values (value list): | ||
local LEFT_DELIMITER="\(${K_DELIMITER}\|${V_DELIMITER}\)" | ||
# If an entry for KEY contains an exact match for VALUE: | ||
# - If VALUE is the only value => Remove entry (line) | ||
# - If VALUE is the last value => Remove VALUE | ||
# - Otherwise => Collapse value to LEFT_DELIMITER (\1) | ||
sedfile --strict -i \ | ||
-e "/^${KEY_LOOKUP}\+${_VALUE_}$/d" \ | ||
-e "/^${KEY_LOOKUP}/s/${V_DELIMITER}${_VALUE_}$//g" \ | ||
-e "/^${KEY_LOOKUP}/s/${LEFT_DELIMITER}${_VALUE_}${V_DELIMITER}/\1/g" \ | ||
"${DATABASE}" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handles commands delalias
and delmailuser
removal of a recipient for an alias(es), and if it was the only recipient, removing the alias too. Both commands call this logic via postfix-virtual.sh
delete method which calls db.sh
with:
[[ ${MAIL_ALIAS} == '_' ]] && MAIL_ALIAS='\S\+' # delmailuser has a similar pattern but does not scope to a single alias
_db_entry_remove "${DATABASE_VIRTUAL}" "${MAIL_ALIAS}" "${RECIPIENT}"
KEY_LOOKUP
is the alias + the delimiter. In some cases the entry may have the delimiter multiple times before the value (eg: white-space with multiple spaces or tabs). As explained below and as the var name implies, it ensures we're looking up the entry key exactly. Avoiding substring mishaps.
The first sed expression uses \+
to deal with any padded delimiter between the key and first value. Usually this isn't necessary.
__db_list_already_contains_value || return 0
is just to avoid sedfile
throwing an error if the value doesn't exist. Technically the operation is successful since the result is the same. Could change sedfile --strict
to sed
instead?
Current related removal code for commands:
docker-mailserver/target/bin/delalias
Lines 19 to 23 in a84b8a1
sed -i \ | |
-e "/^${EMAIL} *${RECIPIENT}$/d" \ | |
-e "/^${EMAIL}/s/,${RECIPIENT}//g" \ | |
-e "/^${EMAIL}/s/${RECIPIENT},//g" \ | |
"${DATABASE}" |
docker-mailserver/target/bin/delmailuser
Lines 113 to 117 in a84b8a1
# delete all aliases where the user is the only recipient( " ${EMAIL}" ) | |
# delete user only for all aliases that deliver to multiple recipients ( ",${EMAIL}" "${EMAIL,}" ) | |
if sed -i \ | |
-e "/ ${EMAIL}$/d" -e "s/,${EMAIL}//g" -e "s/${EMAIL},//g" \ | |
"${ALIAS_DATABASE}" |
The EMAIL
(alias) scope for delalias
is vulnerable to substring matching. The wrong alias can be affected as well.
The recipient match in the 2nd sed expression for both commands does not include a right-side delimiter boundary. It is unlikely for the domain part of a recipient to result in a false-positive (due to substring match), but aliases do not need to be fully qualified email addresses either, they may be just the local part which is valid, which may be at more risk of deleting the wrong recipient in addition to the potential target recipient (delalias
is explicit, whereas delmailuser
is implicit with intent).
The third expression relies on the 2nd one to filter out the concern of a left-side delimiter boundary I think (as the expression applies to the output of the previous one?) EDIT: Nope, each subsequent expression has the full rewritten output to process, not only what was matched/changed.
Example of third sed expression substring match bug causing problems:
EMAIL='sales@example.com'
RECIPIENT='jane@example.com'
sed \
-e "/^${EMAIL} *${RECIPIENT}$/d" \
-e "/^${EMAIL}/s/,${RECIPIENT}//g" \
-e "/^${EMAIL}/s/${RECIPIENT},//g" \
<<< "sales@example.com jane@example.com,john@example.com,mary-jane@example.com,dave@example.com"
# Result: jane removed, but so was part of mary-jane, now there is "mary-dave"..
sales@example.com john@example.com,mary-dave@example.com
The 2nd sed expression would be vulnerable to the same flaw with domain part, such that a recipient domain of a neighbour could be rewritten to send somewhere else.. TLDs today is a huge list, with plenty of overlap opportunities.
Most likely to result in an accident, rather than an attack to redirect mail to another domain (the attacker probably has better options if they're able to know the recipient list order for an alias, and have control to perform a delete for the desired adjacent recipient).
delmailuser
version will delete entire alias mappings if the config is modified by a user. White-space is valid for separating a list of recipients, it's not restricted to ,
only. Shouldn't be an issue if they're only managing with our commands/setup.sh
. The 2nd and third sed expressions have the same flaw, but no longer scoped to a single alias.
New version avoids both substring issues by always ensuring the recipient is matched to the boundaries beside it (delimiter value or end of line).
( 'append' ) | ||
__db_list_already_contains_value && return 1 | ||
|
||
sedfile --strict -i "/^${KEY_LOOKUP}/s/$/${V_DELIMITER}${VALUE}/" "${DATABASE}" | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only used for addalias
command.
A separate method checks if the alias already has the recipient added.
- The error handling is higher level at
postfix-virtual.sh
based on status code returned. - That method is re-used for
remove
and could also helplistmailuser
alias list retrieval I think (which I did not find time to tackle).
Replaces that commands equivalent logic:
docker-mailserver/target/bin/addalias
Lines 42 to 51 in a84b8a1
grep \ | |
-qi "^$(_escape "${EMAIL}")[a-zA-Z@.\ ]*$(_escape "${RECIPIENT}")" \ | |
"${DATABASE}" 2>/dev/null && _exit_with_error "Alias \"${EMAIL} ${RECIPIENT}\" already exists" | |
if grep -qi "^$(_escape "${EMAIL}")" "${DATABASE}" 2>/dev/null | |
then | |
sed -i "/${EMAIL}/s/$/,${RECIPIENT}/" "${DATABASE}" | |
else | |
echo "${EMAIL} ${RECIPIENT}" >> "${DATABASE}" | |
fi |
EMAIL
for sed was not escaped, probably because of mixed usage with echo
. While db.sh
here handles that appropriately further down below. KEY_LOOKUP
contains the KEY
(alias) escaped and with the delimiter to avoid the substring match, recipient won't be at risk of being added to the wrong alias.
The earlier check for the alias in the DB was already handled in db.sh
a few lines above, most commands that need to perform that check now use that common method instead of copy/paste grep -qi
(and inconsistent usage of escaping which is it's own bug).
The earlier check for the recipient has a flaw due to restricting characters between the alias and the target recipient lookup to [a-zA-Z@.\ ]*
. Ignoring support for unicode, this didn't allow -
or _
either, a recipient in that search-space with -
or similar will ignore any valid match and allow appending the same recipient multiple times.
( 'replace' ) | ||
ENTRY=$(__escape_sed_replacement "${ENTRY}") | ||
sedfile --strict -i "s/^${KEY_LOOKUP}.*/${ENTRY}/" "${DATABASE}" | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most common operation beyond removal. Replaces the value in an entry for a key by rewriting the entire entry.
Most commands were using this, or pairing it with echo
if no other value existed (which db.sh
handles further down with the same ENTRY
var when the key does not exist for an entry).
The updatemailuser
+ updatedovecotmasteruser
commands had a rather ugly looking sed command due to the password hash being base64 encoded (there's other encoding options, but for whatever reason that wasn't considered at the time), that includes the /
character in the base64 "alphabet", thus problematic in a sed command:
sed -i "s ^""${USER}""|.* ""${USER}""|""${HASH}"" " "${DATABASE}" |
__escape_sed_replacement
fixes that by escaping /
and &
, it does not escape \
- which I'd rather we had a check to reject on among other special characters instead of escaping them.
Some commands have since been updated to be smarter with the replace approach, but previously, and with the current setquota
, there is a call to a delete command to then add the value as new, instead of update the existing one:
docker-mailserver/target/bin/setquota
Lines 42 to 43 in a84b8a1
delquota "${USER}" | |
echo "${USER}:${QUOTA}" >>"${DATABASE}" |
It makes sense to maintain a single command for the same generic functionality IMO.
The quota commands aren't escaping the target account either, not that a single wildcard .
has much likelihood of matching the wrong user (at least domain part, local part may be at more risk with enough users).
Each relay command leverages this. They roughly had the same pattern (exclude being slightly different):
docker-mailserver/target/bin/addrelayhost
Lines 39 to 46 in a84b8a1
if grep -qi "^@${DOMAIN}" "${DATABASE}" 2>/dev/null | |
then | |
sed -i \ | |
"s|^@${DOMAIN}.*|@${DOMAIN}\t\t[${HOST}]:${PORT}|" \ | |
"${DATABASE}" | |
else | |
echo -e "@${DOMAIN}\t\t[${HOST}]:${PORT}" >>"${DATABASE}" | |
fi |
docker-mailserver/target/bin/addsaslpassword
Lines 26 to 33 in a84b8a1
if grep -qi "^@${DOMAIN}" "${DATABASE}" 2>/dev/null | |
then | |
sed -i \ | |
"s|^@${DOMAIN}.*|@${DOMAIN}\t\t${USER}:${PASSWD}|" \ | |
"${DATABASE}" | |
else | |
echo -e "@${DOMAIN}\t\t${USER}:${PASSWD}" >>"${DATABASE}" | |
fi |
docker-mailserver/target/bin/excluderelaydomain
Lines 14 to 19 in a84b8a1
if grep -qi "^@${DOMAIN}" "${DATABASE}" 2>/dev/null | |
then | |
sed -i "s/^@${DOMAIN}.*/@${DOMAIN}/" "${DATABASE}" | |
else | |
echo -e "@${DOMAIN}" >> "${DATABASE}" | |
fi |
function _db_entry_add_or_append { _db_operation 'append' "${@}" ; } # Only used by addalias | ||
function _db_entry_add_or_replace { _db_operation 'replace' "${@}" ; } | ||
function _db_entry_remove { _db_operation 'remove' "${@}" ; } | ||
|
||
function _db_operation | ||
{ | ||
local DB_ACTION=${1} | ||
local DATABASE=${2} | ||
local KEY=${3} | ||
# Optional arg: | ||
local VALUE=${4} | ||
|
||
# K_DELIMITER provides a match boundary to avoid accidentally matching substrings: | ||
local K_DELIMITER KEY_LOOKUP | ||
K_DELIMITER=$(__db_get_delimiter_for "${DATABASE}") | ||
# Due to usage in regex pattern, KEY needs to be escaped: | ||
KEY_LOOKUP="$(_escape "${KEY}")${K_DELIMITER}" | ||
|
||
# Support for adding or replacing an entire entry (line): | ||
# White-space delimiter should be written into DATABASE as 'space' character: | ||
local V_DELIMITER="${K_DELIMITER}" | ||
[[ ${V_DELIMITER} == '\s' ]] && V_DELIMITER=' ' | ||
local ENTRY="${KEY}${V_DELIMITER}${VALUE}" | ||
|
||
# Support for 'append' + 'remove' operations on value lists: | ||
# NOTE: Presently only required for `postfix-virtual.cf`. | ||
local _VALUE_ | ||
_VALUE_=$(_escape "${VALUE}") | ||
# `postfix-virtual.cf` is using `,` for delimiting a list of recipients: | ||
[[ ${DATABASE} == "${DATABASE_VIRTUAL}" ]] && V_DELIMITER=',' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is a bit noisy/verbose. The inline comments should document it fairly well, but to re-iterate:
- Convenience methods handle
DB_ACTION
andDATABASE
args, those methods themselves are called with theKEY
andVALUE
args. Presently the only timeVALUE
is optional is with theexcluderelaydomain
command (replace
) and when performingdelete
forpostfix-accounts.cf
/dovecot-masters.cf
+dovecot-quotas.cf
databases/configs (postfix-virtual.cf
always provides aVALUE
). _db_has_entry_with_key
defined near the bottom ofdb.sh
shares theK_DELIMITER
block,K_
being a prefix for the KEY delimiter which__db_get_delimiter_for
provides.- Usually the delimiter is the same (eg:
|
forpostfix-accounts.cf
), but for white-space we want to match any with\s
and default toV_DELIMITER
(V_
being prefix for VALUE) helps distinguish it's usage instead of overloading a singleDELIMITER
. ENTRY
(for adding/replacing an entry) needsV_DELIMITER
, but after that the usage is overloaded (if needed, presently only forpostfix-virtual.cf
to a delimiter for when VALUE is an array/list - handled inappend
andremove
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like it. If in doubt, I'd go with more verbose than less verbose, for "future generations" :D
# Internal method for: _db_operation | ||
function __db_list_already_contains_value | ||
{ | ||
# Avoids accidentally matching a substring (case-insensitive acceptable): | ||
# 1. Extract the current value of the entry (`\1`), | ||
# 2. If a value list, split into separate lines (`\n`+`g`) at V_DELIMITER, | ||
# 3. Check each line for an exact match of the target VALUE | ||
sed -e "s/^${KEY_LOOKUP}\(.*\)/\1/" \ | ||
-e "s/${V_DELIMITER}/\n/g" \ | ||
"${DATABASE}" | grep -qi "^${_VALUE_}$" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more reliable implementation that these two methods cover (listmailuser
presently does not use it, so isn't as robust currently):
docker-mailserver/target/bin/addalias
Lines 42 to 44 in a84b8a1
grep \ | |
-qi "^$(_escape "${EMAIL}")[a-zA-Z@.\ ]*$(_escape "${RECIPIENT}")" \ | |
"${DATABASE}" 2>/dev/null && _exit_with_error "Alias \"${EMAIL} ${RECIPIENT}\" already exists" |
docker-mailserver/target/bin/listmailuser
Line 44 in a84b8a1
if [[ -f ${ALIASES} ]] && grep -q "${USER}" "${ALIASES}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we change listmailuser
to use this as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we change
listmailuser
to use this as well?
The concern is raised in the listmailuser
review comment. A check that is more robust won't be as helpful if the actual logic that is performed is problematic itself 😅
local PASSWD_HASH | ||
PASSWD_HASH=$(doveadm pw -s SHA512-CRYPT -u "${MAIL_ACCOUNT}" -p "${PASSWD}") | ||
# Early failure above ensures correct operation => Add (create) or Replace (update): | ||
_db_entry_add_or_replace "${DATABASE}" "${MAIL_ACCOUNT}" "${PASSWD_HASH}" | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create
and update
could be split with a bit of code duplication, it wouldn't be that harmful with co-location. That wouldn't really change the DB operation being called though (presently both add and update are under the replace
action/operation, just different branches based on if the KEY/account exists in the DB).
This doveadm
command isn't actually using -u
. Dovecot docs for the pw
command note that it's only relevant for a different digest scheme (-s
). It could be removed.
echo -e " [ aliases -> $(grep "${USER}" "${ALIASES}" | awk '{print $1;}' | sed ':a;N;$!ba;s/\n/, /g')]\n" | ||
else | ||
echo | ||
grep "${MAIL_ACCOUNT}" "${DATABASE_VIRTUAL}" | awk '{print $1;}' | sed ':a;N;$!ba;s/\n/, /g' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get around to trying to interpret what sed ':a;N;$!ba;s/\n/, /g'
translates into. As such I didn't bother to have _account_has_an_alias
leverage the more robust __db_list_already_contains_value
method (which only returns a status).
I understand the grep here will return any lines from the postfix-virtual.cf
database file regardless of substring matches (including alias keys), and then do some incantation with those lines to extract the aliases (keys) that matches were found?
`source-path` added to `index.sh` is sufficient if used directly above the `source` line, otherwise it can be placed early on in the sourced file, but must be before the dependent `source` lines appear (function scope is ignored). Likewise, I forgot that the `PATH_TO_SCRIPTS` var is only for a common ancestor of the real path and shellchecks source path, it's stripped/ignored by shellcheck in favor of trying any known source-path. So `/manage` must be added to each of these `source` lines instead. The prior `source-path` applied within the function, as noted only applied to the direct line beneath it, not the entire scope. Added comments for future maintainers to be aware of. Decided to keep it all within this `db.,sh` file, rather than `helpers/index.sh` , although perhaps that's a better place to document/demonstrate such. Otherwise `test/linting/lint.sh` covers this advice for the most part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I took some time to look at everything, but this is a rather large PR, so there is a fair chance of missing something. But as far as I can tell, LGTM 👍🏼
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I took some time to look at everything, but this is a rather large PR, so there is a fair chance of missing something. But as far as I can tell, LGTM 👍🏼
I second this. Also the tests all pass, so I finally approve this 🎉
Thanks to both of you for taking the time to review. Again my apologies for the size, I realize how daunting that'd be to review 😅 |
Description
In preparation for switching the file locking back to flock, I wanted to clean up the CLI commands involved, but got a bit carried away 😅
It should also help with keeping the encryption feature PR simple to resolve/test.
Commands refactored:
Overall changes involve:
_main
method at the top provides an overview of logical steps:helpers/database/db.sh
), the_main
is called at the bottom of the file.delmailuser
additionally processes option support for-y
prior to calling_main
.__usage
is now consistent with each of these commands, along with thehelp
command.USER
/EMAIL
/FULL_EMAIL
to refer to the same expected value).helpers/database/manage/
) using a common structure for managing changes to their respective "Database" config file.postfix-accounts.sh
unified not only add and update commands, but also all the dovecot-master versions, a single password call for all 4 of them, with a 5th consumer of the password prompt from the relay commandaddsaslpassword
.helpers/database/db.sh
which provides a common API to support the changes made.db.sh
is only really managing writes. I didn't see a nice way for removing the code duplication for list commands as the duplication was fairly minimal, especially forlistalias
andlistdovecotmasteruser
which were quite simple in their differences in the loop body.listmailuser
anddelmailuser
also retain methods exclusive to respective commands, I wasn't sure if there was any benefit to move those, but they were refactored.Happy to drop the
helpers/database/**
files if it seems a bit over-engineered 👍I figured with
flock
the locks can often be done in thedb.sh:_db_operation
method, only some methods may want to lock multiple files for larger operations such asdelmailuser
. This would be fine withflock -x
in the same process, and not conflict withflock -x
in_db_operation
(it'll re-use the existing lock for the FD it has instead of blocking).Apologies for the large commit history not likely being that helpful for review (file diffs probably aren't great either). I went through several iterations to reach this point.
If the PR changes are a bit large for review, let me know and I can split this out into smaller PRs.
Commit messages for reference
Type of change
Checklist: