trusteddomainproject / OpenARC

Open source ARC implementation
BSD 2-Clause "Simplified" License
135 stars 45 forks source link

issue #174: Fix incomplete AAR header if there is no own AR header #176

Open futatuki opened 2 months ago

futatuki commented 2 months ago

This fix issue #174

flowerysong commented 1 month ago

This should be fixed in libopenarc, not the milter. If you could test https://github.com/flowerysong/OpenARC/commit/9dc8658366b9b3cadbed09754fdf1a7e5cd61e47 and see if it works for you I would appreciate it.

futatuki commented 1 month ago

@flowerysong Thank you for the review.

I agree that arc_getseal() in libopenarc/arc.c should be fix, because the function already awares that the argment ar may be NULL pointer, and processes it without error. Althouh I've not yet tested your commit, it looks good to me. I've check it later.

However I don't think it is incorrect that a caller of arc_getseal() passes a string "none" as an ar argment, because arc_getseal() trusts the value of ar without checking other than if it is not NULL, so the responsibility of the correctness of the ar value as a authres-payload value is the caller (and the description of arc_getseal() does not mention the behaviour when ar is NULL).

flowerysong commented 1 month ago

The caller's responsible for it being correct if they provide one, but if they don't provide one the library should either still produce a syntactically valid seal or return an error. I think it's better behaviour for it to treat a NULL as no authentication results than it is to error out, especially since that's what the milter expected to happen.

Of course it's also fine for callers to explicitly pass in "none", we just shouldn't leave a trap in the API for unwary developers.

futatuki commented 1 month ago

Althouh I've not yet tested your commit, it looks good to me. I've check it later.

I've checked with https://github.com/flowerysong/OpenARC/commit/9dc8658366b9b3cadbed09754fdf1a7e5cd61e47 without ef5a6d046052fd8807f72b476d50e053b6b74dfc. It works fine as we expected.

So I've cherry-picked it, and update the description of arc_getseal().

Thanks,