[__opendkim*] FreeBSD support and minor fixes #18

Closed

evilham wants to merge 0 commits from opendkim-freebsd into master

evilham commented

2022-03-10 19:04:46 +00:00

Collaborator

While adding FreeBSD support to the type I noticed various issues:

We were making sure that the KeyTable and SigningTable were created in
__opendkim_genkey, but that was being done with the default cdist permissions
(0400) which could result in issues when reloading the service after privilege
drop.
This is addressed by checking that it exists/creating it in __opendkim (just
once, not once per __opendkim_genkey call) with laxer permissions (0444).
In __opendkim, the service was being started after the config file was
installed. This is insufficient as OpenDKIM will refuse to start with the
generated config if either SigningTable or KeyTable do not exist yet.
__opendkim_genkey had the implicit assumption that the --directory parameter
always ended in a slash. This was not documented and error-prone; we are now
a bit laxer and add the trailing slash if it is missing.
__opendkim_genkey was not changing permissions for the resulting .txt file.
This was not critical for it to function, but it was inconsistent.
As documented in #17, __opendkim allows for a --userid parameter that might
cause issues with keys generated by __opendkim_genkey.
This issue has not been addressed yet, but I recommend deprecating the
--userid parameter.

While adding FreeBSD support to the type I noticed various issues: - We were making sure that the KeyTable and SigningTable were created in __opendkim_genkey, but that was being done with the default cdist permissions (0400) which could result in issues when reloading the service after privilege drop. This is addressed by checking that it exists/creating it in __opendkim (just once, not once per __opendkim_genkey call) with laxer permissions (0444). - In __opendkim, the service was being started after the config file was installed. This is insufficient as OpenDKIM will refuse to start with the generated config if either SigningTable or KeyTable do not exist yet. - __opendkim_genkey had the implicit assumption that the --directory parameter always ended in a slash. This was not documented and error-prone; we are now a bit laxer and add the trailing slash if it is missing. - __opendkim_genkey was not changing permissions for the resulting .txt file. This was not critical for it to function, but it was inconsistent. - As documented in #17, __opendkim allows for a --userid parameter that might cause issues with keys generated by __opendkim_genkey. This issue has not been addressed yet, but I recommend deprecating the --userid parameter.

evilham added 1 commit 2022-03-10 19:04:46 +00:00

[__opendkim*] FreeBSD support and minor fixes 1fc8a7e547

While adding FreeBSD support to the type I noticed various issues:

- We were making sure that the KeyTable and SigningTable were created in
  __opendkim_genkey, but that was being done with the default cdist permissions
  (0400) which could result in issues when reloading the service after privilege
  drop.
  This is addressed by checking that it exists/creating it in __opendkim (just
  once, not once per __opendkim_genkey call) with laxer permissions (0444).
- In __opendkim, the service was being started after the config file was
  installed. This is insufficient as OpenDKIM will refuse to start with the
  generated config if either SigningTable or KeyTable do not exist yet.
- __opendkim_genkey had the implicit assumption that the --directory parameter
  always ended in a slash. This was not documented and error-prone; we are now
  a bit laxer and add the trailing slash if it is missing.
- __opendkim_genkey was not changing permissions for the resulting .txt file.
  This was not critical for it to function, but it was inconsistent.
- As documented in #17, __opendkim allows for a --userid parameter that might
  cause issues with keys generated by __opendkim_genkey.
  This issue has not been addressed yet, but I recommend deprecating the
  --userid parameter.

evilham commented

2022-03-10 19:05:58 +00:00

Author

Collaborator

cc @sparrowhawk

evilham force-pushed opendkim-freebsd from 1fc8a7e547 to ecd10de2d3

2022-03-10 19:10:00 +00:00

Compare

evilham commented

2022-03-10 20:41:49 +00:00

Author

Collaborator

Ugh, apparently I'm very tired and effed up my git foo, so this was pushed.

In prder to avoid force-pushing and all the dance, I promise pretty please to respond quickly to feedback and integrate it back to the branch.

Sorry about the mishap >,< leaving this open so we can do proper review even if after the fact in this case.

Ugh, apparently I'm very tired and effed up my git foo, so this was pushed. In prder to avoid force-pushing and all the dance, I promise pretty please to respond quickly to feedback and integrate it back to the branch. Sorry about the mishap >,< leaving this open so we can do proper review even if after the fact in this case.

sparrowhawk commented

2022-03-15 14:22:16 +00:00

Collaborator

Huh, I was wondering why there was a PR with the same content as a git commit I'd read with interest x)

I'm happy as it is, so it's fine that you merged it. As a personal preference, I tend to be paranoid with shell variables and mark everything that shouldn't be empty with an ${:?} expansion, but that's just me. We're all good :)

Huh, I was wondering why there was a PR with the same content as a git commit I'd read with interest x) I'm happy as it is, so it's fine that you merged it. As a personal preference, I tend to be paranoid with shell variables and mark everything that shouldn't be empty with an `${:?}` expansion, but that's just me. We're all good :)

🎉 1