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.

Sign up for GitHub

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

Jump to bottom

milter: Authentication-Results improvements #4

Merged
merged 2 commits into from
Oct 15, 2024
Merged

milter: Authentication-Results improvements #4

merged 2 commits into from
Oct 15, 2024

Conversation

flowerysong
Copy link
Owner

@flowerysong flowerysong commented Oct 11, 2024 •
edited
Loading

From trusteddomainproject/OpenARC#177:

  • Fail parsing if no-result is combined with actual results
  • Ignore unknown methods during parsing
  • Parse the entire header for correctness even if we run out of room to store results
  • Increase the number of tokens it's possible to parse

In addition:

  • Add tests.
  • Use named states to make it less terribad to read.
  • Track the current method in a temporary struct instead of putting partial results into ar.
  • Don't parse headers if we don't care about them.
  • Comprehensively dedup authentication results, instead of just within a single parsed header.
  • Store all interesting comments.
  • Clean up whitespace inside of comments.
  • Fix non-terminal state conditions in the tokenizer and parser.
  • Output "reason" in the correct location.
  • Return ARES_RESULT_UNDEFINED when mapping result strings to values does not find a match; ARES_RESULT_UNKNOWN is an actual result.
  • Parsed values can also need quoting and escaping on output.
  • Add missing IANA registered values from https://www.iana.org/assignments/email-auth/email-auth.xhtml

@futatuki @flowerysong
Improve AR parsing
ccd528f 
openarc/openarc-ar.c: silence a compiler with ARTEST defined.

* openarc/openarc-ar.c (main):
  explicitly cast pointer to signed char into pointer to u_char.

openarc/openarc-ar.c: guarantiee that no-result AR has no other resifo

* openarc/openarc.c (ares_parse):
  Check prevstate if "none" is found on input token in state 3.

openarc/openarc-ar.c: Store truely result comment into result_comment field

"Result comment" is a comment just after "result" of the "methodspec".
No other comments that can be allowed in AR syntax should be ignored.

* openarc/openarc-ar.c (ares_parse):
   Check prevstate before storing result comment. If it stored once
   update prevstate so as not to store again.

openarc/openarc-ar.c: Overwriting former result instead of dedup after adding

When the parser find a new result and its "method" is not "dkim",
overwriting the old result of same "method" if exists, instead of
check the duplicate of the latest result just finished recording
and move it. This may allow to parse longer header a bit.

* openarc/openarc-ar.c
  (ares_dedup): Removed
  (ares_method_seen): New.
  (ares_parse): As the description above.

openarc/openarc-ar.c: Ignore results of unknown methods.

According to RFC 8601 Section 2.7.6[1], unknown authentication
methods in AR header should be ignored or deleted. With this
commit, ares_parse() does not store the result of authres
which method is 'unknown'.

[1] https://www.rfc-editor.org/rfc/rfc8601.html#section-2.7.6

* openarc/openarc-ar.c (ares_parse): As described above.

openarc/openarc-ar.c: continue parsing after reached the limit of
the number of auth results.

* openarc/openarc-ar.c (ares_parse):
   Do not return and continue parsing even if there is no space to
   record more results, for syntax checking, or overwrite the result
   of some auth method already seen.

openarc/openarc-ar.c (main (for testing)): Use appropriate macros

openarc/openarc-ar.c (main (for testing)): print result_comment

openarc/openarc-ar.c: Inclease max number of tokens for parsing AR header

openarc: Ignore unknown method in the result of parsing AR header

Since we discard resinfo of which method is not known in AR header
parsing now, it should never happen. However we check it as a foolproof.

* openarc/openarc.c (mlfi_eom):
   Ignore unknown method in the result of parsing AR header, and
   log it.
@flowerysong flowerysong force-pushed the ar branch 4 times, most recently from b9cadce to 3910059 Compare October 14, 2024 16:19
@flowerysong flowerysong marked this pull request as ready for review October 14, 2024 16:20
@flowerysong
Rework AR parsing
52b28e9 
* Add tests.
* Use named states to make it less terribad to read.
* Track the current method in a temporary struct instead of putting
  partial results into `ar`.
* Don't parse headers if we don't care about them.
* Comprehensively dedup authentication results, instead of just within a
  single parsed header.
* Store all interesting comments.
* Clean up whitespace inside of comments.
* Fix non-terminal state conditions in the tokenizer and parser.
* Output "reason" in the correct location.
* Return ARES_RESULT_UNDEFINED when mapping result strings to values
  does not find a match; ARES_RESULT_UNKNOWN is an actual result.
* Parsed values can also need quoting and escaping on output.
* Add missing IANA registered values from https://www.iana.org/assignments/email-auth/email-auth.xhtml
@flowerysong flowerysong merged commit 52b28e9 into main Oct 15, 2024
4 checks passed
@flowerysong flowerysong deleted the ar branch October 15, 2024 18:32
@flowerysong flowerysong added this to the v1.0 milestone Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug enhancement
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@flowerysong @futatuki