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

Add opendkim -O option and StdoutLog config parameter (for Docker) #160

Closed
wants to merge 1 commit into from
Closed

Conversation

rafork
Copy link

@rafork rafork commented Dec 26, 2022

Hi. This patch attempts to fix issue #153.

This patch adds the ability to log to stdout (as well as or instead of syslog(3)). People who want to use OpenDKIM in a Docker container would really appreciate this ability as it means that they don't need to install, configure, and run an rsyslogd process in the container as well.

The opendkim command gets a new -O option (like the -l option), and the configuration file gets a new StdoutLog parameter (like the Syslog parameter). The existing SyslogSuccess parameter has the same effect when logging to stdout.

This mostly affects opendkim/opendkim.c (and documentation).

There are three calls to syslog() in util.c that haven't been modified to be able to log to stdout because it would require moving the definition of struct dkimf_config (and the declaration of curconf) so that they become accessible to util.c (i.e., opendkim.h). That's a more invasive change, and it's probably not worth it. But if you prefer it, I can make the additional necessary changes.

Also, I fixed what looked like two bugs in mlfi_eom(). The two "header remove failed" error messages were being emitted when there was an error or when logging was enabled (rather than and when logging was enabled).

Warning: I couldn't find instructions for configuring and building from the git repository, so I couldn't actually test this. There might be embarrasing typos.

Copy link

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

Very nice work, Raf. Thank you. I hope someone gets to merge this eventually. Apparently the repo is dead (?). Cheers!

@glts
Copy link

glts commented Dec 28, 2022

Warning: I couldn't find instructions for configuring and building from the git repository, so I couldn't actually test this. There might be embarrasing typos.

Instructions are the same as here: https://github.com/trusteddomainproject/OpenDMARC/blob/9cebf724d601452d1a671ed5331551dbc18df83a/INSTALL#L90-L111

Use autoreconf -f -i, use make check to run tests.

edit: also pull requests should target current development on branch develop, not master

@rafork
Copy link
Author

rafork commented Dec 29, 2022

Thanks. I did see advice somewhere that autoreconf --install was needed, and that worked too. Also, I should have started with the develop branch rather than the master branch. Running configure on this breaks with:

configure: error: Cannot build shared opendkim
                                          against static openssl libraries.
                                          Configure with --disable-shared
                                          to get this working or obtain a
                                          shared libssl library for
                                          opendkim to use.

Which makes no sense. This system has shared libssl. But configure worked against the develop branch. This patch does have several typos, but when they are fixed, make check is still happy. I'll replace this pull request with one based on the develop branch. Thanks again.

@thegushi
Copy link
Collaborator

thegushi commented Jan 7, 2023

This looks solid, yes, please do refactor against develop.

@rafork rafork closed this by deleting the head repository Jan 9, 2023
@schildbach
Copy link

Despite ratork deleting his or her repository, can this feature be merged anyway?

@rafork
Copy link
Author

rafork commented Oct 6, 2023

Hi, I just worked out why I closed this pull request. This pull request was based on the master branch. It was replaced with a new pull request based on the develop branch: #166 which is still open. Rather than waiting, you might be better off applying the patch to your own copy of the source code, and creating a docker image layer for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants