DkimVerifier report both header and body result seperatly

[The-Nutty]

When verifying dkim in our use case it is useful to be able to tell if the header check failed or the body check failed (as opposed to currently where you just know that one of them failed).

So instead of the DkimVerifier Verify/VerifyAsync returning a bool they now return a DkimValidationResult with 2 property's: BodySignatureValid true if the body portion of the dkim check passes and HeaderSignatureValid true if the header portion passes. Dkim should only be considered to pass if both are true. I toyed with adding an extra property along the lines of public bool VerificationResult => HeaderSignatureValid && BodySignatureValid; but did not in the end.

[jstedfast]

The problem with this is that it changes/breaks the API :-\

[The-Nutty]

Yeah, would adding a method (under the current name) to maintain the existing interface be better? so you could have bool VerifyAsync and DkimValidationResult VerifyAdvancedAsync for example (i dont think i like the name VerifyAdvancedAsync but more the idea)?

[jstedfast]

Yea, I was thinking along those lines - I just haven't come up with a good method name either.

Another option I was thinking about was just making the core private method protected instead, but I hate exposing the bool doAsync argument because it's an ugly hack.

The other thing that I was thinking was to modify the internal core method (the internal VerifyAsync method) to return a bitflag enum value as opposed to a class with 2 bools. Along that same line, I was also thinking of modifying that method to take an option to fully verify or not so that if someone uses the current method where they only care about valid/not-valid, it doesn't waste time fully evaluating things.

I really hate that the ArcVerifier returns an ArcValidationResult and that DKIM is already inconsistent and will be even more inconsistent with any change made here.

I'm thinking I'd rather hold off on this until a "MimeKit 3.0" which would give me an excuse to break API.

[The-Nutty]

Would a cleaner temporary solution be to add 2 extra methods VeryBodyAsync and VerifyHeadersAsync? They could even be protected methods such that the VerifyAsync method just returns await VeryBodyAsync(...) && VerifyHeadersAsync(...)? This would allow me to override the class and add a VerifyAdvancedAsync method of my own.

[jstedfast]

I added 2 new methods to DkimVerifierBase for verifying the body hash and the signature.

[The-Nutty]

Unless im missing something do ParseParameterTags and ValidateDkimSignatureParameters need to be protected as well so i can implement my own method like:

        async Task<DkimResult> VerifyCompexAsync (FormatOptions options, MimeMessage message, Header dkimSignature, CancellationToken cancellationToken)
        {
            if (options == null)
                throw new ArgumentNullException (nameof (options));

            if (message == null)
                throw new ArgumentNullException (nameof (message));

            if (dkimSignature == null)
                throw new ArgumentNullException (nameof (dkimSignature));

            if (dkimSignature.Id != HeaderId.DkimSignature)
                throw new ArgumentException ("The signature parameter MUST be a DKIM-Signature header.", nameof (dkimSignature));

            var parameters = ParseParameterTags (dkimSignature.Id, dkimSignature.Value);
            DkimCanonicalizationAlgorithm headerAlgorithm, bodyAlgorithm;
            DkimSignatureAlgorithm signatureAlgorithm;
            AsymmetricKeyParameter key;
            string d, s, q, bh, b;
            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);

            if (!IsEnabled (signatureAlgorithm))
                return false;

            options = options.Clone ();
            options.NewLineFormat = NewLineFormat.Dos;

            // first check the body hash (if that's invalid, then the entire signature is invalid)

            key = await PublicKeyLocator.LocatePublicKeyAsync (q, d, s, cancellationToken).ConfigureAwait (false);

            if ((key is RsaKeyParameters rsa) && rsa.Modulus.BitLength < MinimumRsaKeyLength)
                return false;

            return new DkimResult
            {
                SignatureResult = VerifySignature(options, message, dkimSignature, signatureAlgorithm, key, headers,
                    headerAlgorithm, b),
                HeaderResult = VerifyBodyHash(options, message, signatureAlgorithm, bodyAlgorithm, maxLength, bh)
            };
        }

[jstedfast]

Ugh, I was hoping to keep those as an implementation detail since the APIs are kinda gross.

[jstedfast]

Can you just copy & paste those 2 methods into your custom class? Exposing those methods traps me into never being able to change/refactor them.

[The-Nutty]

Yeah sure, thanks for the help.