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 support for enforcing CRL expiration #1922

Merged
merged 2 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions fuzz/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions rustls/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ log = { version = "0.4.4", optional = true }
once_cell = { version = "1.16", default-features = false, features = ["alloc", "race"] }
ring = { version = "0.17", optional = true }
subtle = { version = "2.5.0", default-features = false }
webpki = { package = "rustls-webpki", version = "0.102.2", features = ["alloc"], default-features = false }
pki-types = { package = "rustls-pki-types", version = "1.2", features = ["alloc"] }
webpki = { package = "rustls-webpki", version = "0.102.4", features = ["alloc"], default-features = false }
jasperpatterson marked this conversation as resolved.
Show resolved Hide resolved
pki-types = { package = "rustls-pki-types", version = "1.7", features = ["alloc"] }
zeroize = "1.7"

[features]
Expand Down
28 changes: 24 additions & 4 deletions rustls/examples/internal/test_ca.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,26 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
.unwrap(),
_ => panic!("unexpected role for CRL generation: {role:?}"),
};
let crl = crl_for_serial(

let revoked_crl = crl_for_serial(
cert.params()
.serial_number
.clone()
.unwrap(),
)
.signed_by(&issuer.cert, &issuer.key_pair)?;
let mut crl_file = File::create(
let mut revoked_crl_file = File::create(
alg.output_directory()
.join(format!("{}.revoked.crl.pem", role.label())),
)?;
crl_file.write_all(crl.pem().unwrap().as_bytes())?;
revoked_crl_file.write_all(revoked_crl.pem().unwrap().as_bytes())?;

let expired_crl = expired_crl().signed_by(&issuer.cert, &issuer.key_pair)?;
let mut expired_crl_file = File::create(
alg.output_directory()
.join(format!("{}.expired.crl.pem", role.label())),
)?;
expired_crl_file.write_all(expired_crl.pem().unwrap().as_bytes())?;
}

// When we're issuing end entity or client certs we have a bit of extra work to do
Expand Down Expand Up @@ -125,7 +133,7 @@ fn crl_for_serial(serial_number: SerialNumber) -> CertificateRevocationListParam
let now = OffsetDateTime::now_utc();
CertificateRevocationListParams {
this_update: now,
next_update: now + Duration::from_secs(60 * 60 * 24 * 5),
next_update: now + Duration::from_secs(60 * 60 * 24 * 365 * 100), // 100 years
crl_number: SerialNumber::from(1234),
issuing_distribution_point: None,
revoked_certs: vec![RevokedCertParams {
Expand All @@ -138,6 +146,18 @@ fn crl_for_serial(serial_number: SerialNumber) -> CertificateRevocationListParam
}
}

fn expired_crl() -> CertificateRevocationListParams {
let now = OffsetDateTime::now_utc();
CertificateRevocationListParams {
this_update: now - Duration::from_secs(60),
next_update: now,
crl_number: SerialNumber::from(1234),
issuing_distribution_point: None,
revoked_certs: vec![],
key_identifier_method: KeyIdMethod::Sha256,
}
}

// Note: these are ordered such that the data dependencies for issuance are satisfied.
const ROLES: [Role; 4] = [
Role::TrustAnchor,
Expand Down
6 changes: 5 additions & 1 deletion rustls/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,9 @@ pub enum CertificateError {
/// The certificate's revocation status could not be determined.
UnknownRevocationStatus,

/// The certificate's revocation status could not be determined, because the CRL is expired.
ExpiredRevocationList,

/// A certificate is not correctly signed by the key of its alleged
/// issuer.
BadSignature,
Expand Down Expand Up @@ -358,6 +361,7 @@ impl PartialEq<Self> for CertificateError {
(NotValidForName, NotValidForName) => true,
(InvalidPurpose, InvalidPurpose) => true,
(ApplicationVerificationFailure, ApplicationVerificationFailure) => true,
(ExpiredRevocationList, ExpiredRevocationList) => true,
_ => false,
}
}
Expand All @@ -378,7 +382,7 @@ impl From<CertificateError> for AlertDescription {
Revoked => Self::CertificateRevoked,
// OpenSSL, BoringSSL and AWS-LC all generate an Unknown CA alert for
// the case where revocation status can not be determined, so we do the same here.
UnknownIssuer | UnknownRevocationStatus => Self::UnknownCA,
UnknownIssuer | UnknownRevocationStatus | ExpiredRevocationList => Self::UnknownCA,
BadSignature => Self::DecryptError,
InvalidPurpose => Self::UnsupportedCertificate,
ApplicationVerificationFailure => Self::AccessDenied,
Expand Down
36 changes: 35 additions & 1 deletion rustls/src/webpki/client_verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use alloc::sync::Arc;
use alloc::vec::Vec;

use pki_types::{CertificateDer, CertificateRevocationListDer, UnixTime};
use webpki::{CertRevocationList, RevocationCheckDepth, UnknownStatusPolicy};
use webpki::{CertRevocationList, ExpirationPolicy, RevocationCheckDepth, UnknownStatusPolicy};

use super::{pki_error, VerifierBuilderError};
#[cfg(doc)]
Expand Down Expand Up @@ -30,6 +30,7 @@ pub struct ClientCertVerifierBuilder {
crls: Vec<CertificateRevocationListDer<'static>>,
revocation_check_depth: RevocationCheckDepth,
unknown_revocation_policy: UnknownStatusPolicy,
revocation_expiration_policy: ExpirationPolicy,
anon_policy: AnonymousClientPolicy,
supported_algs: WebPkiSupportedAlgorithms,
}
Expand All @@ -46,6 +47,7 @@ impl ClientCertVerifierBuilder {
anon_policy: AnonymousClientPolicy::Deny,
revocation_check_depth: RevocationCheckDepth::Chain,
unknown_revocation_policy: UnknownStatusPolicy::Deny,
revocation_expiration_policy: ExpirationPolicy::Ignore,
supported_algs,
}
}
Expand Down Expand Up @@ -138,6 +140,19 @@ impl ClientCertVerifierBuilder {
self
}

/// Enforce the CRL nextUpdate field (i.e. expiration)
///
/// If CRLs are provided with [`with_crls`][Self::with_crls] and the verification time is
/// beyond the time in the CRL nextUpdate field, it is expired and treated as an error condition.
/// Overrides the default behavior where expired CRLs are not treated as an error condition.
///
/// If no CRLs are provided then this setting has no effect as revocation status checks
/// are not performed.
pub fn enforce_revocation_expiration(mut self) -> Self {
self.revocation_expiration_policy = ExpirationPolicy::Enforce;
self
}

/// Build a client certificate verifier. The built verifier will be used for the server to offer
/// client certificate authentication, to control how offered client certificates are validated,
/// and to determine what to do with anonymous clients that do not respond to the client
Expand Down Expand Up @@ -165,6 +180,7 @@ impl ClientCertVerifierBuilder {
parse_crls(self.crls)?,
self.revocation_check_depth,
self.unknown_revocation_policy,
self.revocation_expiration_policy,
self.anon_policy,
self.supported_algs,
)))
Expand Down Expand Up @@ -237,6 +253,7 @@ pub struct WebPkiClientVerifier {
crls: Vec<CertRevocationList<'static>>,
revocation_check_depth: RevocationCheckDepth,
unknown_revocation_policy: UnknownStatusPolicy,
revocation_expiration_policy: ExpirationPolicy,
anonymous_policy: AnonymousClientPolicy,
supported_algs: WebPkiSupportedAlgorithms,
}
Expand Down Expand Up @@ -305,6 +322,7 @@ impl WebPkiClientVerifier {
crls: Vec<CertRevocationList<'static>>,
revocation_check_depth: RevocationCheckDepth,
unknown_revocation_policy: UnknownStatusPolicy,
revocation_expiration_policy: ExpirationPolicy,
anonymous_policy: AnonymousClientPolicy,
supported_algs: WebPkiSupportedAlgorithms,
) -> Self {
Expand All @@ -314,6 +332,7 @@ impl WebPkiClientVerifier {
crls,
revocation_check_depth,
unknown_revocation_policy,
revocation_expiration_policy,
anonymous_policy,
supported_algs,
}
Expand Down Expand Up @@ -356,6 +375,7 @@ impl ClientCertVerifier for WebPkiClientVerifier {
.unwrap()
.with_depth(self.revocation_check_depth)
.with_status_policy(self.unknown_revocation_policy)
.with_expiration_policy(self.revocation_expiration_policy)
.build(),
)
};
Expand Down Expand Up @@ -605,6 +625,20 @@ test_for_each_provider! {
builder.build().unwrap();
}

#[test]
fn test_client_verifier_enforce_expiration() {
// We should be able to build a client verifier that allows unknown revocation status
let builder = WebPkiClientVerifier::builder_with_provider(
test_roots(),
provider::default_provider().into(),
)
.with_crls(test_crls())
.enforce_revocation_expiration();
// The builder should be Debug.
println!("{:?}", builder);
builder.build().unwrap();
}

#[test]
fn test_builder_no_roots() {
// Trying to create a client verifier builder with no trust anchors should fail at build time
Expand Down
1 change: 1 addition & 0 deletions rustls/src/webpki/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ fn pki_error(error: webpki::Error) -> Error {
CertNotValidForName => CertificateError::NotValidForName.into(),
CertRevoked => CertificateError::Revoked.into(),
UnknownRevocationStatus => CertificateError::UnknownRevocationStatus.into(),
CrlExpired => CertificateError::ExpiredRevocationList.into(),
IssuerNotCrlSigner => CertRevocationListError::IssuerInvalidForCrl.into(),

InvalidSignatureForPublicKey
Expand Down
37 changes: 36 additions & 1 deletion rustls/src/webpki/server_verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use alloc::sync::Arc;
use alloc::vec::Vec;

use pki_types::{CertificateDer, CertificateRevocationListDer, ServerName, UnixTime};
use webpki::{CertRevocationList, RevocationCheckDepth, UnknownStatusPolicy};
use webpki::{CertRevocationList, ExpirationPolicy, RevocationCheckDepth, UnknownStatusPolicy};

use crate::crypto::{CryptoProvider, WebPkiSupportedAlgorithms};
#[cfg(feature = "logging")]
Expand All @@ -28,6 +28,7 @@ pub struct ServerCertVerifierBuilder {
crls: Vec<CertificateRevocationListDer<'static>>,
revocation_check_depth: RevocationCheckDepth,
unknown_revocation_policy: UnknownStatusPolicy,
revocation_expiration_policy: ExpirationPolicy,
supported_algs: WebPkiSupportedAlgorithms,
}

Expand All @@ -41,6 +42,7 @@ impl ServerCertVerifierBuilder {
crls: Vec::new(),
revocation_check_depth: RevocationCheckDepth::Chain,
unknown_revocation_policy: UnknownStatusPolicy::Deny,
revocation_expiration_policy: ExpirationPolicy::Ignore,
supported_algs,
}
}
Expand Down Expand Up @@ -83,6 +85,19 @@ impl ServerCertVerifierBuilder {
self
}

/// Enforce the CRL nextUpdate field (i.e. expiration)
///
/// If CRLs are provided with [`with_crls`][Self::with_crls] and the verification time is
/// beyond the time in the CRL nextUpdate field, it is expired and treated as an error condition.
/// Overrides the default behavior where expired CRLs are not treated as an error condition.
///
/// If no CRLs are provided then this setting has no effect as revocation status checks
/// are not performed.
pub fn enforce_revocation_expiration(mut self) -> Self {
self.revocation_expiration_policy = ExpirationPolicy::Enforce;
self
}

/// Build a server certificate verifier, allowing control over the root certificates to use as
/// trust anchors, and to control how server certificate revocation checking is performed.
///
Expand All @@ -107,6 +122,7 @@ impl ServerCertVerifierBuilder {
parse_crls(self.crls)?,
self.revocation_check_depth,
self.unknown_revocation_policy,
self.revocation_expiration_policy,
self.supported_algs,
)
.into())
Expand All @@ -121,6 +137,7 @@ pub struct WebPkiServerVerifier {
crls: Vec<CertRevocationList<'static>>,
revocation_check_depth: RevocationCheckDepth,
unknown_revocation_policy: UnknownStatusPolicy,
revocation_expiration_policy: ExpirationPolicy,
supported: WebPkiSupportedAlgorithms,
}

Expand Down Expand Up @@ -167,6 +184,7 @@ impl WebPkiServerVerifier {
Vec::default(),
RevocationCheckDepth::Chain,
UnknownStatusPolicy::Allow,
ExpirationPolicy::Ignore,
supported_algs,
)
}
Expand All @@ -187,13 +205,15 @@ impl WebPkiServerVerifier {
crls: Vec<CertRevocationList<'static>>,
revocation_check_depth: RevocationCheckDepth,
unknown_revocation_policy: UnknownStatusPolicy,
revocation_expiration_policy: ExpirationPolicy,
supported: WebPkiSupportedAlgorithms,
) -> Self {
Self {
roots: roots.into(),
crls,
revocation_check_depth,
unknown_revocation_policy,
revocation_expiration_policy,
supported,
}
}
Expand Down Expand Up @@ -234,6 +254,7 @@ impl ServerCertVerifier for WebPkiServerVerifier {
.unwrap()
.with_depth(self.revocation_check_depth)
.with_status_policy(self.unknown_revocation_policy)
.with_expiration_policy(self.revocation_expiration_policy)
.build(),
)
};
Expand Down Expand Up @@ -413,4 +434,18 @@ test_for_each_provider! {
println!("{:?}", builder);
builder.build().unwrap();
}

#[test]
fn test_server_verifier_enforce_expiration() {
// We should be able to build a server cert. verifier that allows unknown revocation
// status.
let builder = WebPkiServerVerifier::builder_with_provider(
test_roots(),
provider::default_provider().into(),
)
.enforce_revocation_expiration();
// The builder should be Debug.
println!("{:?}", builder);
builder.build().unwrap();
}
}