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

validation: add Rust-side certificate validation helpers #9757

Merged
merged 7 commits into from
Oct 26, 2023

Conversation

tetsuo-cpp
Copy link
Contributor

@tetsuo-cpp tetsuo-cpp commented Oct 24, 2023

Breakout from #8873.

@tetsuo-cpp tetsuo-cpp marked this pull request as draft October 24, 2023 07:58
@tetsuo-cpp tetsuo-cpp marked this pull request as ready for review October 25, 2023 07:09
@tetsuo-cpp
Copy link
Contributor Author

@woodruffw This should be good to go. Can you please give this a quick review? Then we can hand the review off to Alex and Paul.

@tetsuo-cpp
Copy link
Contributor Author

I have some WIP to breakout cryptography-x509-validation/src/policy/extension.rs but some of the helpers are dependent on this so let's get this in first.

@tetsuo-cpp
Copy link
Contributor Author

As far as I can tell, the remaining CI failure looks unrelated.

@alex
Copy link
Member

alex commented Oct 25, 2023

re-started it

Comment on lines +43 to +58
fn ca_pem() -> pem::Pem {
// From vectors/cryptography_vectors/x509/custom/ca/ca.pem
pem::parse(
"-----BEGIN CERTIFICATE-----
MIIBUTCB96ADAgECAgIDCTAKBggqhkjOPQQDAjAnMQswCQYDVQQGEwJVUzEYMBYG
A1UEAwwPY3J5cHRvZ3JhcGh5IENBMB4XDTE3MDEwMTEyMDEwMFoXDTM4MTIzMTA4
MzAwMFowJzELMAkGA1UEBhMCVVMxGDAWBgNVBAMMD2NyeXB0b2dyYXBoeSBDQTBZ
MBMGByqGSM49AgEGCCqGSM49AwEHA0IABBj/z7v5Obj13cPuwECLBnUGq0/N2CxS
JE4f4BBGZ7VfFblivTvPDG++Gve0oQ+0uctuhrNQ+WxRv8GC177F+QWjEzARMA8G
A1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDSQAwRgIhANES742XWm64tkGnz8Dn
pG6u2lHkZFQr3oaVvPcemvlbAiEA0WGGzmYx5C9UvfXIK7NEziT4pQtyESE0uRVK
Xw4nMqk=
-----END CERTIFICATE-----",
)
.unwrap()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: maybe put this as a pub(crate) helper under ops::tests, like v1_cert_pem? But not strong opinion.

Comment on lines +70 to +87
struct PublicKeyErrorOps {}
impl CryptoOps for PublicKeyErrorOps {
type Key = ();
type Err = ();

fn public_key(&self, _cert: &Certificate<'_>) -> Result<Self::Key, Self::Err> {
// Simulate failing to retrieve a public key.
Err(())
}

fn verify_signed_by(
&self,
_cert: &Certificate<'_>,
_key: Self::Key,
) -> Result<(), Self::Err> {
Ok(())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +12 to +17
#[allow(dead_code)]
pub(crate) fn cert_is_self_issued(cert: &Certificate<'_>) -> bool {
cert.issuer() == cert.subject()
}

#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

@alex or @reaperhulk can counterindicate, but IMO we could make these pub instead of allow(dead_code) to get around any warnings here (since this crate isn't published anywhere anyways).

Copy link
Contributor

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM overall, a few small thoughts.

@alex alex merged commit 16a969e into pyca:main Oct 26, 2023
56 checks passed
@woodruffw woodruffw deleted the alex/cert-validation-helpers branch October 26, 2023 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants