Fix failed signatures with multiple transitions

[abeverley]

Commit f6b57dc causes messages transitioning multiple times in the same Authserv-ID domain to always have an ARC result of fail. This PR allows that to happen and takes the most recent result instead.

Fixes issue #172

[futatuki]

Perhaps the commit f6b57dc intended to reject messages with maricious AR header(s), and I agree that it is not good.

However, I think the condition if (arfound > 1) does not suffice for ignore ARC result in AR header.

If an openarc milter runs on mode contains "v", it means that the openarc milter verifies messages by itself (and then insert a AR header), so it should not believe any ARC results in AR header in the case, I think. (e.g. assume a message is submitted to the MTA with openarc running on "sv" mode, the message signed as i=1 cv=none and added AR header with arc=none. Then it sent to remote MTA and the message is forwarded by an alias or mailing list system, and finally returns to the first MTA with remaining the AR header for ARC result "none").

On the other hand, if the milter runs on mode "s" without "v", it means that it believe the most recent result even if the message was modified after verification and use the result in the AR header for signing. It seems the PR code covers this case.

[abeverley]

Thanks @futatuki - just to confirm, do you think the PR is okay as it is?

As far as I am aware, authentication-results headers for the same authserv-id are ignored, unless they arrive from hosts on a trusted network (?)

[futatuki]

I'm not okay the PR as it is. Only the basic concept, that we should not regardlessly reject message which have multiple AR headers of ARC result in own Authserve-ID, is okay.

The config parameter PeerList is used for determine if the SMTP client is connected within a same ADMD and then accept if it is, no farther operation about ARC (so whole AR headers are ignored, of course).

The config parameter InternalHosts is only if the config parameter Mode is not set explicitly, used for determine which of "s" mode or "v" mode to be used. If a SMTP client is connected with in a InternalHosts, the openarc filter works as if "s" is specified in Mode, otherwise it works as if "v" is specified in Mode.

In the mode Mode "s", the milter uses previous result of ARC verification in AR header with same Authserve-id as mentions before, uses for both in the cv value of the ARC set and resinfo in the AAR header.

In the Mode "v" (without "s"), as the milter does not need to sign and does need to verify by itself, it ignores (does not check) all AR header. (Perhaps your awareness comes from this behaviour)

However, in the Mode "sv", as verify mode is explicitly specified, the milter should use its own verification result and should ignore the ARC result from AR header both in cv value of the ARC set and resinfo in the AAR header, I think. This mode is useful if the MTA is the final MTA which has mailboxes and it handles forwarding without modification (or with non-important modification such as adding X-foo header, etc.) of messages by aliases or users' .foward file.

[flowerysong]

(e.g. assume a message is submitted to the MTA with openarc running on "sv" mode, the message signed as i=1 cv=none and added AR header with arc=none. Then it sent to remote MTA and the message is forwarded by an alias or mailing list system, and finally returns to the first MTA with remaining the AR header for ARC result "none").

This is the responsibility of the ADMD border MTA to deal with; as per RFC 8601 "any MTA conforming to this specification MUST delete any discovered instance of this header field that claims, by virtue of its authentication service identifier, to have been added within its trust boundary but that did not come directly from another trusted MTA."

It's out of scope for OpenARC to make this modification, so in order to use A-R at all we have to be able to trust that this processing has occurred, and that any A-R headers for our ADMD were added within the trust boundary.

I do think it's reasonable to have this behaviour controlled by a config flag, and off by default. flowerysong/OpenARC@63fd6e8

[futatuki]

This is the responsibility of the ADMD border MTA to deal with; as per RFC 8601 "any MTA conforming to this specification MUST delete any discovered instance of this header field that claims, by virtue of its authentication service identifier, to have been added within its trust boundary but that did not come directly from another trusted MTA."

Sure, actually for that purpose I use opendkim with RemoveARAll yes.

It's out of scope for OpenARC to make this modification, so in order to use A-R at all we have to be able to trust that this processing has occurred, and that any A-R headers for our ADMD were added within the trust boundary.

This assumption seems to be valid.

I do think it's reasonable to have this behaviour controlled by a config flag, and off by default.

My thought is that it is controled by "v" flag in Mode, although the default is "allow override by the result of AR header".

flowerysong/OpenARC@63fd6e8

In one of my use case:

1. openarc with Mode "v" in inbound boundary of ADMD verify a message and MTA deliver a message to Mailing List Manager (MLM).
2. MLM modifies the message and then submit the message for derivery to subscribers.
3. submission MTA with openarc Mode "s" seals the message by using AR header added by inbound boundary openarc.

With this case, an ADMD is constructed be step 1. - 3. and modification in step 2. may break the signature of the last ARC set, however if I correctly understand, it can use the varidation result in step 1. If the milter always update arc_state by arc_eom() and it is not allowed to override a fail status on arc_eom() by AR header, we can't use it as above case.

[flowerysong]

My thought is that it is controled by "v" flag in Mode,

I don't really like magical behaviour like that, which is one reason I made it a config flag. The other reason is people who don't specify a mode, who should also have a way to control this behaviour.

although the default is "allow override by the result of AR header".

We can change the default, I don't feel strongly about it. Defaulting it to "no" is safer in the case of misconfigured environments, but it might be better to remain compatible with the old behaviour by default.

If the milter always update arc_state by arc_eom() and it is not allowed to override a fail status on arc_eom() by AR header, we can't use it as above case.

Right, this is why the flag exists. If you enable it (or if we default it to "yes") it will be allowed to override the fail status. The only cases where my change disallows an override are explicit cv=fail in an existing seal (doing this creates an invalid seal) or too many ARC sets existing (which is unlikely to happen in the real world, and can't be sealed anyway.)

[futatuki]

... The only cases where my change disallows an override are explicit cv=fail in an existing seal (doing this creates an invalid seal) or too many ARC sets existing (which is unlikely to happen in the real world, and can't be sealed anyway.)

My bad, I couldn't read in ar_eom() correctly. In the case I wrote above, if ARC result in step 1. is pass and modification in step 2. make the result of arc_validate_msg() fail, it makes cv = fail but does not make msg->arc_infail = TRUE (and in this case it should be misg->arc_infail == FALSE because of the result in step 1), so it is still possible to override the cv value.