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

modify.dkim.require_sender_match doesn't do anything in practice #465

Closed
drdaeman opened this issue Mar 9, 2022 · 2 comments
Closed

modify.dkim.require_sender_match doesn't do anything in practice #465

drdaeman opened this issue Mar 9, 2022 · 2 comments
Assignees
Labels
bug Something isn't working. ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele
Milestone

Comments

@drdaeman
Copy link

drdaeman commented Mar 9, 2022

My apologies for not following the bug report template. This is a bug I've spotted by reading the source code, not running Maddy in production - I'm trying to replace an ancient Postfix server and still figuring out the configuration I need, using source as a supplemental documentation.

I believe modify.dkim's require_sender_match setting is not doing anything. The setting is recognized, but all it does is creation of empty struct{}{}s in Modifier.senderMatch, with this field not referenced anywhere else. To best of my awareness, it's simply not used. Unit tests only ever use "off" as a value.

Before 9915c8a there used to be a shouldSign function that did some checks, but those checks were lost during the overhaul.

The dysfunctionality is probably harmless, as it's a responsibility of check.authorize_sender module to perform the proper authorization checks. Nonetheless, to avoid confusion and possible false expectations I would suggest entirely removing senderMatch struct field and adding a depreciation warning upon seeing require_sender_match, saying that this setting is non-functional.

Thank you!

@drdaeman drdaeman added the bug Something isn't working. label Mar 9, 2022
@drdaeman drdaeman changed the title Bug report modify.dkim.require_sender_match doesn't do anything in practice Mar 9, 2022
@foxcpp
Copy link
Owner

foxcpp commented Jun 18, 2022

You are right - check.authorize_sender replaced require_sender_match and should have been removed back then.

@foxcpp foxcpp self-assigned this Jun 18, 2022
@foxcpp foxcpp added this to the 0.6 milestone Jun 18, 2022
@foxcpp foxcpp added the ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele label Jun 18, 2022
@foxcpp foxcpp closed this as completed Jun 23, 2022
shift pushed a commit to shift/maddy that referenced this issue Jul 5, 2022
@cuu508
Copy link
Contributor

cuu508 commented May 23, 2023

require_sender_match is still mentioned in docs here: https://maddy.email/reference/modifiers/dkim/

Additionally, for each messages From header is checked to match MAIL FROM and authorization identity (username sender is logged in as). This can be controlled using require_sender_match directive.

Syntax: sign_subdomains boolean
Default: no
Sign emails from subdomains using a top domain key.
Allows only one domain to be specified (can be workarounded using modify.dkim multiple times).

It can make a PR which removes these mentions, but it would be good to also add a note that require_sender_match used to exist but was replaced with check.authorize_sender at version x.y.z. I'm not sure how to word that properly – I'm just starting to explore maddy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. ready-for-release Feature is implemented and available for testing in dev branch. It will be included in the next rele
Projects
None yet
Development

No branches or pull requests

3 participants