refactor: CLI commands for database management#2654
refactor: CLI commands for database management#2654polarathene merged 55 commits intodocker-mailserver:masterfrom
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.
polarathene
added
service/dovecot
service/postfix
area/scripts
kind/improvement
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
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.
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
docker-mailserver/target/bin/delmailuser
Lines 113 to 117 in a84b8a1
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='[email protected]'
RECIPIENT='[email protected]'
sed \
-e "/^${EMAIL} *${RECIPIENT}$/d" \
-e "/^${EMAIL}/s/,${RECIPIENT}//g" \
-e "/^${EMAIL}/s/${RECIPIENT},//g" \
<<< "[email protected] [email protected],[email protected],[email protected],[email protected]"
# Result: jane removed, but so was part of mary-jane, now there is "mary-dave"..
[email protected] [email protected],[email protected]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.
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.shbased on status code returned. - That method is re-used for
removeand could also helplistmailuseralias 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
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.
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:
__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
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
docker-mailserver/target/bin/addsaslpassword
Lines 26 to 33 in a84b8a1
docker-mailserver/target/bin/excluderelaydomain
Lines 14 to 19 in a84b8a1
| 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.
This section is a bit noisy/verbose. The inline comments should document it fairly well, but to re-iterate:
- Convenience methods handle
DB_ACTIONandDATABASEargs, those methods themselves are called with theKEYandVALUEargs. Presently the only timeVALUEis optional is with theexcluderelaydomaincommand (replace) and when performingdeleteforpostfix-accounts.cf/dovecot-masters.cf+dovecot-quotas.cfdatabases/configs (postfix-virtual.cfalways provides aVALUE). _db_has_entry_with_keydefined near the bottom ofdb.shshares theK_DELIMITERblock,K_being a prefix for the KEY delimiter which__db_get_delimiter_forprovides.- Usually the delimiter is the same (eg:
|forpostfix-accounts.cf), but for white-space we want to match any with\sand 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.cfto a delimiter for when VALUE is an array/list - handled inappendandremove.
There was a problem hiding this comment.
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.
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
docker-mailserver/target/bin/listmailuser
Line 44 in a84b8a1
There was a problem hiding this comment.
So should we change listmailuser to use this as well?
There was a problem hiding this comment.
So should we change
listmailuserto 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.
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.
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.
georglauterbach
left a comment
georglauterbach
left a comment
There was a problem hiding this comment.
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.
casperklein
left a comment
casperklein
left a comment
There was a problem hiding this comment.
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 😅 |
3 participants
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:
_mainmethod at the top provides an overview of logical steps:helpers/database/db.sh), the_mainis called at the bottom of the file.delmailuseradditionally processes option support for-yprior to calling_main.__usageis now consistent with each of these commands, along with thehelpcommand.USER/EMAIL/FULL_EMAILto refer to the same expected value).helpers/database/manage/) using a common structure for managing changes to their respective "Database" config file.postfix-accounts.shunified 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.shwhich provides a common API to support the changes made.db.shis 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 forlistaliasandlistdovecotmasteruserwhich were quite simple in their differences in the loop body.listmailuseranddelmailuseralso 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
flockthe locks can often be done in thedb.sh:_db_operationmethod, only some methods may want to lock multiple files for larger operations such asdelmailuser. This would be fine withflock -xin the same process, and not conflict withflock -xin_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: