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 certificate revocation #38

Merged
merged 4 commits into from
Jan 29, 2024
Merged

Add certificate revocation #38

merged 4 commits into from
Jan 29, 2024

Conversation

Moudoux
Copy link
Contributor

@Moudoux Moudoux commented Jan 24, 2024

Adds certificate revocation, though I'm not entirely sure if the impl should be located in the types.rs.

Copy link
Owner

@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.

Thanks for adding this! Have some thoughts on how to expose this in the API.

src/types.rs Outdated
/// The reason for a certificate revocation
/// Defined in <https://datatracker.ietf.org/doc/html/rfc5280#section-5.3.1>
#[allow(missing_docs)]
pub enum RevocationReason {
Copy link
Owner

@djc djc Jan 25, 2024

Choose a reason for hiding this comment

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

Style nit: please move this below CertificateRevocation instead.

src/types.rs Outdated
/// Payload for a certificate revocation request
/// Defined in <https://datatracker.ietf.org/doc/html/rfc8555#section-7.6>
#[derive(Debug, Serialize)]
pub struct CertificateRevocation {
Copy link
Owner

@djc djc Jan 25, 2024

Choose a reason for hiding this comment

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

Nit: I think I'd prefer calling this just Revocation. The Certificate prefix seems superfluous? Or maybe RevocationRequest?

src/types.rs Outdated
/// The certificate should not contain a header or footer or line breaks
pub certificate: String,
/// Reason for revocation
pub reason: Option<i32>,
Copy link
Owner

Choose a reason for hiding this comment

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

This should contain an actual RevocationReason, and we should convert it to i32 during serialization. I would suggest making the code explicit in the enum (Unspecified = 0), give the enum a #[repr(u8)], then make a manual impl Serialize for RevocationReason that just calls serializer.serialize_u8(self as u8).

src/types.rs Outdated
impl CertificateRevocation {
/// Create a new revocation request from a certificate.
/// The certificate cannot be a chain.
pub fn new(certificate: &str, reason: Option<RevocationReason>) -> CertificateRevocation {
Copy link
Owner

@djc djc Jan 25, 2024

Choose a reason for hiding this comment

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

If we're going to do a bunch of processing here on the certificate &str, it should not be a pub field. Instead of having this constructor on CertificateRevocation (and making the fields private), I would suggest that we add a dependency on rustls-pki-types, take a CertificateDer (which can decoded from PEM using the rustls-pemfile crate -- which can be handled by the caller) and then base64url encode it during serialization with another custom Serialize impl. What do you think?

@Moudoux
Copy link
Contributor Author

Moudoux commented Jan 27, 2024

I've done most of the changes. The RevocationRequest::new method now accepts a CertificateDer which is encoded to base64 url safe in the new method. It is not possible to implement a foreign trait for a foreign struct afaik.

@djc
Copy link
Owner

djc commented Jan 29, 2024

Sorry, I meant that we should (also) have a custom Serialize impl for RevocationRequest. If we leave the field as CertificateDer<'a> we can also just lose the constructor and keep the fields pub.

Copy link
Owner

@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.

LGTM, have you tested that this works?

@Moudoux
Copy link
Contributor Author

Moudoux commented Jan 29, 2024

Yes, I have tested revoking a generated certificate from LetsEncrypt staging

@djc djc merged commit 8290a95 into djc:main Jan 29, 2024
7 checks passed
@djc
Copy link
Owner

djc commented Jan 29, 2024

I've released this as 0.4.2.

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