search

jstedfast / MimeKit

A .NET MIME creation and parser library with support for S/MIME, PGP, DKIM, TNEF and Unix mbox spools.
http://www.mimekit.net
MIT License
1.84k stars 372 forks source link

Support for ARC signatures #386

Closed rbfajardo closed 5 years ago

rbfajardo commented 6 years ago

Verifying DKIM signatures against forwarded messages will fail. The forwarder (receiver of the original email) will have verified the DKIM signature and ideally re-signed the message with an ARC signature.

Feature request for MimeKit: Verifying ARC signatures

Info: http://arc-spec.org/ RFC: https://tools.ietf.org/html/draft-andersen-arc-00 (Not sure if this is the latest one)

jstedfast commented 5 years ago

They seem to be up to draft 21 now: https://tools.ietf.org/id/draft-ietf-dmarc-arc-protocol-21.txt

The-Nutty commented 5 years ago

@jstedfast Would a good and easy to implement first step be to modify the Verify/VerifyAsync to accept a ARC-Message-Signature as it can be verified in the same way a Dkim-Signature header can be verified.

From the RFC

The AMS header field has the same syntax and semantics as the DKIM-Signature field [RFC6376], with three (3) differences:

the name of the header field itself; no version tag (“v”) is defined for the AMS header field. As required for undefined tags (in [RFC6376]), if seen, a version tag MUST be ignored; the “i” (AUID) tag is not imported from DKIM; instead, this tag is replaced by the “instance tag” as defined in Section 4.2.1;

jstedfast commented 5 years ago

I suppose so...

diff --git a/MimeKit/MimeMessage.cs b/MimeKit/MimeMessage.cs
index 63361162..a1b0a1a3 100644
--- a/MimeKit/MimeMessage.cs
+++ b/MimeKit/MimeMessage.cs
@@ -29,6 +29,7 @@ using System.IO;
 using System.Text;
 using System.Linq;
 using System.Threading;
+using System.Globalization;
 using System.Threading.Tasks;
 using System.Collections.Generic;

@@ -2017,73 +2018,82 @@ namespace MimeKit {
            return parameters;
        }

-       static void ValidateDkimSignatureParameters (IDictionary<string, string> parameters, out DkimSignatureAlgorithm algorithm, out DkimCanonicalizationAlgorithm headerAlgorithm,
+       static void ValidateSignatureParameters (IDictionary<string, string> parameters, HeaderId header, out DkimSignatureAlgorithm algorithm, out DkimCanonicalizationAlgorithm headerAlgorithm,
            out DkimCanonicalizationAlgorithm bodyAlgorithm, out string d, out string s, out string q, out string[] headers, out string bh, out string b, out int maxLength)
        {
            bool containsFrom = false;
-           string v, a, c, h, l, id;

-           if (!parameters.TryGetValue ("v", out v))
-               throw new FormatException ("Malformed DKIM-Signature header: no version parameter detected.");
+           if (header == HeaderId.DkimSignature) {
+               if (!parameters.TryGetValue ("v", out string v))
+                   throw new FormatException ("Malformed DKIM-Signature header: no version parameter detected.");

-           if (v != "1")
-               throw new FormatException (string.Format ("Unrecognized DKIM-Signature version: v={0}", v));
+               if (v != "1")
+                   throw new FormatException (string.Format ("Unrecognized DKIM-Signature version: v={0}", v));
+           }

-           if (!parameters.TryGetValue ("a", out a))
-               throw new FormatException ("Malformed DKIM-Signature header: no signature algorithm parameter detected.");
+           if (!parameters.TryGetValue ("a", out string a))
+               throw new FormatException (string.Format ("Malformed {0} header: no signature algorithm parameter detected.", header.ToHeaderName ()));

            switch (a.ToLowerInvariant ()) {
            case "rsa-sha256": algorithm = DkimSignatureAlgorithm.RsaSha256; break;
            case "rsa-sha1": algorithm = DkimSignatureAlgorithm.RsaSha1; break;
-           default: throw new FormatException (string.Format ("Unrecognized DKIM-Signature algorithm parameter: a={0}", a));
+           default: throw new FormatException (string.Format ("Unrecognized {0} algorithm parameter: a={1}", header.ToHeaderName (), a));
            }

            if (!parameters.TryGetValue ("d", out d))
-               throw new FormatException ("Malformed DKIM-Signature header: no domain parameter detected.");
+               throw new FormatException (string.Format ("Malformed {0} header: no domain parameter detected.", header.ToHeaderName ()));

-           if (parameters.TryGetValue ("i", out id)) {
-               string ident;
-               int at;
+           if (parameters.TryGetValue ("i", out string id)) {
+               switch (header) {
+               case HeaderId.DkimSignature:
+                   string ident;
+                   int at;

-               if ((at = id.LastIndexOf ('@')) == -1)
-                   throw new FormatException ("Malformed DKIM-Signature header: no @ in the AUID value.");
+                   if ((at = id.LastIndexOf ('@')) == -1)
+                       throw new FormatException ("Malformed DKIM-Signature header: no @ in the AUID value.");

-               ident = id.Substring (at + 1);
+                   ident = id.Substring (at + 1);

-               if (!ident.Equals (d, StringComparison.OrdinalIgnoreCase) && !ident.EndsWith ("." + d, StringComparison.OrdinalIgnoreCase))
-                   throw new FormatException ("Invalid DKIM-Signature header: the domain in the AUID does not match the domain parameter.");
+                   if (!ident.Equals (d, StringComparison.OrdinalIgnoreCase) && !ident.EndsWith ("." + d, StringComparison.OrdinalIgnoreCase))
+                       throw new FormatException ("Invalid DKIM-Signature header: the domain in the AUID does not match the domain parameter.");
+                   break;
+               case HeaderId.ArcMessageSignature:
+                   if (!int.TryParse (id, NumberStyles.Integer, CultureInfo.InvariantCulture, out int i) || i < 1 || i > 50)
+                       throw new FormatException ("Malformed ARC-Message-Signature header: invalid instance value.");
+                   break;
+               }
            }

            if (!parameters.TryGetValue ("s", out s))
-               throw new FormatException ("Malformed DKIM-Signature header: no selector parameter detected.");
+               throw new FormatException (string.Format ("Malformed {0} header: no selector parameter detected.", header.ToHeaderName ()));

            if (!parameters.TryGetValue ("q", out q))
                q = "dns/txt";

-           if (parameters.TryGetValue ("l", out l)) {
+           if (parameters.TryGetValue ("l", out string l)) {
                if (!int.TryParse (l, out maxLength))
-                   throw new FormatException (string.Format ("Malformed DKIM-Signature header: invalid length parameter: l={0}", l));
+                   throw new FormatException (string.Format ("Malformed {0} header: invalid length parameter: l={1}", header.ToHeaderName (), l));
            } else {
                maxLength = -1;
            }

-           if (parameters.TryGetValue ("c", out c)) {
+           if (parameters.TryGetValue ("c", out string c)) {
                var tokens = c.ToLowerInvariant ().Split ('/');

                if (tokens.Length == 0 || tokens.Length > 2)
-                   throw new FormatException (string.Format ("Malformed DKIM-Signature header: invalid canonicalization parameter: c={0}", c));
+                   throw new FormatException (string.Format ("Malformed {0} header: invalid canonicalization parameter: c={1}", header.ToHeaderName (), c));

                switch (tokens[0]) {
                case "relaxed": headerAlgorithm = DkimCanonicalizationAlgorithm.Relaxed; break;
                case "simple": headerAlgorithm = DkimCanonicalizationAlgorithm.Simple; break;
-               default: throw new FormatException (string.Format ("Malformed DKIM-Signature header: invalid canonicalization parameter: c={0}", c));
+               default: throw new FormatException (string.Format ("Malformed {0} header: invalid canonicalization parameter: c={1}", header.ToHeaderName (), c));
                }

                if (tokens.Length == 2) {
                    switch (tokens[1]) {
                    case "relaxed": bodyAlgorithm = DkimCanonicalizationAlgorithm.Relaxed; break;
                    case "simple": bodyAlgorithm = DkimCanonicalizationAlgorithm.Simple; break;
-                   default: throw new FormatException (string.Format ("Malformed DKIM-Signature header: invalid canonicalization parameter: c={0}", c));
+                   default: throw new FormatException (string.Format ("Malformed {0} header: invalid canonicalization parameter: c={1}", header.ToHeaderName (), c));
                    }
                } else {
                    bodyAlgorithm = DkimCanonicalizationAlgorithm.Simple;
@@ -2093,8 +2103,8 @@ namespace MimeKit {
                bodyAlgorithm = DkimCanonicalizationAlgorithm.Simple;
            }

-           if (!parameters.TryGetValue ("h", out h))
-               throw new FormatException ("Malformed DKIM-Signature header: no signed header parameter detected.");
+           if (!parameters.TryGetValue ("h", out string h))
+               throw new FormatException (string.Format ("Malformed {0} header: no signed header parameter detected.", header.ToHeaderName ()));

            headers = h.Split (':');
            for (int i = 0; i < headers.Length; i++) {
@@ -2105,19 +2115,19 @@ namespace MimeKit {
            }

            if (!containsFrom)
-               throw new FormatException (string.Format ("Malformed DKIM-Signature header: From header not signed."));
+               throw new FormatException (string.Format ("Malformed {0} header: From header not signed.", header.ToHeaderName ()));

            if (!parameters.TryGetValue ("bh", out bh))
-               throw new FormatException ("Malformed DKIM-Signature header: no body hash parameter detected.");
+               throw new FormatException (string.Format ("Malformed {0} header: no body hash parameter detected.", header.ToHeaderName ()));

            if (!parameters.TryGetValue ("b", out b))
-               throw new FormatException ("Malformed DKIM-Signature header: no signature parameter detected.");
+               throw new FormatException (string.Format ("Malformed {0} header: no signature parameter detected.", header.ToHeaderName ()));
        }

-       static Header GetSignedDkimSignatureHeader (Header dkimSignature)
+       static Header GetSignedSignatureHeader (Header header)
        {
            // modify the raw DKIM-Signature header value by chopping off the signature value after the "b="
-           var rawValue = (byte[]) dkimSignature.RawValue.Clone ();
+           var rawValue = (byte[]) header.RawValue.Clone ();
            int length = 0, index = 0;

            do {
@@ -2155,14 +2165,14 @@ namespace MimeKit {
            } while (index < rawValue.Length);

            if (index == rawValue.Length)
-               throw new FormatException ("Malformed DKIM-Signature header: missing signature parameter.");
+               throw new FormatException (string.Format ("Malformed {0} header: missing signature parameter.", header.Id.ToHeaderName ()));

            while (index < rawValue.Length)
                rawValue[length++] = rawValue[index++];

            Array.Resize (ref rawValue, length);

-           return new Header (dkimSignature.Options, dkimSignature.RawField, rawValue);
+           return new Header (header.Options, header.RawField, rawValue);
        }

        async Task<bool> VerifyAsync (FormatOptions options, Header dkimSignature, IDkimPublicKeyLocator publicKeyLocator, bool doAsync, CancellationToken cancellationToken)
@@ -2173,8 +2183,8 @@ namespace MimeKit {
            if (dkimSignature == null)
                throw new ArgumentNullException (nameof (dkimSignature));

-           if (dkimSignature.Id != HeaderId.DkimSignature)
-               throw new ArgumentException ("The dkimSignature parameter MUST be a DKIM-Signature header.", nameof (dkimSignature));
+           if (dkimSignature.Id != HeaderId.DkimSignature && dkimSignature.Id != HeaderId.ArcMessageSignature)
+               throw new ArgumentException ("The dkimSignature parameter MUST be a DKIM-Signature or ARC-Message-Signature header.", nameof (dkimSignature));

            if (publicKeyLocator == null)
                throw new ArgumentNullException (nameof (publicKeyLocator));
@@ -2187,8 +2197,8 @@ namespace MimeKit {
            string[] headers;
            int maxLength;

-           ValidateDkimSignatureParameters (parameters, out signatureAlgorithm, out headerAlgorithm, out bodyAlgorithm,
-                                            out d, out s, out q, out headers, out bh, out b, out maxLength);
+           ValidateSignatureParameters (parameters, dkimSignature.Id, out signatureAlgorithm, out headerAlgorithm, out bodyAlgorithm,
+                                        out d, out s, out q, out headers, out bh, out b, out maxLength);

            if (doAsync)
                key = await publicKeyLocator.LocatePublicKeyAsync (q, d, s, cancellationToken).ConfigureAwait (false);
@@ -2210,9 +2220,9 @@ namespace MimeKit {

                    DkimWriteHeaders (options, headers, headerAlgorithm, filtered);

-                   // now include the DKIM-Signature header that we are verifying,
+                   // now include the DKIM-Signature or ARC-Message-Signature header that we are verifying,
                    // but only after removing the "b=" signature value.
-                   var header = GetSignedDkimSignatureHeader (dkimSignature);
+                   var header = GetSignedSignatureHeader (dkimSignature);

                    switch (headerAlgorithm) {
                    case DkimCanonicalizationAlgorithm.Relaxed:
jstedfast commented 5 years ago

I need test cases for this.

The-Nutty commented 5 years ago

I need test cases for this.

Gmail adds ARC to all the emails so thats one way to get them. Alternatively the python dkimpy project supports arc signing and could be used to generate examples.

A list of arc implementations can be found here http://arc-spec.org/?page_id=79 although currently there are not meany...

jstedfast commented 5 years ago

This is WAY WAY WAY more work than just passing the ARC-Message-Signature header into the same validator method used by DKIM-Signature :-\

jstedfast commented 5 years ago

Here's the start of some validator unit tests for anyone who decides to take on the task of implementing support for ARC validation: https://gist.github.com/jstedfast/36736af88424fa380c0ed1f382532f90

jstedfast commented 5 years ago

Here's a link to the draft tracker: https://datatracker.ietf.org/doc/draft-ietf-dmarc-arc-protocol/

Latest draft is https://tools.ietf.org/id/draft-ietf-dmarc-arc-protocol-23.txt

jstedfast commented 5 years ago

There are some failing tests that I can't figure out. Maybe one of you guys can look it over and figure out why they are failing.

jstedfast commented 5 years ago

Okay, all but 2 or 3 ARC unit tests now pass and the ones that still fail are questionable.

jstedfast commented 5 years ago

@The-Nutty please take a look at the latest DkimSigner API: https://github.com/jstedfast/MimeKit/blob/master/MimeKit/Cryptography/DkimSigner.cs

Specifically, take a look at the Sign() methods and help me come up with an API for generating ARC headers on a MimeMessage.

Questions I need answers to:

  1. Should the ArcSigner class's Sign() method take an ArcVerifier so that it can verify any existing ARC chains? Or should the caller supply the cv= value? Or should it just assume "pass" (or "none" if it is the first ARC set)?
  2. Should the caller be responsible for figuring out the i= value to use or should ArcSigner be responsible for that?
  3. How should ARC-Authentication-Results be generated?
jstedfast commented 5 years ago

Also feel free to comment on the ArcVerifier API - is the current tri-state enum what people want? Or should it return more information? If so, what additional information should it return?

The-Nutty commented 5 years ago
  1. I think ideally the ArcSigner should verify the chain and apply the appropriate "cv=" tag, i cant think of a reason someone would want to override that, but i guess there could be a use case.....
  2. Same answer here, from an API UX perspective i think the caller should not be responsible for figuring out the "i=" value. I cant think of a (non malicious) use case where you would want to override the "i=" value. Furthermore if you do override the "i=" value i assume you would want to also override the "cv=" so there may be a use case there.
  3. So there are a few use cases as i see it a. If "cv=pass" then you presumably want the dkim, spf and dmarc potion to be the same as the previous ARC-Authentication-Results. I imagen in this case this is what 99% of people will want to do. b. If there is no prior ARC headers, then the header will need to be generated. I cant see why this would not want to be generated automatically, so the only issue i see here would be generating SPF, so the MAIL FROM would need to be supply as well as an interface for looking up SPF records (Similar to or an extension of the dkim one currently in place perhaps). For dmarc you have the same problem of needing to fetch a DNS record, but you also then have the problem of what todo if dmarc fails and for example p=reject (I would think this should be upto the caller to look at the resulting arc-authentication-results and take the appropriate action including sending feedback to a mailto address). I would think all mailkit should do to veriify dmarc is check for a dmarc record and verify dkim/spf/mailfrom domains are in alignment https://tools.ietf.org/html/rfc7489. c. There could be a use case where you want to supply fabricated authentication results, although i cant see a non malicious use case for this. I think this is a call on your part you could allow the caller to pass in other implementations of an AuthResultsGenerator for example?

As for the ArcVerifier i think the one this that is defiantly would be good is returning the "i=" value as then the caller can at least find the corresponding ARC-Authentication-Results and do as they want with that data, perhaps its just worth returning those headers but im not sure that would fit into the api very well.

Furthermore it might be worth returning (for each step in the chain) the hostname of the party that signed the arc-seal (for example google.com), the "cv=" and the "i=" values. I say this as im aware that currently arc implementing services (for example google) dont blindly trust any arc implementing party but instead have a white list of people who they trust. By returning this extra data it would allow people to implement a similar white list (for example http://arc-spec.org/?page_id=155).

I really appropriate all the work you are doing to support arc if you have any other questions let me know.

jstedfast commented 5 years ago

Well, just wrote an [ARC-]Authentication-Results parser/serializer. Yikes that was a ton of work...

I hope there's some donations in this for me 😅

jstedfast commented 5 years ago

Ok, I've got a working ArcSigner now complete with unit tests.

Take a look at https://github.com/jstedfast/MimeKit/blob/master/UnitTests/Cryptography/ArcSignerTests.cs and https://github.com/jstedfast/MimeKit/blob/master/UnitTests/Cryptography/DummyArcSigner.cs to see how to use this new API.

Currently, the 2 methods a subclass MUST implement are the sync & async versions of GetArcAuthenticationResults/Async().

There's an Async version because a subclass MAY wish to invoke the ArcVerifier and potentially the DkimVerifier as well when generating the ARC-Authentication-Results in order to populate the authentication methods and their results.

The DummyArcSigner currently just parses and merges any existing Authentication-Results headers and uses the result.

The-Nutty commented 5 years ago

Thanks, i have taken a look and that looks really good, i seriously appreciate the work you have put into this.

jstedfast commented 5 years ago

Going back to what you suggested the other day, is this what you were thinking as far as what ArcVerifier.Verify() should return?

/// <summary>
/// An ARC validation result.
/// </summary>
/// <remarks>
/// An ARC validation result.
/// </remarks>
public enum ArcValidationResult
{
    /// <summary>
    /// No validation was performed.
    /// </summary>
    None,

    /// <summary>
    /// The validation passed.
    /// </summary>
    Pass,

    /// <summary>
    /// The validation failed.
    /// </summary>
    Fail
}

/// <summary>
/// An ARC header and its validation result.
/// </summary>
public class ArcHeader
{
    public ArcValidationResult Result;
    public Header Header;
}

/// <summary>
/// The results of verifying the ARC signatures of a message.
/// </summary>
class ArcVerifyResult // or maybe ArcStatus or ArcValidationStatus? or ArcSignatures?
{
    public ArcHeader MessageSignature;
    public ArcHeader[] Seals;
    public ArcValidationResult Chain;
}

I'm not sure about the naming convention, here, but the general idea is what I think you were hinting at.

The index into the Seals list would be the ARC instance adjusted for a 0-base (i.e. instance - 1).

The-Nutty commented 5 years ago

Yeah, that seems to be basically it, you can now gather all the important data (instance and signing party from the seal headers).

Few comments though:

jstedfast commented 5 years ago

Correct, the ArcHeader MessageSignature would be the latest ARC-Message-Signature header which is the only ARC-Message-Signature that the verifier checks (as per the specs). The reason I included it is because it tells you whether the message itself has been modified or not since the latest ARC headers were generated.

Each ARC-Seal only signs the previous ARC headers (including the other ARC headers with the same instance value) and so do not tell you anything about the validity of the message itself.

The Chain status gives you the high-level valid vs invalid state which is all ArcVerifier.Verify() currently returns right now.

Adding the ARC-Authentication-Results isn't unreasonable. I guess the question there is, do we want the Header? Or the AuthenticationResults? Consistency would dictate the Header be supplied, but the AuthenticationResults might be more useful.

That brings me to the question of whether or not it's really all that useful to have the Header for the other ARC headers as well. Their signature validity is the most valuable piece of info.

The-Nutty commented 5 years ago

Ah of course yeah.

For ARC-Authentication-Results i think adding the Header makes the most sense as then the caller has access to the raw value which may be useful (although looking through the AuthenticationResults no info should be lost by doing AuthenticationResults.toString() just potentially re formatted) and furthermore it can easily be parsed by the caller to an AuthenticationResults but personally i dont think there is much in it.

As for including headers at all, i think its a nice QOL thing but it is all extra info that can be gathered by the caller from the headers and the instance.

jstedfast commented 5 years ago

I think we can call this done now 😅🎉🎊