Skip to content

Remember certificate policies extensions but do nothing further#211

Closed
kikuomax wants to merge 2 commits intorustls:mainfrom
codemonger-io:policy-handling
Closed

Remember certificate policies extensions but do nothing further#211
kikuomax wants to merge 2 commits intorustls:mainfrom
codemonger-io:policy-handling

Conversation

@kikuomax
Copy link
Copy Markdown

Deal with certificate policies extensions so that rustls-webpki won't fail when it faces a critical certificate policies extensions.

This PR makes rustls-webpki remember the values of the certificate policies extensions it faces but do nothing further. This change lets rustls-webpki accept certificates with invalid critical certificate policies extensions, though, this should be at least equivalent to OpenSSL's default behavior.

If one wants to verify the policy tree, can use verify_path option to do so. You can find an example in my fork.

- Introduces a new test case `win_hello_attest_tpm` that tests
  `verify_for_usage` with certificates including a critical non-any
  certificate policy extension. The test fixtures were taken from
  `core::tests::win_hello_attest_tpm` of `webauthn-rs`.
- `Cert` introduces a new field `certificate_policies` for an optional
  "Certificate Policies" extension (RFC 5280 4.2.1.4).
- `remember_cert_extension` remembers the extension data in
  `Cert::certificate_policies`.
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eed037b) 97.21% compared to head (0471312) 97.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #211      +/-   ##
==========================================
+ Coverage   97.21%   97.23%   +0.02%     
==========================================
  Files          19       19              
  Lines        4266     4268       +2     
==========================================
+ Hits         4147     4150       +3     
+ Misses        119      118       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

djc
djc previously approved these changes Nov 27, 2023
Copy link
Copy Markdown
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks okay to me.

If the test depends on the newly introduced behavior (not clear to me if it does?), I suggest that we switch the order of commits such that all commits can pass tests (since we rebase). (I suppose we could also just squash these commits.)

@ctz
Copy link
Copy Markdown
Member

ctz commented Nov 27, 2023

I don't think we should do this. The default openssl behaviour is incorrect and drives a bus through the intended meaning of extension criticality.

What do the BRs say about the cert policy extension?

@djc djc dismissed their stale review November 27, 2023 08:34

Remove approval pending Joe's concerns

@kikuomax kikuomax marked this pull request as draft November 27, 2023 13:48
@kikuomax
Copy link
Copy Markdown
Author

I don't think we should do this. The default openssl behaviour is incorrect and drives a bus through the intended meaning of extension criticality.

I understand your concern. Integrity should not be compromised for a very specific use case.
@djc @ctz Is there any chance to have this as an opt-in feature like accept-invalid-policies?

Anyway, please reject this PR if it is not relevant.

@kikuomax
Copy link
Copy Markdown
Author

If the test depends on the newly introduced behavior (not clear to me if it does?), I suggest that we switch the order of commits such that all commits can pass tests (since we rebase). (I suppose we could also just squash these commits.)

@djc The order of commits reflected my test-driven approach. I will reorder them if this PR would be accepted.

@djc
Copy link
Copy Markdown
Member

djc commented Nov 27, 2023

What exactly is the use case for this? Why would someone want to ignore these invalid extensions?

@kikuomax
Copy link
Copy Markdown
Author

What exactly is the use case for this? Why would someone want to ignore these invalid extensions?

@djc My use case is not client-server communication but server-side verification of signatures signed by authenticator devices in the context of Web Authentication. I commented in Issue #30:

The certificate policies in the certificate chain I attached to the above comment are valid, but the end entity certificate contains a critical certificate policy extension:

            X509v3 Certificate Policies: critical
                Policy: 1.3.6.1.4.1.311.21.31
                  User Notice:
                    Explicit Text:

I did not mean to make it accept invalid extensions, however, I need more study to propose comprehensive validation.

@cpu
Copy link
Copy Markdown
Member

cpu commented Jan 25, 2024

I think we should close this PR for now. I agree with ctz's earlier assessment.

@cpu cpu closed this Jan 25, 2024
@kikuomax
Copy link
Copy Markdown
Author

@cpu Sorry for a stale PR. I would like to come back to this issue later.

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.

Support anyPolicy Certificate Policy

4 participants