Refine algorithm policy handling

MatthiasValvekens commented 1 year ago

Description of the changes

Introduce a subclass CMSAlgorithmUsagePolicy of AlgorithmUsagePolicy for use with CMS validation specifically
- Use that subclass to put in a hook to allow incongruent digest algorithms (fixes #242)

Caveats

There is some minor API incompatibility, but only in internal APIs.

Checklist

Please go over this checklist to increase the chances of your PR being worked on in a timely manner. Deviations are allowed with proper justification (see previous section).

[x] I have read the project's CoC and contribution guidelines.
[x] I understand and agree to the terms in the Developer Certificate of Origin as applied to this contribution.
[x] All new code in this PR has full test coverage.

For new features (delete if not applicable)

[x] I have discussed the implementation of this feature with the project maintainer(s) on the discussion forum or over email.
[x] I have verified that my changes do not break existing API or CLI functionality, or ensured that all breaking changes are clearly documented in this PR.
[x] All public API functionality in this PR is documented.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 :tada:

Comparison is base (01a35a5) 98.23% compared to head (329dec1) 98.24%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #245 +/- ## ========================================== + Coverage 98.23% 98.24% +0.01% ========================================== Files 86 86 Lines 13016 13048 +32 ========================================== + Hits 12786 12819 +33 + Misses 230 229 -1 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `98.24% <100.00%> (+0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/MatthiasValvekens/pyHanko/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens) | Coverage Δ | | |---|---|---| | [pyhanko/sign/general.py](https://codecov.io/gh/MatthiasValvekens/pyHanko/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens#diff-cHloYW5rby9zaWduL2dlbmVyYWwucHk=) | `99.41% <100.00%> (+<0.01%)` | :arrow_up: | | [pyhanko/sign/validation/ades.py](https://codecov.io/gh/MatthiasValvekens/pyHanko/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens#diff-cHloYW5rby9zaWduL3ZhbGlkYXRpb24vYWRlcy5weQ==) | `91.35% <100.00%> (+0.01%)` | :arrow_up: | | [pyhanko/sign/validation/generic\_cms.py](https://codecov.io/gh/MatthiasValvekens/pyHanko/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens#diff-cHloYW5rby9zaWduL3ZhbGlkYXRpb24vZ2VuZXJpY19jbXMucHk=) | `96.26% <100.00%> (+0.05%)` | :arrow_up: | | [pyhanko/sign/validation/pdf\_embedded.py](https://codecov.io/gh/MatthiasValvekens/pyHanko/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens#diff-cHloYW5rby9zaWduL3ZhbGlkYXRpb24vcGRmX2VtYmVkZGVkLnB5) | `98.59% <100.00%> (+<0.01%)` | :arrow_up: | | [pyhanko/sign/validation/policy\_decl.py](https://codecov.io/gh/MatthiasValvekens/pyHanko/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens#diff-cHloYW5rby9zaWduL3ZhbGlkYXRpb24vcG9saWN5X2RlY2wucHk=) | `98.48% <100.00%> (+1.56%)` | :arrow_up: | | [pyhanko/sign/validation/utils.py](https://codecov.io/gh/MatthiasValvekens/pyHanko/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens#diff-cHloYW5rby9zaWduL3ZhbGlkYXRpb24vdXRpbHMucHk=) | `95.12% <100.00%> (+1.90%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Matthias+Valvekens)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

peteris-zealid commented 1 year ago

I looked at the changes. It seems like the 'proper' way to implement the feature. I cannot shake the feeling that this is an overkill for supporting functionality that was strongly discouraged in the previous spec and prohibited in the current one.

Still, this is your codebase and your rules - I can respect that. Thank you for putting in the effort. My only question is when do you sleep?

MatthiasValvekens commented 1 year ago

Thanks!

I looked at the changes. It seems like the 'proper' way to implement the feature. I cannot shake the feeling that this is an overkill for supporting functionality that was strongly discouraged in the previous spec and prohibited in the current one.

That's fair, but either way the AlgorithmUsagePolicy interface might need some CMS-specific attention in the future anyway, so I don't mind the extra effort. :) I spent like an hour on this at most (plus some extra to diagnose an issue in an existing test).

FWIW, I plan to give these "policy objects" a more prominent place in the API going forward, as a way to gradually clean up the stateful mess that is ValidationContext... Don't want to incur too much technical debt there :).

My only question is when do you sleep?

Having a baby in the house does strange things to one's circadian rhythm 🙃 ...

peteris-zealid commented 1 year ago

A small thing I noticed is that algorithm_usage_policy argument in function validate_sig_integrity is marked as optional, but in code it will always have a real value. The typing is not correct? (also see validate_raw)

fixes.patch

peteris-zealid commented 1 year ago

Also found a typo fix-typo.patch

MatthiasValvekens commented 1 year ago

A small thing I noticed is that algorithm_usage_policy argument in function validate_sig_integrity is marked as optional, but in code it will always have a real value. The typing is not correct? (also see validate_raw)

This is the way it is for historical reasons; there are a couple tests that predate the addition of the algo policy parameter, for example, and I have some experimental code lying around that calls validate_sig_integrity directly. I'm not necessarily opposed to making it a required param, but it'd require some care to do it right.

I've applied and merged your other suggestion, though :)

MatthiasValvekens / pyHanko