Fix invalid ARC-Seal when email contains existing sets #167
Conversation
|
I'm not sure this is correct, though the code structure's fairly complicated and I don't really have time right now to try to grok it fully. This should already have been done by |
|
Thanks @flowerysong I see what you mean. If I get a chance I will have another look. |
|
Yes - the problem is at this line: Line 2915 in eb430db It only runs I assume it's as simple as removing the condition for |
|
Arguably that's a configuration error, and if you're signing messages that might already have ARC chains you should be setting the mode to I don't really feel like making that argument, though. Having a trap configuration that's maybe slightly more performant when you're purely a message originator but breaks for everyone else doesn't seem like a good idea. |
I've actually got no mode defined at all in the configuration, so it's just the default value. From the manual: "If neither is specified, the operating mode will be inferred on a per-connection basis based on the entries in the InternalHosts list; connections from internal hosts will be assigned to signing mode, and all others will be assigned to verify mode". So I guess it's automatically setting the mode to "sign", which then means arc_canon_runheaders_seal() is not executed.
Agreed - as it is the mode needs to include "verify" in order to properly "sign" ;-) I'll take a look to see if there is any way to still execute conditionally, but it would seem to make sense to always run that code as it could be needed for verification or signing. Thanks for your comments. |
This fixes a bug whereby existing sets were not being included in a signature and thus the signature was invalid. This was only happening when Mode was undefined (default value) or only signing. This meant that the code to verify existing sets was never executed. This commit removes the check for running the previous-set verification function, to ensure that it is run regardless (if there are no previous sets then arc_canon_runheaders_seal() is basically a no-op anyway.
abeverley
force-pushed
the
previous_sets
branch
from
7c51d3a
to
2093e7a
Compare
|
Based on the above discussion, I have changed this PR to always verify previous ARC sets, regardless of the mode. |
This fixes a bug whereby existing sets were not being included in a signature and thus the signature was invalid. This was only happening when Mode was undefined (default value) or only signing. This meant that the code to verify existing sets was never executed. This commit removes the check for running the previous-set verification function, to ensure that it is run regardless (if there are no previous sets then arc_canon_runheaders_seal() is basically a no-op anyway. trusteddomainproject/OpenARC#167
…_sets Fix invalid ARC-Seal when email contains existing sets This fixes a bug whereby existing sets were not being included in a signature and thus the signature was invalid. This was only happening when Mode was undefined (default value) or only signing. This meant that the code to verify existing sets was never executed. This commit removes the check for running the previous-set verification function, to ensure that it is run regardless (if there are no previous sets then arc_canon_runheaders_seal() is basically a no-op anyway. trusteddomainproject#167
When creating the ARC-Seal, any existing sets in the email must be included in the seal. See https://www.rfc-editor.org/rfc/rfc8617#section-5.1.1
The existing code does not include existing sets and is thus creating invalid signatures in these circumstances. This PR includes them.