Add clock drift tolerance to signature validation

[epinter]

Avoids signature validation failures when clock drift is lower than the threshold.

[chibenwa]

Thanks for this contribution.

A clock drift tolerence of 5 minutes being the operational norm (OpenDKIM default) this contribution is welcome but I would personally love to have this configurable.

[chibenwa]

I do think the tolerated clock drift can be a field of the verifier.
Then a simple setter can thus allow to control its value.
What do you think about this solution? (cf the related commit)

[chibenwa]

CF https://issues.apache.org/jira/browse/JDKIM-49

I can squash this upon merge to get the issue number into the commit message.

[epinter]

I do think the tolerated clock drift can be a field of the verifier. Then a simple setter can thus allow to control its value. What do you think about this solution? (cf the related commit)

After I pushed the PR I saw the other PR with API change proposal, I tought a setter would not be welcome, but it was the my first solution and it is what I'm running in my mail server.

I agree with it, but I think instead of throwing an exception in the setter we could just keep it with the default value when parameter is negative.

[epinter]

Updated commit message

[chibenwa]

If mutability is an issue we can make the field final and have a

DkimVerifier withClockDriftTolerance(Duration tolerance);

field in DkimVerifier that does a copy of it and update that only field.

Regarding API proposed in #27 I think we would be able to easily add that useful feature in it.

Thoughts?

[epinter]

If mutability is an issue we can make the field final and have a

DkimVerifier withClockDriftTolerance(Duration tolerance);

field in DkimVerifier that does a copy of it and update that only field.

Regarding API proposed in #27 I think we would be able to easily add that useful feature in it.

Thoughts?
I like the use of Duration. Your idea is make withClockDriftTolerance static that returns an instance of DKIMVerifier ?

I don't see a problem with mutability with this configuration alone, I suggested to remove the exception just to avoid a runtime error if there's some weird bug on the user side, I prefer to throw exceptions in the constructor to avoid weird object states if more features are added. Looking to just this config, I'm fine with both solutions. In the future if more configuration options are needed, maybe an Options class containing parameters could be added to a second constructor in DKIMVerifier.

EDIT: something like this

[epinter]

Let me know what is more aligned with your view of the possible API changes and I will update this PR.

[chibenwa]

Let me know what is more aligned with your view of the possible API changes and I will update this PR.

This looks great to me 👍

[epinter]

I kept DKIMVerifier(PublicKeyRecordRetriever publicKeyRecordRetriever) for backwards compatibility. For me it could be removed or marked as deprecated. Let me know if you prefer anything different, if you find it is too much for just a few options, I can drop the VerifierOptions and keep just the setter like you said earlier.

[chibenwa]

Awesome!

I will wait 1 day or 2 to let the chance others to review before I merge this, but it looks perfect as is.

[Arsnael]

LGTM, thank you for this work @epinter !

[chibenwa]

Thanks @epinter for this very nice contribution and everyone for the review!