Feat/enable custom dkim selector #1811

astrocket · 2021-02-16T05:50:59Z

Description

Enable generate-dkim-config to accept selector as third parameter.

./setup.sh config dkim <key-size> <domain.tld>[,<domain2.tld>] <selector>

which will apply to private key generation, key names, KeyTable, SigningTable

Fixes #1304

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (non-breaking change that does improve existing functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation (README.md or ENVIRONMENT.md or the Wiki)
If necessary I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

astrocket · 2021-02-16T06:21:35Z

Anyone has documentation for running test locally?

georglauterbach · 2021-02-16T09:06:15Z

There's this thing, you know, called CONTRIBUTING.md. I assume you've not read it...

Your addition is very welcome. But the way you've done it cannot work. You're using the selector as $3 but the domains themself are actually optional. This way arguments will mix up. This script is not perfect, I mean before your change.

@wernerfred I thought about another way of parsing the arguments. I will explain in a few minutes in this thread.

georglauterbach · 2021-02-16T09:31:32Z

From my PoV, the script should work like this:

SYNOPSIS
     ./setup.sh dkim [OPTIONS...]

DESCRIPTION
    Configures DKIM keys. OPTIONS can be used to configure a more complex setup. LDAP setups require these options.

OPTIONS
  Generic Program Information
    help       Print the usage information.

  Configuration adjustments
    size       Set the size of the keys to be generated. Possible are 1024, 2024 (default) and 4096.
    selector   Set a manual selector (default is "mail") for the key.
    domains    Provide the domains for which keys are to be generated.

This adheres to a man-page style. I will provide a PR. This is a breaking change, but with the upcoming merge of #1802 and release 9.0.0, this is fine. Furthermore, the ./setup.sh config ssl ... will be thrown out as well in either #1802 or #1801.

@astrocket I will then leave it up to you to rebase and implement the new selector feature of course :D

wernerfred · 2021-02-16T09:46:32Z

I really like your approach. When I "hotfixed" the script I had no time to do a complete refactor. Your change will require many changes to tests too. Hit me up if you need support. If you use a branch and not a fork I can contribute too (maybe then it is also time to get rid of this ugly wc -l tests).

Regarding 9.0.0 we should definitely wait until all changes are merged. We now have a lot of PRs with breaking changes upfront.

astrocket · 2021-02-16T09:49:25Z

@aendeavor Thanks a lot :) I hope I can follow up.

georglauterbach · 2021-02-16T10:20:05Z

I really like your approach. When I "hotfixed" the script I had no time to do a complete refactor. Your change will require many changes to tests too. Hit me up if you need support. If you use a branch and not a fork I can contribute too (maybe then it is also time to get rid of this ugly wc -l tests).

I will open a PR with on a branch and assign you too :D

Regarding 9.0.0 we should definitely wait until all changes are merged. We now have a lot of PRs with breaking changes upfront.

Most definitely.

@aendeavor Thanks a lot :) I hope I can follow up.

@astrocket We will get back to you once we're finished. Until then, I will mark this as a draft.

georglauterbach · 2021-02-16T12:09:12Z

@wernerfred There is the new origin/open-dkim branch. I've refactored the whole script, and outsourced tests into their own script. #1812 is open :)

georglauterbach · 2021-02-18T10:12:08Z

@astrocket #1812 was merged. You can go ahead and rebase on master (should require a force push).

Please note: Your script adjustments were fine. But the issue #1304 features a TODO list which is longer:

To do that:

Add a DKIM_SELECTOR option with mail as the default value

Modify the setup script to use the selector [DONE by me already]

Modify the startup script to patch the configuration to use the selector

Update the instructions

Write some kind of test that proves it works

georglauterbach · 2021-02-18T12:46:43Z

Just for our (the maintainers) information: How long do you think it will take you to make this PR ready for merge @astrocket ?

wernerfred · 2021-02-19T20:10:46Z

I rebased the pr branch as the modified files were all splitted in new files in the meantime.
I removed the hint in the README.md as it doesn't fit there with the new wording imho. The information needs to be added to the wiki.
@astrocket pls have a look and @ALL pls review

test/open_dkim.bats

wernerfred · 2021-02-19T21:33:58Z

Tests currently failing:

not ok 182 checking opendkim: generator creates keys, tables and TrustedHosts using manual provided selector name
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/open_dkim.bats, line 421)
#   `assert_output 4' failed
# 
# -- output differs --
# expected : 4
# actual   : 1
# --
#

EDIT:

actually the tests checked the existence of 4 domains but only one is added in test setup. Removed the 3 obsolete domains

diff --git a/test/open_dkim.bats b/test/open_dkim.bats
index 44b1526d..f05b8d0f 100644
--- a/test/open_dkim.bats
+++ b/test/open_dkim.bats
@@ -416,15 +416,15 @@ function teardown_file
   run docker run --rm \
     -v "${PRIVATE_CONFIG}/with-selector/opendkim":/etc/opendkim \
     "${IMAGE_NAME:?}" /bin/sh -c \
-    "egrep 'domain1.tld|domain2.tld|domain3.tld|domain4.tld' /etc/opendkim/KeyTable | wc -l"
+    "grep 'domain1.tld' /etc/opendkim/KeyTable | wc -l"
   assert_success
-  assert_output 4
+  assert_output 1
   
   # Check valid entries actually present in SigningTable
   run docker run --rm \
     -v "${PRIVATE_CONFIG}/with-selector/opendkim":/etc/opendkim \
     "${IMAGE_NAME:?}" /bin/sh -c \
-    "egrep 'domain1.tld|domain2.tld|domain3.tld|domain4.tld' /etc/opendkim/SigningTable | wc -l"
+    "grep 'domain1.tld' /etc/opendkim/SigningTable | wc -l"
   assert_success
-  assert_output 4
+  assert_output 1
 }

georglauterbach · 2021-02-19T22:44:02Z

LGTM 👍🏼

Ready for merge I guess :D

wernerfred · 2021-02-19T23:03:26Z

Please note: Your script adjustments were fine. But the issue #1304 features a TODO list which is longer:

To do that:

Add a DKIM_SELECTOR option with mail as the default value

Modify the setup script to use the selector [DONE by me already]

Modify the startup script to patch the configuration to use the selector

Update the instructions

Write some kind of test that proves it works

1, 2, 4 and 5 ist done. What needs to be done with 3?

georglauterbach · 2021-02-20T08:41:01Z

I guess maybe

docker-mailserver/target/scripts/start-mailserver.sh

Lines 1105 to 1135 in 9efa94c

    
           function _setup_dkim 
        
           { 
        
             _notify 'task' 'Setting up DKIM' 
        
             mkdir -p /etc/opendkim && touch /etc/opendkim/SigningTable 
        
             # Check if keys are already available 
        
             if [[ -e "/tmp/docker-mailserver/opendkim/KeyTable" ]] 
        
             then 
        
               cp -a /tmp/docker-mailserver/opendkim/* /etc/opendkim/ 
        
               _notify 'inf' "DKIM keys added for: $(ls -C /etc/opendkim/keys/)" 
        
               _notify 'inf' "Changing permissions on /etc/opendkim" 
        
               chown -R opendkim:opendkim /etc/opendkim/ 
        
               chmod -R 0700 /etc/opendkim/keys/ # make sure permissions are right 
        
             else 
        
               _notify 'warn' "No DKIM key provided. Check the documentation to find how to get your keys." 
        
               local KEYTABLE_FILE="/etc/opendkim/KeyTable" 
        
               [[ ! -f ${KEYTABLE_FILE} ]] && touch "${KEYTABLE_FILE}" 
        
             fi 
        
             # setup nameservers paramater from /etc/resolv.conf if not defined 
        
             if ! grep '^Nameservers' /etc/opendkim.conf 
        
             then 
        
               echo "Nameservers $(grep '^nameserver' /etc/resolv.conf | awk -F " " '{print $2}' | paste -sd ',' -)" >> /etc/opendkim.conf 
        
               _notify 'inf' "Nameservers added to /etc/opendkim.conf" 
        
             fi 
        
           }

needs adjustments? I can't see any, but that doesn't mean they don't exist...

PS: Although this function could also use some love, and could be shortened down a bit.

wernerfred · 2021-02-21T14:32:02Z

@astrocket did you test my changes again?

wernerfred · 2021-02-21T14:51:21Z

What do you think of changing this:

docker-mailserver/target/scripts/start-mailserver.sh

Lines 1105 to 1135 in 9efa94c

    
           function _setup_dkim 
        
           { 
        
             _notify 'task' 'Setting up DKIM' 
        
             mkdir -p /etc/opendkim && touch /etc/opendkim/SigningTable 
        
             # Check if keys are already available 
        
             if [[ -e "/tmp/docker-mailserver/opendkim/KeyTable" ]] 
        
             then 
        
               cp -a /tmp/docker-mailserver/opendkim/* /etc/opendkim/ 
        
               _notify 'inf' "DKIM keys added for: $(ls -C /etc/opendkim/keys/)" 
        
               _notify 'inf' "Changing permissions on /etc/opendkim" 
        
               chown -R opendkim:opendkim /etc/opendkim/ 
        
               chmod -R 0700 /etc/opendkim/keys/ # make sure permissions are right 
        
             else 
        
               _notify 'warn' "No DKIM key provided. Check the documentation to find how to get your keys." 
        
               local KEYTABLE_FILE="/etc/opendkim/KeyTable" 
        
               [[ ! -f ${KEYTABLE_FILE} ]] && touch "${KEYTABLE_FILE}" 
        
             fi 
        
             # setup nameservers paramater from /etc/resolv.conf if not defined 
        
             if ! grep '^Nameservers' /etc/opendkim.conf 
        
             then 
        
               echo "Nameservers $(grep '^nameserver' /etc/resolv.conf | awk -F " " '{print $2}' | paste -sd ',' -)" >> /etc/opendkim.conf 
        
               _notify 'inf' "Nameservers added to /etc/opendkim.conf" 
        
             fi 
        
           }

to this:

function _setup_dkim
{
  _notify 'task' 'Setting up DKIM'

  mkdir -p /etc/opendkim

  # Check if any keys are available
  if [[ -e "/tmp/docker-mailserver/opendkim/KeyTable" ]]
  then
    cp -a /tmp/docker-mailserver/opendkim/* /etc/opendkim/

    _notify 'inf' "DKIM keys added for: $(ls -C /etc/opendkim/keys/)"
    _notify 'inf' "Changing permissions on /etc/opendkim"

    chown -R opendkim:opendkim /etc/opendkim/
    chmod -R 0700 /etc/opendkim/keys/ 
  else
    _notify 'warn' "No DKIM key provided. Check the documentation on how to get your keys."
  fi

  # setup nameservers paramater from /etc/resolv.conf if not defined
  if ! grep '^Nameservers' /etc/opendkim.conf
  then
    echo "Nameservers $(grep '^nameserver' /etc/resolv.conf | awk -F " " '{print $2}' | paste -sd ',' -)" >> /etc/opendkim.conf

    _notify 'inf' "Nameservers added to /etc/opendkim.conf"
  fi
}

Imho no need to:

touch /etc/opendkim/SigningTable as SigningTable gets copied later
[[ ! -f ${KEYTABLE_FILE} ]] && touch "${KEYTABLE_FILE}" in case of no provided keys

If nothing speaks against this change I will commit and merge

georglauterbach · 2021-02-21T17:49:12Z

If these changes work, go ahead :D

georglauterbach requested review from georglauterbach and wernerfred February 16, 2021 10:20

georglauterbach assigned astrocket Feb 16, 2021

georglauterbach added area/features area/scripts kind/new feature A new feature is requested in this issue or implemeted with this PR meta/wip priority/medium labels Feb 16, 2021

georglauterbach marked this pull request as draft February 16, 2021 10:20

georglauterbach marked this pull request as ready for review February 18, 2021 10:12

astrocket added 2 commits February 19, 2021 20:51

let dkim generator accept selector as parameter

2d2f549

test dkim-generator with selector parameter

7815119

wernerfred force-pushed the feat/enable-custom-dkim-selector branch from 18bc2ac to 7815119 Compare February 19, 2021 20:08

wernerfred added 2 commits February 19, 2021 21:17

fix: correct name of domain argument in usage

62120ff

fix: adapt command to new syntax

9aedf0c

wernerfred reviewed Feb 19, 2021

View reviewed changes

test/open_dkim.bats Outdated Show resolved Hide resolved

wernerfred added 2 commits February 19, 2021 21:52

tests: use different quotes

aca7794

tests: use different quotes

b5c1630

wernerfred added 2 commits February 19, 2021 22:44

tests: remove domains that were never added

b6d3053

style: change test name

dc6be4a

wernerfred removed the meta/wip label Feb 19, 2021

georglauterbach approved these changes Feb 19, 2021

View reviewed changes

georglauterbach removed the pr/needs review label Feb 19, 2021

wernerfred added 3 commits February 21, 2021 20:44

refactor: dkim setup

88563e1

style: remove trailing whitespace

8df6460

tests: remove test of removed dummy file

5764c50

wernerfred merged commit a7ecb0e into docker-mailserver:master Feb 21, 2021

This was referenced Mar 7, 2023

tests(refactor): open_dkim.bats #3060

Merged

Support custom DKIM selector #1304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/enable custom dkim selector #1811

Feat/enable custom dkim selector #1811

astrocket commented Feb 16, 2021 •

edited by wernerfred

astrocket commented Feb 16, 2021

georglauterbach commented Feb 16, 2021

georglauterbach commented Feb 16, 2021 •

edited

wernerfred commented Feb 16, 2021

astrocket commented Feb 16, 2021

georglauterbach commented Feb 16, 2021

georglauterbach commented Feb 16, 2021

georglauterbach commented Feb 18, 2021

georglauterbach commented Feb 18, 2021

wernerfred commented Feb 19, 2021 •

edited

wernerfred commented Feb 19, 2021 •

edited

georglauterbach commented Feb 19, 2021 •

edited

wernerfred commented Feb 19, 2021

georglauterbach commented Feb 20, 2021 •

edited

wernerfred commented Feb 21, 2021

wernerfred commented Feb 21, 2021 •

edited

georglauterbach commented Feb 21, 2021

Feat/enable custom dkim selector #1811

Feat/enable custom dkim selector #1811

Conversation

astrocket commented Feb 16, 2021 • edited by wernerfred

Description

Type of change

Checklist:

astrocket commented Feb 16, 2021

georglauterbach commented Feb 16, 2021

georglauterbach commented Feb 16, 2021 • edited

wernerfred commented Feb 16, 2021

astrocket commented Feb 16, 2021

georglauterbach commented Feb 16, 2021

georglauterbach commented Feb 16, 2021

georglauterbach commented Feb 18, 2021

georglauterbach commented Feb 18, 2021

wernerfred commented Feb 19, 2021 • edited

wernerfred commented Feb 19, 2021 • edited

georglauterbach commented Feb 19, 2021 • edited

wernerfred commented Feb 19, 2021

georglauterbach commented Feb 20, 2021 • edited

wernerfred commented Feb 21, 2021

wernerfred commented Feb 21, 2021 • edited

georglauterbach commented Feb 21, 2021

astrocket commented Feb 16, 2021 •

edited by wernerfred

georglauterbach commented Feb 16, 2021 •

edited

wernerfred commented Feb 19, 2021 •

edited

wernerfred commented Feb 19, 2021 •

edited

georglauterbach commented Feb 19, 2021 •

edited

georglauterbach commented Feb 20, 2021 •

edited

wernerfred commented Feb 21, 2021 •

edited