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 verification assumes message receipt time is *now* #63

Open
oogali opened this issue May 8, 2024 · 8 comments
Open

DKIM verification assumes message receipt time is *now* #63

oogali opened this issue May 8, 2024 · 8 comments

Comments

@oogali
Copy link

oogali commented May 8, 2024

I use go-msgauth/dkim to verify signatures in my internal MDA.

In my development environment, I had an issue where mail was backed up for a few days. Once I fixed the issue and resumed queue processing at the upstream MTA, I began receiving a number of DKIM signature expiration failures.

It appears that the timestamp for verification is always the time at which the message was presented to the library.
https://github.com/emersion/go-msgauth/blob/master/dkim/verify.go#L268

I think it would be beneficial for both testing and delayed delivery if the caller could optionally present a timestamp to perform verification against rather than the current time.

@oogali oogali changed the title DKIM verification assumes receipt time is _now_ DKIM verification assumes message receipt time is *now* May 8, 2024
@oogali
Copy link
Author

oogali commented May 8, 2024

I understand it's a bit of a quandary since the published DNS records that are used for verification may or may not be different from the time that the message was sent, versus the time go-msgauth performs the query.

But I don't think failing (or in an extreme instance, skipping DKIM verification altogether) on MTA-delayed emails is the right behavior.

@emersion
Copy link
Owner

emersion commented May 8, 2024

In general when signing it's advisable to set the expiration to at least a few days to accommodate for cases like these.

I'm a bit wary about exposing security-sensitive details like this, it could easily be misused e.g. by passing the Date header field.

@oogali
Copy link
Author

oogali commented May 8, 2024

I understand that position.

If it helps shed more light on the situation, the email in question came from a newsletter delivered by Mailgun.

My guess is they're rotating keys on a daily basis because the DKIM verification result showed a 24-hour expiry.

In my MDA, I receive the message via LMTP, then queue it for processing with an out-of-band timestamp recorded at the moment of LMTP ingest.

I'm not requesting that you parse the Date header, but perhaps allow me as a library caller to perform some action along the lines of this crude example:

verifications, err := dkim.Verify(data, &dkim.VerifyOpts{
    Timestamp: receivedAt,
})

@emersion
Copy link
Owner

emersion commented May 8, 2024

Yeah, in a similar way that the stdlib does it: https://pkg.go.dev/crypto/x509#VerifyOptions

I'm worried about library users misunderstanding how to set this option, and passing a user-controlled field (Date) instead of the time when the message hit a trusted SMTP server. I suppose some good docs could help here.

Good to know that Mailgun rotates keys aggressively! I thought you were generating the signature.

@oogali
Copy link
Author

oogali commented May 8, 2024

I agree that this could be a potential footgun and I'm not sure you can completely avoid it, but only hope to mitigate it with a combination of documentation and a strongly worded struct member name?

TimestampEmailWasActuallyReceivedAt or something equally descriptive but less absurd?

@emersion
Copy link
Owner

emersion commented May 9, 2024

What about this?

// Time that the message was first received at the administrative domain of the
// verifier. Note, this must not be set to a user-controlled value. If zero,
// the current time is used.
VerifiedReceiptTime time.Time

@AGWA
Copy link
Contributor

AGWA commented May 9, 2024

// Time that the message was first received at the administrative domain of the
// verifier. Note, this must not be set to a user-controlled value. If zero,
// the current time is used.

I think "user" is ambiguous as it could also refer to the user of the library. How about:

// Time that the message was first received at the administrative domain of the
// verifier. Note, this value must not come from an untrusted source, such as
// the sender of the email. If zero, the current time is used.

@emersion
Copy link
Owner

emersion commented May 9, 2024

Good point!

@emersion emersion added the dkim label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants