pyca
Inconsistent IDP extension constraint check
Hello, I found an inconsistent IDP extension constraint check per RFC5280 Section 5.2.5. I do not think the indirectCRL needs to be subject to that. Excerpt from the RFC:
-- at most one of onlyContainsUserCerts, onlyContainsCACerts, -- and onlyContainsAttributeCerts may be set to TRUE.
Thank you
Cc: @woodruffw this may be a CABF requirement
On Wed, Aug 21, 2024, 2:30 PM Han Yu @.***> wrote:
Hello, I found an inconsistent IDP extension constraint check per RFC5280 Section 5.2.5. I do not think the indirectCRL needs to be subject to that. Excerpt from the RFC:
-- at most one of onlyContainsUserCerts, onlyContainsCACerts, -- and onlyContainsAttributeCerts may be set to TRUE.
Thank you
You can view, comment on, or merge this pull request online at:
https://github.com/pyca/cryptography/pull/11467 Commit Summary
- 8ccec8d https://github.com/pyca/cryptography/pull/11467/commits/8ccec8db68f616a7a9263b9bae54339b8b395185 Per RFC5280 Section 5.2.5, the Issuing Distribution Point extension in a CRL can have only one of onlyContainsUserCerts, onlyContainsCACerts, onlyContainsAttributeCerts set to TRUE. However, extensions.py (lines 1991 : 2003), indirectCRL is also included, which leads to invalid CRL even if the RFC requirement is met. The proposed fix is to drop indirectCRL from the check so it conforms to the RFC.
File Changes
(1 file https://github.com/pyca/cryptography/pull/11467/files)
- M src/cryptography/x509/extensions.py https://github.com/pyca/cryptography/pull/11467/files#diff-2ad33db9160057b2dc1719647908099d649c68f1dad9f672153d24bda0d21fa3 (6)
Patch Links:
- https://github.com/pyca/cryptography/pull/11467.patch
- https://github.com/pyca/cryptography/pull/11467.diff
— Reply to this email directly, view it on GitHub https://github.com/pyca/cryptography/pull/11467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBDYQVPBR5JD6O5IO63ZSTMFDAVCNFSM6AAAAABM4SWWA2VHI2DSMVQWIX3LMV43ASLTON2WKOZSGQ3TQNZTGI3DAOA . You are receiving this because you are subscribed to this thread.Message ID: @.***>
Cc: @woodruffw this may be a CABF requirement
Here's the relevant CABF section: https://cabforum.org/working-groups/server/baseline-requirements/requirements/#7221-crl-issuing-distribution-point
The indirectCRL and onlyContainsAttributeCerts fields MUST be set to FALSE (i.e., not asserted).
The CA MAY set either of the onlyContainsUserCerts and onlyContainsCACerts fields to TRUE, depending on the scope of the CRL.
The CA MUST NOT assert both of the onlyContainsUserCerts and onlyContainsCACerts fields.
So CABF is consistent with 5280 on this, it seems, insofar as onlyContainsAttributeCerts is always FALSE and the other two are always exclusive of each other.
(I think this has no effect on the validator, though, since this extension is for CRLs only and not CAs/leafs?)
Ah, I see... I think your comment about the having no effect on the validator may be true though I am not 100% sure. But, wouldn't this check mean that indirectCRL can never be set to TRUE in the CRL for it to be valid? I basically discovered this when loading the CRL that contains the IDP with indirectCRL, onlyContainsUserCerts set to TRUE.
@reaperhulk do you remember why we did this? just an accident?
Either way, it seems like this change is correct, and if you can fix the tests, we can get this merged.
Yes this appears to be a mistake, so if the tests get fixed here then this fix looks correct.
Hi @alex I can work on fixing the tests. Would you let me know how I can invoke the tests locally?
https://cryptography.io/en/latest/development/getting-started/
On Tue, Sep 24, 2024 at 12:49 AM Han Yu @.***> wrote:
Hi @alex I can work on fixing the tests. Would you let me know how I can invoke the tests locally?
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>
-- All that is necessary for evil to succeed is for good people to do nothing.
Thank you. I shorted the comment line and also removed the invalid test case. I used nox -e local to check. All test cases passed. Could you take a look? Test is still running but I see one failure "CI / distros (ubuntu-rolling:aarch64, tests, self-hosted, Linux, ARM64) (pull_request)". Not sure if this is related to the IDP change. First time doing this, so let me know if I missed anything. Thank you.
@alex Can you confirm if this change will be included in the 44.0.0 release and when that (or the release it will be in) is expected to be live? Thanks!
This change will be in 44. We do not have a date, but it's likely it will be before the end of the year.