-
Notifications
You must be signed in to change notification settings - Fork 88
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
Adds DKIM support for multiple domains #89
Conversation
…ningTable,ExternalIgnoreList,InternalHosts
entrypoint.sh
Outdated
|
||
echo "${HOSTNAME} default._domainkey.${HOSTNAME}" >> /etc/opendkim/SigningTable | ||
|
||
echo "${HOSTNAME}" >> /etc/opendkim/TrustedHosts |
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.
Please fix the intention. And if you append with >>
, does it mean that on every SMF restart new lines will be added to /etc/opendkim/{KeyTable, SigningTable, TrustedHosts}
?
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.
Thanks for checking! Indentation fixed at line 220, unless you meant something else.
The >>
is required, because for all files {KeyTable, SigningTable, TrustedHosts}
I am inserting infos on multiple occasions:
- first for the entry your PR creates (the ${HOSTNAME})
- over a loop for all
virtualDomain in $virtualDomains
All new files in /etc/opendkim/
are re-created at each restart
or down/up
. They are dynamic according to the inserted domains, so there is no danger of inserting new lines.
docker exec -it smf cat /etc/opendkim/KeyTable | wc -l
10
docker-compose restart
docker exec -it smf cat /etc/opendkim/KeyTable | wc -l
10
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.
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.
Whops! Thanks for pointing it out. Fixed.
entrypoint.sh
Outdated
|
||
echo "${HOSTNAME}" >> /etc/opendkim/TrustedHosts | ||
|
||
for virtualDomain in $virtualDomains; do |
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.
HOSTNAME
is probably also in $virtualDomains
, so I suspect that configuration for HOSTNAME
is done twice.
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.
From what I understand from here, HOSTNAME
is not part of $virtualDomains
. I did check for this empirically, to be sure. A folder /var/db/dkim/${HOSTNAME}/
was not created in a for loop over $virtualDomains
.
In any case, I think that this handles the case of reverting to a single domain (your PR), because HOSTNAME and all forwarded domains are kept separately.
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.
- SMF_CONFIG=@example.com:xxx@gmail.com:pwd
- SMF_DOMAIN=example.com
This is my config, based on https://github.com/dgraziotin/docker-simple-mail-forwarder/blob/99f27761e49e131a52b03159d69e7c652d25ffff/entrypoint.sh#L101 virtualDomains
is a list of domains extracted from emailFrom
which is the first element when split with :
. I have to actually test but I'm quite sure that with my config, example.com
is in virtualDomains
and SMF_DOMAIN
.
And what do you think, is #83 actually needed? Maybe dkim should always use a per-domain path style? Even if there is only one domain? This would mean removing this and removing SMF_DKIM_ALL
here.
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.
@petslane it was great that you pointed out your case, because it showcases my different use case! Luckily, both seem covered.
I think that we are both right: your HOSTNAME
is part of virtualDomains
because it is extracted from SMF_CONFIG
. Normally people will have addresses @${HOSTNAME}
in SMF_CONFIG
.
I do not. My HOSTNAME
is dedicated to the SMTP server and does not appear in SMF_CONFIG
. My domains use HOSTNAME
in their MX entries. HOSTNAME
is not used for sending or forwarding.
In light of this, I believe that both solutions should stay. #83 is needed for those with a single domain for everything (which I believe will be most people). #89 is for more complex cases. Everything is handled in entrypoint.sh
and, if I did not make any mistake, things should stay clean all in all. @huan, what do you think?
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.
In light of this, I believe that both solutions should stay. #83 is needed for those with a single domain for everything (which I believe will be most people). #89 is for more complex cases.
I have only one domain, why shouldn't I use your way of setting DKIM keys? From the user's point of view, it's currently complicated as there are 2 ways to set up DKIM, what option should the user choose? Why not have only one way to set up DKIM for SMF, your way, and there is no difference in how many domains are used (I expect that your way should work also with only 1 domain).
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 would be OK to have both solutions merged, but I am not sure I could deliver this in short. PR to the PR are welcome, I guess 😄
Edit: I might have some time, after all.
FYI, I am using this PR in production (😨) and testing it with dgraziotin/simple-mail-forwarder-dkim. I have four domains plus |
@petslane this last push merges #83 and #89 following my approach. You are right, it looks cleaner now. No config variable needed. Please review, though, and test. I hope I am covering all edge cases. Edit: I am now making sure that config lines are never entered twice accidentally. I once found duplicated entries in a config file. This never repeated, but you made me suspicious it might happen sometimes. Now it won't happen. |
entrypoint.sh
Outdated
echo "Inserting ${HOSTNAME} data to /etc/opendkim/{KeyTable, SigningTable, TrustedHosts}" | ||
echo "default._domainkey.${HOSTNAME} ${HOSTNAME}:default:/var/db/dkim/${HOSTNAME}/default.private" >> /etc/opendkim/KeyTable | ||
echo "${HOSTNAME} default._domainkey.${HOSTNAME}" >> /etc/opendkim/SigningTable | ||
echo "${HOSTNAME}" >> /etc/opendkim/TrustedHosts | ||
|
||
chmod 400 /var/db/dkim/default.private | ||
chown opendkim:opendkim /var/db/dkim/default.private | ||
for virtualDomain in $virtualDomains; do | ||
# skip generating keys for $HOSTNAME twice in case it is also used as forwarded domain. | ||
if [ "$virtualDomain" = "$HOSTNAME" ]; then | ||
continue | ||
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.
echo "Inserting ${HOSTNAME} data to /etc/opendkim/{KeyTable, SigningTable, TrustedHosts}" | |
echo "default._domainkey.${HOSTNAME} ${HOSTNAME}:default:/var/db/dkim/${HOSTNAME}/default.private" >> /etc/opendkim/KeyTable | |
echo "${HOSTNAME} default._domainkey.${HOSTNAME}" >> /etc/opendkim/SigningTable | |
echo "${HOSTNAME}" >> /etc/opendkim/TrustedHosts | |
chmod 400 /var/db/dkim/default.private | |
chown opendkim:opendkim /var/db/dkim/default.private | |
for virtualDomain in $virtualDomains; do | |
# skip generating keys for $HOSTNAME twice in case it is also used as forwarded domain. | |
if [ "$virtualDomain" = "$HOSTNAME" ]; then | |
continue | |
fi | |
allDomains="$virtualDomains"' | |
[[ $allDomains =~ $HOSTNAME ]] || { | |
allDomains="$allDomains $HOSTNAME" | |
} | |
for virtualDomain in $allDomains; do |
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.
Very nice! And it does not manipulate virtualDomains
, solving cases like mine. I have to apply this manually, not supported here on GitHub.
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.
@petslane just to confirm, there is a typo on line 202 (extra '
). I'm asking because, you never know with bash exoteric stuff.
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.
Yes, this is typo. '
key too close to enter
key.
I like this solution a lot better! |
Co-authored-by: Peeter N <petslane@users.noreply.github.com>
Co-authored-by: Peeter N <petslane@users.noreply.github.com>
@petslane I am very satisfied with this PR now. Many thanks! |
@huan I think we are done here. I edited the PR description to reflect what's in the code right now. In the end it got way simpler than originally submitted. Code is complete, tests are complete. Feel free to merge or ask anything. |
@dgraziotin It's great to see this new feature has been implemented, and thank your effort for reviewing this PR to make it better @petslane Please help this PR to get at least one approvement before we have enough confidence to merge it. |
Tried to test, but I get errors on startup:
Looks like grep fails if these files are missing. |
But then again, I can see that default keys were moved to domain-specific folders and https://www.mail-tester.com/ test gets 10/10. Maybe |
Yeah, these aren't exactly errors. Mostly unwanted output. That's why I put
That was it, thanks. Was not aware of |
No warnings anymore. LGTM. |
@petslane thank your very much for the testing, reviewing, and approvement! Appreciate for all efforts from @dgraziotin and @petslane to make the SMF better, thank you very much! P.S. I have bumped the version to 1.4 , the docker image should be published shortly. |
* generating a DKIM key for all virtualDomains * including HOSTNAME in folder of domains for DKIM * KeyTable, SigningTable, TrustedHosts for HOSTNAME and all virtualDomain * Generate new DKIM data only if keys do not exist yet * disabled opendkim.conf settings for single domain, added KeyTable,SigningTable,ExternalIgnoreList,InternalHosts * Correct permissions of DKIM files regardless of prior creation * Added test for multiple domains and DKIM. Ready for huan#88 * Updated README on DKIM for multiple domains * Fixed indentation on entrypoint * Fixed wrong indentation (style) * Cleaner handling of multiple DKIM keys. No settings required. Renders huan#83 redundant * Making sure we never insert the same config twice huan#89 * Forgot one last mention of SMF_DKIM_ALL * Better tld naming for DKIM in README Co-authored-by: Peeter N <petslane@users.noreply.github.com> * DKIM test no longer changes working directory Co-authored-by: Peeter N <petslane@users.noreply.github.com> * More elegant generation of DKIM entries for HOSTNAME and virtual domains * Correct switch to suppress grep complains when files miss Co-authored-by: Peeter N <petslane@users.noreply.github.com>
…103) * Use alpine:latest as base image * Roll back to sillelien/base-alpine:0.10 (#23) * fix doc * Update README.md * Update README.md * Update Base to Alpine 3.8 * Update base image to Alpine 3.8 * Install s6 process manager directly * Upgraded BATS to 1.1.0 * Install syslog-ng for postfix logging to stdout * syslog-ng: Disable statistic messages These spam the console too much, so disable them. * Fix typo in README couse => course * Upgrade circleci from v1 to v2 * add ide config * fix circleci config * fix circleci config * fix circleci config * fix circleci config * fix yml * fix yml add docker run type * fix yml add machine run type * fix yml * year 2019 * Add voice from Paweł Czochański * EC key support (#51) * Fix nickname typo * Add support for EC keys * Update README.md * Update README.md * add ec key support * Fix layout * Timezone tzdata packagge (#57) Add custom timezone support * make circler yaml linter happy * Fix leak of EC Cert/Key problem (#58) * code clean * only generate not existing files (#51 #58) * one line -> one-line * Timezone supported * Update author & copyright * Update master changelog * fix chinese charactor bug * v1.1 * Update README.md timezone (#59) Update README.md for Timezone support * clean doc * Fix H1 title * Update README.md * Update README.md * Fix typo (#66) * add auth for relayhost (#68) * add auth for relayhost * indent fix * example for AUTH * remove excess line * environment var typo fix * fix for mail log not displaying * Enable GitHub Actions * Add Actions Badge * basic "proofreading" (#69) * basic "proofreading" fixed some grammatical and spelling errors, made the descriptions flow a little better * PR revisions * Update README.md * add hall of flame * Upgrade BATS & S6, with multiple-platform docker image published with version 1.2 (#76) * v1.2 * Upgrade Alpine to 3.8 (#77) * upgrade base image to alpine 3.8 * v1.3 * Deploy docker image arm platform from github action * test * test * checkout before deploy * clean * use buildx as default bugild * republish v1.2 for amd64 with s6 fix (#79) * republish v1.3 for amd64 with s6 fix * fix comment * v1.2 * Add test for deleting test user (#82) * Add makefile * makefile * makefile * makefile * 1.2.14 * add make version * 1.2.15 * v1.3.0 for multi platforms image * Use script to install s6 with right platforms (arm/x86) (#76) * use aarch64 for s6 release (#76) * v1.3.1 * Add DKIM support (#83) * Add DKIM support * 1.2.11 Co-authored-by: Huan (李卓桓) <zixia@zixia.net> * generating a DKIM key for all virtualDomains * including HOSTNAME in folder of domains for DKIM * KeyTable, SigningTable, TrustedHosts for HOSTNAME and all virtualDomain * Generate new DKIM data only if keys do not exist yet * disabled opendkim.conf settings for single domain, added KeyTable,SigningTable,ExternalIgnoreList,InternalHosts * Correct permissions of DKIM files regardless of prior creation * Added test for multiple domains and DKIM. Ready for #88 * Updated README on DKIM for multiple domains * Fixed indentation on entrypoint * Fixed wrong indentation (style) * Cleaner handling of multiple DKIM keys. No settings required. Renders #83 redundant * Making sure we never insert the same config twice #89 * Forgot one last mention of SMF_DKIM_ALL * Better tld naming for DKIM in README Co-authored-by: Peeter N <petslane@users.noreply.github.com> * DKIM test no longer changes working directory Co-authored-by: Peeter N <petslane@users.noreply.github.com> * More elegant generation of DKIM entries for HOSTNAME and virtual domains * Correct switch to suppress grep complains when files miss * Update VERSION * Strips sender details (IP, client, user agent) when sending (#91) * Strips sender's IP, client, and user agent headers * Bumping patch verison number * Allow for setting any Postfix variables in the config file (both main.cf and master.cf) (#93) * Strips sender's IP, client, and user agent headers * Bumping patch verison number * SMF_POSTFIXMAIN_* to set custom postfix main.cf entries * SMF_POSTFIXMASTER_* to set custom postfix master.cf entries * Using sed to handle master.cf custom variables * README.md explains env variables for custom main.cf and master.cf * Tests for custom main.cf and master.cf * Fixes #92 * Simplify docker run command with SMF_CONFIG * v1.4.3 (#94) * Add VERSION & Update README.md (#94) * fix ignore * 1.4.4 * add v1.4 changelog * clean * show version * 1.4.5 * layout * Update configuration after variables has been injected to the main configuration (#98) * Add an option to override postfix's default logging configuration * Add tests * Update README.md * push to build * fix overwriting variable * update with postfix-configuration * delete drone for pull request Co-authored-by: Tamaro Skaljic <49238587+tamaro-skaljic@users.noreply.github.com> * Add an option to override postfix's default logging configuration (#97) * Add an option to override postfix's default logging configuration * Add tests * Update README.md * v1.4.6 * Change Postfix logging configuration tests behaviour (#99) * fix default postfix logging configuration test * Change logfile path in custom postfix logging configuration test * Make postfix logging configuration tests restore the preconditions * start PostSRSd and generate Secret * start PostSRSd and generate Secret * start PostSRSd if is set * start PostSRSd if is set * only start PostSRSd if is set * only start PostSRSd if is set * Updated README.md * Updated README.md * spelling... * spelling... * Updated the if-statement for PostSRSd * Updated the if-statement for PostSRSd Co-authored-by: Martijn Rondeel <martijn@rondeel.email> Co-authored-by: Huan LI <zixia@zixia.net> Co-authored-by: Chris Blake <chrisrblake93@gmail.com> Co-authored-by: universeroc <universeroc@gmail.com> Co-authored-by: Paweł Czochański <czochanski@gmail.com> Co-authored-by: me1299 <50422731+me1299@users.noreply.github.com> Co-authored-by: David Gonzalez <davidgg666@gmail.com> Co-authored-by: Choon-Siang Lai <mycyberpet@yahoo.com> Co-authored-by: Bailey <bailey.riezebos@gmail.com> Co-authored-by: Peeter N <petslane@users.noreply.github.com> Co-authored-by: Daniel Graziotin <daniel.graziotin@iste.uni-stuttgart.de> Co-authored-by: Daniel Graziotin <daniel@ineed.coffee> Co-authored-by: Cenk Kılıç <cenk1cenk2cenk3@gmail.com> Co-authored-by: Tamaro Skaljic <49238587+tamaro-skaljic@users.noreply.github.com> Co-authored-by: Linux User <harato@alpine.members.linode.com>
Fixes #88 and merges #83.
Creates DKIM public/private keys and puts them into use.
- uses all domains defined in
$SMF_DOMAIN
and$SMF_CONFIG
(only source domains for the latter) to generate public/private keypairs.- converts
/var/db/dkim/
to a per-domain path style/var/db/dkim/domain.ext/
(if already present).- creates
KeyTable
,SigningTable
,ExternalIgnoreList
, andInternalHosts
with all relevant values.- changes /etc/opendkim/opendkim.conf to use
KeyTable
,SigningTable
,ExternalIgnoreList
, andInternalHosts
.- tests on SMF startup that all DNS entries are set correctly.