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: Provide a low-level Verify function for more control #10

Closed
foxcpp opened this issue Sep 15, 2019 · 2 comments
Closed

dkim: Provide a low-level Verify function for more control #10

foxcpp opened this issue Sep 15, 2019 · 2 comments

Comments

@foxcpp
Copy link
Contributor

foxcpp commented Sep 15, 2019

Use-case

I'm thinking of ways to make maddys DKIM support work well and it needs quite a lot of control over verification process for that. Here is my experience report so far:

  • maddy uses replaceable DNS resolver implementations and also uses context.Context for cancellation of checks code running in parallel.

Non-cancelable network I/O done by dkim.Verify makes latter less useful. Also, it is done not using
custom DNS resolver implementation provided by maddy.

  • maddy operates on already parsed header.

Currently I have to serialize header to bytes.Buffer and then pass it to dkim.Verify.
Allowing to pass already parsed header would be nice.

It is not that important, as I can keep non-parsed header blob around and pass it to dkim.Verify, thus eliminating one part of the problem and hoping other is not that significant.

Proposed solution(s)

  1. Expose low-level functionality
    Expose internal structures and separate operations such as: Parse DKIM key record, parse DKIM signature header, verify the signature using provided header subset and body reader.

With that I would simply pass DKIM-Signature fields to dkim.ParseField, then fetch keys using replaceable DNS resolver with context.Context support, let dkim.ParseKey parse them and then finally pass everything to the dkim.VerifySig to do the actual verification.

Even more: I can take the subset of fields required for signature and pass it to dkim.VerifySig instead of resorting to serialize-then-parse hack. That could be complicated though, so I guess being able to simply pass go-message/textproto.Header is better..

This way I even will be able to distinguish different failure types and act accordingly (e.g. increase the "quarantine score" for messages with broken signatures, immediately quarantine messages with a signature without a corresponding key in DNS, but only if this is not caused by a temporary resolution error).

... or ...

  1. Add support for custom resolver (Allow caching DNS lookups #4), context.Context, possibly address "already parsed header" case or leave it as is.

Tell me what you think. I am willing to help with the implementation of whatever solution you think is better.

foxcpp added a commit to foxcpp/maddy that referenced this issue Sep 18, 2019
See emersion/go-msgauth#10
for improvements that *should be* made to this initial implementation.
@foxcpp
Copy link
Contributor Author

foxcpp commented Oct 27, 2019

type VerifierOptions struct {
  // if nil - net.LookupTXT is used, AD flag is used to indicate DNSSEC status which is then included into Verification struct.
  LookupTXT func(domain string) (ad bool, txt []string, err error)
}

@foxcpp
Copy link
Contributor Author

foxcpp commented Apr 29, 2020

Closing, not worth the effort.

@foxcpp foxcpp closed this as completed Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant