Skip to content
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

Document DKIM support and fix permissions bug #27

Closed
wants to merge 1 commit into from

Conversation

yanokwa
Copy link
Contributor

@yanokwa yanokwa commented Sep 16, 2023

Closes #26.

I've confirmed that the instructions and bash if block works on my container, but I have not tested a fresh end-to-end configuration.

@yanokwa yanokwa force-pushed the add-dkim branch 2 times, most recently from d5266ed to c661e86 Compare September 16, 2023 13:37
@yanokwa yanokwa marked this pull request as ready for review September 16, 2023 13:38
@tlex
Copy link
Member

tlex commented Sep 16, 2023

Thank you for the pull request.

I particularly appreciate the enrichment of the documentation.

I would prefer however, instead of changing permissions on the fly, to have the file directly with the right permissions.

For this to work, the file should have group id 101 and permissions 640 (assuming userns is disabled):

root@4230594df42e:/# id Debian-exim
uid=101(Debian-exim) gid=101(Debian-exim) groups=101(Debian-exim)

My suggestion, therefore, is to add two extra steps to the documentation (after generating the keys):

chown :101 rsa.private
chmod 640 rsa.private

@yanokwa
Copy link
Contributor Author

yanokwa commented Sep 16, 2023

Thanks for the fast review and for maintaining this useful container!

I had considered the approach you suggested and decided against it because openssl writes rsa.private with 600 (as recommended at http://linuxcommand.org/lc3_man_pages/ssh1.html). Given that, weakening the permissions outside the container seems unnecessarily less secure, no?

@tlex
Copy link
Member

tlex commented Sep 17, 2023

Hi again,

Thinking about this:

Given that, weakening the permissions outside the container seems unnecessarily less secure, no?

The approach with the bind mount will actually change the permissions of the file on disk, if implemented like this.

I'll make a suggestion to the code, to handle this differently. Basically, if a certain file exists at start, to copy it with the right permissions inside the container.

README.md Outdated
image: "ixdotai/smtp"
volumes:
- ./config/ixdotai-smtp/config:/etc/exim4/_docker_additional_macros:ro
- ./config/ixdotai-smtp/rsa.private:/etc/exim4/domain.key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- ./config/ixdotai-smtp/rsa.private:/etc/exim4/domain.key
- ./config/ixdotai-smtp/rsa.private:/etc/exim4/private.key

entrypoint.sh Outdated
Comment on lines 115 to 118
if [ -f /etc/exim4/domain.key ]; then
chgrp Debian-exim /etc/exim4/domain.key
chmod 640 /etc/exim4/domain.key
fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if [ -f /etc/exim4/domain.key ]; then
chgrp Debian-exim /etc/exim4/domain.key
chmod 640 /etc/exim4/domain.key
fi
if [ -f /etc/exim4/private.key ]; then
cp /etc/exim4/private.key /etc/exim4/domain.key
chown :101 /etc/exim4/domain.key
chmod 640 /etc/exim4/domain.key
fi

entrypoint.sh Show resolved Hide resolved
entrypoint.sh Show resolved Hide resolved
@yanokwa
Copy link
Contributor Author

yanokwa commented Sep 17, 2023

I've force pushed some further changes to make it easier for users to implement. Just one mount and one ENV variable.

I don't love appending .temp to the name, but it at least gives users a sense that the file isn't used as is.

@tlex tlex self-assigned this Sep 17, 2023
@tlex tlex added the enhancement New feature or request label Sep 17, 2023
@tlex
Copy link
Member

tlex commented Sep 17, 2023

Thank you again, I'm quite happy with the solution.

@tlex
Copy link
Member

tlex commented Sep 17, 2023

Released in v0.5.0

@tlex tlex closed this Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document DKIM_KEY support, fix DKIM permission bug
2 participants