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

feature: pem certificate support #19

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Conversation

Alexey-N-Chernyshov
Copy link
Contributor

@Alexey-N-Chernyshov Alexey-N-Chernyshov commented Dec 7, 2022

Adds CERTIFICATE tag for pem files.
RSA tests were extended with certificates.
Self-signed certificates were generated:

# openssl req -new -key private.pem -out certificate.csr 
# openssl x509 -req -days 358000 -in certificate.csr -signkey private.pem -out certificate.crt

certificate.crt files may be checked at https://www.sslchecker.com/certdecoder

Adds CERTIFICATE tag for pem files.
RSA tests were extended with certificates.
Self-signed certificates were generated:

# openssl req -new -key private.pem -out certificate.csr
# openssl x509 -req -days 358000 -in certificate.csr -signkey private.pem -out certificate.crt

certificate.crt files may be checked at https://www.sslchecker.com/certdecoder
tests/rsa/mod.rs Outdated Show resolved Hide resolved
@rib
Copy link
Owner

rib commented Dec 8, 2022

Thanks this looks good to me, with just one comment about whether it's really valid to differentiate between pkcs#1 and pkcs#8 for these certificates vs just referring to them as "pem".

I'll also link to this issue: Keats/jsonwebtoken#259 and this PR Keats/jsonwebtoken#274 with equivalent changes that help add relevant context here.

@Alexey-N-Chernyshov
Copy link
Contributor Author

I there anything you want me to change in this PR?

@rib
Copy link
Owner

rib commented Dec 13, 2022

I there anything you want me to change in this PR?

Is there a practical difference between round_trip_sign_verification_certificate_pem_pkcs1 and round_trip_sign_verification_certificate_pem_pkcs8 apart from testing with different certificate data?

As I understand it they test the same thing (it doesn't really matter that the input keys to create the certificates were in pkcs1 vs pkcs8?).

If that's the case then we can remove one of the tests and just add a single .crt and .csr pair.

@Alexey-N-Chernyshov
Copy link
Contributor Author

I removed the test, but left the name certificate_rsa_pkcs1 to indicate it is generated with rsa_pkcs1 keys.

@Alexey-N-Chernyshov
Copy link
Contributor Author

Can I ask you to make a release when the PR is merged?

@rib
Copy link
Owner

rib commented Dec 14, 2022

Thanks this looks good to me.

Yeah, I'm travelling at the moment but will try to spin a release after merging this.

I'd like to add a CHANGELOG and double check that it's only a semver patch bump.

Since it looks like the libs team came to a good conclusion here rust-lang/rust#103763 (comment) then I'm also inclined to revert back to simply using Arc::ptr_eq here:

// WARNING:
(and disable the clippy warning)

@rib rib merged commit 55f1761 into rib:master Dec 14, 2022
@Alexey-N-Chernyshov Alexey-N-Chernyshov deleted the certificates branch August 1, 2023 06:10
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.

2 participants