RFC - Proposal for an updated JDKIM API #27
Conversation
In the proposed changes, DKIMSigner API is made more functional, both signature template and private key become a parameter of the signing operation. Additionally the signature algorithm is also made a parameter This enables per signature template and private key variation and more importantly allows signing with multiple keys which is necessary to support migration to new signing algoritms as proposed in https://datatracker.ietf.org/doc/html/rfc8463#section-3 The other signature template fields would probably also benefit from having a structured API switching from string parsing to a record to make it easier for consumer to handle different signature requirements for different domains for example. It would also make the API safer to use by better documenting which fields are mandatory and which are optional with default values. The change to DKIMVerifier still needs work but it returns a record instead of a list the record encodes the standard pass criteria, it contains a list of signature verifications that record the exact state of verifying each record. The introduction of the SignatureVerification allows to uncouple the structre of a template and the structure of a verification that are currently welded together through the SignatureRecord interface despite having different mandatory fields or behaviors. This part may still need work as I am not very familiar with the verification constraints and may have missed operational concerns that are not in the specification.
| FAIL, | ||
| /** | ||
| * The message was signed, but some aspect of the signature or | ||
| * signatures was not acceptable to the ADMD. |
There was a problem hiding this comment.
ADministrative Management Domain (ADMD)
The descriptions are copy pasted straight from the [Message Header Field for Indicating Message Authentication Status RFC] (https://datatracker.ietf.org/doc/html/rfc7001#section-2.6.)
The full definition for an administrative management domain is given this section
A final version of the API should include these references in the comments of the class
| case FAIL: | ||
| return Stream.of("dkim=fail", reason, tag) | ||
| .filter(it->!it.isEmpty()) | ||
| .collect(joining("", " ", ";")); | ||
| case POLICY: | ||
| return Stream.of("dkim=policy", reason, tag) | ||
| .filter(it->!it.isEmpty()) | ||
| .collect(joining("", " ", ";")); | ||
| case NEUTRAL: | ||
| return Stream.of("dkim=neutral", reason, tag) | ||
| .filter(it->!it.isEmpty()) | ||
| .collect(joining("", " ", ";")); | ||
| case TEMPFAIL: | ||
| return Stream.of("dkim=temperror", reason, tag) | ||
| .filter(it->!it.isEmpty()) | ||
| .collect(joining("", " ", ";")); | ||
| case PERMFAIL: | ||
| return Stream.of("dkim=permerror", reason, tag) | ||
| .filter(it->!it.isEmpty()) | ||
| .collect(joining("", " ", ";")); |
There was a problem hiding this comment.
Minor remark: we could get rid of a bit of duplication with method extraction as all those do the same?
There was a problem hiding this comment.
I haven't even tried to polish the implementation. this PR focuses on the shape of the API, supporting implementation is only here to help understand the shape of the API and by no means definitive or mergeable :D
|
|
||
| private static class DkimVerification { | ||
| private final List<SignatureVerification> signatureVerifications; | ||
| boolean isSuccess; |
chibenwa
left a comment
chibenwa
left a comment
There was a problem hiding this comment.
On the principle I more than agree with the proposed change set but I am surprised not to see code for the dkimVerifier.verify internals though I think it is straight forward.
|
I want to only focus on the API and the shape of the API from a user's point of view. This MR is not destined to be merged only to agree on the target API. There is supporting code that participates in the API but I didn't even try to build the full implementation |
|
I more than agree with the proposed changes. |
|
I would love to hear the opinion of @epinter especially on the verification API as he recently contributed to it. the verification api design is probably not perfect yet but I wanted to start the discussion |
|
Didn't see this before, sorry... The signing api changes are positive, I like it. About the verifier, I think we couldn't avoid to keep a state in the verifier, so why not just make the verify method return void, or even a boolean with the result of "DkimVerification.isSuccess" ? About the "DkimVerification.asHeaderString", I think it should return a list. Today I'm using the DKIMVerifier.getResults() header text to build an authentication-results header with a class dedicated to compile results from jSPF, JDKIM and "iprev". If I had a multi-line string, I think I would need to do some work on it before put it in auth-results header. As an example, I'm testing with and without reason field. If we simply make the verify method return void or boolean, we could improve the current Result class, for example add getters to return header.b and header.s individually so the user can customize string to put into authentication results. EDIT: quick and dirty test with boolean, with verify returning List<Result>, and with void |
3 participants
In the proposed changes, DKIMSigner API is made more functional, both signature template and private key become a parameter of the signing operation. Additionally the signature algorithm is also made a parameter
This enables per signature template and private key variation and more importantly allows signing with multiple keys which is necessary to support migration to new signing algoritms as proposed in https://datatracker.ietf.org/doc/html/rfc8463#section-3
The other signature template fields would probably also benefit from having a structured API switching from string parsing to a record to make it easier for consumer to handle different signature requirements for different domains for example. It would also make the API safer to use by better documenting which fields are mandatory and which are optional with default values.
The change to DKIMVerifier still needs work but it returns a record instead of a list
the record encodes the standard pass criteria,
it contains a list of signature verifications that record the exact state of verifying each record.
The introduction of the SignatureVerification allows to uncouple the structre of a template and the structure of a verification that are currently welded together through the SignatureRecord interface despite having different mandatory fields or behaviors.
This part may still need work as I am not very familiar with the verification constraints and may have missed operational concerns that are not in the specification.