trusteddomainproject
Improve AR parser
openarc/openarc-ar.c (ares_parse):
- strictly check the use of "none" (no-result) instead of one or more resinfo
- record really "result comment" to result_comment field
- overwrite the former result instead of dedup after writing to new result
- do not record the results of unknown method (Fix issue #113)
- even after the number of recorded result has reached to the limit, continue to parse for syntax checking (and update the former result if newer value is found for seen methods).
Others:
- Remove dedup() in openarc/opearc-ar.c
- Fix main() function for ARTEST enabled in openarc-ar.c for testing above
- In mlfi_eom in openarc.c, add foolproof check for ignoring unknown results
@futatuki This is next on my list to review, but it might take a couple of days. It looks reasonable on its face and compiles, but I want to write some tests specifically for this functionality and see if they turn up any edge cases.
Okay, I've had a chance to look at this and while it's an improvement I was still unhappy overall with the results I was getting from my first couple of high-level tests.
https://github.com/flowerysong/OpenARC/commit/da1fa93d51f273e68fe69bbdb0b5dfb11de8add1 is my initial work on top of your changes. This cleans up the parsing code by using names for the state, building the current resinfo in a temporary struct, and deduplicating method results across all parsed headers from the ADMD instead of just within a single header. This does move the limit of 16 stored results from per-header to overall, but I think that's reasonable.
The potentially controversial changes to things that you also touched:
Comments: Instead of only storing one comment and requiring that it immediately follow the methodspec, treat comments as properties so that we can have multiple per method and they're correctly located in relation to properties (e.g. iprev=fail (IP not in A record) iprev.policy=192.0.2.1 (PTR mail.protection.example.com);). Comments located before the methodspec or in the middle of parsing units (like smtp.mailfrom(authenticated) = [email protected]) will still be discarded, and I'm considering adding a milter config to discard all comments.
Method deduplication: instead of taking the last result for a given method and discarding previous ones, take the first one and then refuse to replace it. The choice of which one to respect is pretty arbitrary, and doing it this way means we don't have different rules for duplicates within a header (last one wins) and duplicates across multiple headers (first one wins.)
I still have more testing to do, but I wanted to check in and see if you have any feedback about what I've done so far.