Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

futatuki
Copy link

This fix issue #174

futatuki added a commit to futatuki/OpenARC that referenced this pull request Sep 16, 2024
@flowerysong
Copy link
Contributor

flowerysong commented Oct 2, 2024

This should be fixed in libopenarc, not the milter. If you could test flowerysong/OpenARC@9dc8658 and see if it works for you I would appreciate it.

@futatuki
Copy link
Author

futatuki commented Oct 3, 2024

@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
Copy link
Contributor

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 added a commit to futatuki/OpenARC that referenced this pull request Oct 3, 2024
…ue174-fix-aar-no-result"

This reverts commit 27365b9, reversing
changes made to 68bc7aa.
flowerysong and others added 2 commits October 3, 2024 15:39
It is not valid to have no authentication results; if no message
authentication was performed the special value no-result
(`[CFWS] ";" [CFWS] "none"`) should be used.

trusteddomainproject#174
@futatuki
Copy link
Author

futatuki commented Oct 3, 2024

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

I've checked with flowerysong/OpenARC@9dc8658 without ef5a6d0. It works fine as we expected.

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

Thanks,

futatuki added a commit to futatuki/OpenARC that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants