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

DKIM support (signing & verfication) #7

Closed
emersion opened this issue Jan 9, 2019 · 13 comments
Closed

DKIM support (signing & verfication) #7

emersion opened this issue Jan 9, 2019 · 13 comments
Assignees
Labels
filter Related to message processing middleware ("filters") new feature New feature.

Comments

@emersion
Copy link
Collaborator

emersion commented Jan 9, 2019

No description provided.

@emersion emersion added the new feature New feature. label Jan 9, 2019
@foxcpp
Copy link
Owner

foxcpp commented Mar 23, 2019

Defaults

Verification is enabled by default for all incoming messages.
Signing is enabled by default for all outgoing messages. Keys are generated automatically and the public key is generated. DNS zone record for the public key is written to log.

New modules

dkim_verify module implements the Filter interface and does verification of message signatures.
dkim_sign module implements the Filter interface too but signs messages.

dkim_sign configuration

pub_key, priv_key directives with a path to keys, defaults to $MADDYSTATE/dkim_public_<config_block_name> and $MADDYSTATE/dkim_private_<config_block_name>.

If both files don't exist - keys are generated (algorithm to use by default? RSA-2048?) and saved (format to use for keys? probably we want to interoperate with OpenDKIM). If only one of the files exists - we refuse to start, this is a configuration error. If both files exist - we use keys from them.

We also probably want to add params to tweak key generation (algo? params?).

dkim_verify configuration

dkim_verify { invalid accept }

Insert Authentication-Result header and accept the message even if it contains invalid DKIM signature(s).

dkim_verify { invalid reject }

Reject messages with invalid DKIM signature(s). Default?

dkim_verify { required }

Reject messages without a DKIM signature.

dkim_verify { no-bounce }

Don't send bounce messages if DKIM verification fails for any reason.

@emersion emersion added the rfc Request For Comments (ongoing discussion / research needed). label Apr 13, 2019
@foxcpp
Copy link
Owner

foxcpp commented Apr 27, 2019

Updated my post, there are some questions in bold, but in overall that's how I would design/implement it.

@emersion
Copy link
Collaborator Author

DNS zone record for the public key is written to log.

We should probably write it in a file. But that can be done as a second step.

RSA-2048?

Probably. It would be neat to use ECC, but I don't know how well this is supported.

format to use for keys?

PEM

Reject messages with invalid DKIM signature(s). Default?

Other e-mail servers quarantine. In my experience DKIM signatures still break a lot so we probably shouldn't be too harsh, at least at first.

(The DMARC policy of the sender can request to quarantine/bounce/… messages)

Don't send bounce messages if DKIM verification fails for any reason.

So the message is just lost in limbo? Or just reports are not sent?

@emersion
Copy link
Collaborator Author

DKIM also has things like selector and other parameters, we probably want to make them configurable

@foxcpp
Copy link
Owner

foxcpp commented Apr 27, 2019

Probably. It would be neat to use ECC, but I don't know how well this is supported.

AFAIK, ECC is not supported by OpenDKIM (the most widespread implementation?) so we are out of luck here.

Other e-mail servers quarantine. In my experience DKIM signatures still break a lot so we probably shouldn't be too harsh, at least at first.

(The DMARC policy of the sender can request to quarantine/bounce/… messages)

Okay, so then the following set of values for the invalid option would make more sense: ignore/quarantine/reject. DMARC policy of the sender overrides the value set in the config.

ignore - accept, but still add Authentication-Results header

quarantine - accept, but set a marker in delivery context for broken signatures, so, IMAP storage will place the message into Spam mailbox, for example.

reject - refuse to accept the message.
Note: In the current design we don't send the bounce messages themselves and instead just don't accept messages that don't pass something. I wonder if this is a valid approach, but this is out-of-scope of this issue anyway.

@foxcpp foxcpp added mta-in Related to incoming message processing part of the MTA functionality (mail exchanger). mta-out Related to MSA or outgoing message processing part of MTA functionality. filter Related to message processing middleware ("filters") and removed mta-in Related to incoming message processing part of the MTA functionality (mail exchanger). mta-out Related to MSA or outgoing message processing part of MTA functionality. labels May 25, 2019
@foxcpp foxcpp changed the title Automatic DKIM DKIM support (signing & verfication) May 27, 2019
@foxcpp foxcpp added this to the 0.1 milestone May 27, 2019
@foxcpp foxcpp removed the rfc Request For Comments (ongoing discussion / research needed). label May 27, 2019
@foxcpp foxcpp self-assigned this May 27, 2019
@foxcpp
Copy link
Owner

foxcpp commented Aug 30, 2019

  1. There will be nosig_action and brokensig_actions to override behavior on failures. Both will default to "ignore" since this is the way DKIM is meant to work by default.

  2. DMARC policy of the sender domain overrides the local policy for failure handling.

  3. Authentication-Result header field is always added. Its MUA responsibility to warn the user about invalid/missing DKIM signature.

@foxcpp foxcpp pinned this issue Aug 30, 2019
@foxcpp
Copy link
Owner

foxcpp commented Aug 31, 2019

go-msgauth/dkim buffers the whole message in memory if multiple signatures are present.
Per #76, we really want to avoid creating additional copies of message in memory.

@emersion
Copy link
Collaborator Author

We really should do the verification in parallel instead of buffering. Requires feeding data from a single io.Reader to multiple consumers in separate goroutines.

@foxcpp
Copy link
Owner

foxcpp commented Aug 31, 2019

I will take a look.

@emersion
Copy link
Collaborator Author

We can potentially use io.MultiWriter and io.Pipe for this (e.g. https://stackoverflow.com/questions/24677285/how-to-have-multiple-consumer-from-one-io-reader).

@foxcpp
Copy link
Owner

foxcpp commented Sep 15, 2019

emersion/go-msgauth#10

@foxcpp
Copy link
Owner

foxcpp commented Oct 18, 2019

Verification:

  1. Signatures with "body limit" tag are considered invalid by default.
  2. DKIM relies DNSSEC to authenticate keys, resolution on dkim: Provide a low-level Verify function for more control emersion/go-msgauth#10 is needed so we can use DNSSEC-aware resolver with go-msgauth.

Signing:

  1. Header fields should be "oversigned" by default to prevent all modifications.
  2. Header fields to sign: everything visible for the user (Subject, To, CC, From, Date), everything that affects body handling (Content-Type, Content-Transfer-Encoding, possibly more), everything that affects user interactions (Reply-To).

Src: https://noxxi.de/research/breaking-dkim-on-purpose-and-by-chance.html

@foxcpp
Copy link
Owner

foxcpp commented Oct 27, 2019

Signing is implemented in beef9e2.

@foxcpp foxcpp closed this as completed Oct 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filter Related to message processing middleware ("filters") new feature New feature.
Projects
None yet
Development

No branches or pull requests

2 participants