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.

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

Add PreSpecified(Vec<u8>) option to KeyIdMethod. #197

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

brocaar
Copy link
Contributor

@brocaar brocaar commented Dec 6, 2023

If using from_ca_cert_der/_pem, the key_identifier_method would always be set to Sha256, which is not always true. If using OpenSSL for example SHA1 would be used.

If the provided CA certificate contains a SubjectKeyIdentifier extension, then this option will be automatically set.

Fixes #195.

Copy link
Member

@cpu cpu 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 great, just one nit and a request for a test from me.

rcgen/src/lib.rs Outdated Show resolved Hide resolved
rcgen/src/lib.rs Show resolved Hide resolved
@brocaar
Copy link
Contributor Author

brocaar commented Dec 8, 2023

I've implemented all feedback and all tests are green :) Thanks for helping me with resolving this issue and reviewing this pull-request. Your feedback was really helpful and I learned a few new things!

Once merged, would it be possible to push a new release containing this fix?

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM mod my one suggestion for removing the duplication of the expected SKI in the test.

rcgen/src/lib.rs Outdated Show resolved Hide resolved
rcgen/src/lib.rs Show resolved Hide resolved
@cpu
Copy link
Member

cpu commented Dec 8, 2023

Once merged, would it be possible to push a new release containing this fix?

There's a separate request for this in #198 I think as a project we need to decide whether we want to block that release on finishing some of the in-progress work (e.g. #170, #183, #190).

@cpu cpu mentioned this pull request Dec 8, 2023
4 tasks
@cpu
Copy link
Member

cpu commented Dec 8, 2023

as a project we need to decide whether we want to block that release on finishing some of the in-progress work (e.g. #170, #183, #190).

Tracking this in #199

If using from_ca_cert_der/_pem, the key_identifier_method would always
be set to Sha256, which is not always true. If using OpenSSL for example
SHA1 would be used.

If the provided CA certificate contains a SubjectKeyIdentifier extension,
then this option will be automatically set.

Fixes rustls#195.
@cpu
Copy link
Member

cpu commented Dec 9, 2023

I updated the branch to address my tiny review nit and to rebase on main.

@cpu cpu enabled auto-merge December 9, 2023 15:10
@cpu cpu added this pull request to the merge queue Dec 9, 2023
Merged via the queue into rustls:main with commit 5ed5fcc Dec 9, 2023
17 checks passed
@cpu cpu mentioned this pull request Dec 16, 2023
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.

Generated cert signed by external CA cert returns cert that can't be validated against CA
4 participants