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

Simplify X.509 certificate validity check #247

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

mgeisler
Copy link
Contributor

Description of changes:

Refactor of the verify_time code for X.509 certificates: instead of chaining Option values, I used a simple if statement. I find that clearer.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT license.

@mgeisler mgeisler requested a review from a team as a code owner January 23, 2025 14:28
@mgeisler
Copy link
Contributor Author

I found this code while looking at validating X.509 certificates. I would also like to discuss the X509Error::ValidityError(now, format!("{cert:?}")) error returned with a big debug print of the certificate 😄 It doesn't seem super useful?

Would there be appetite for changing X509Error::ValidityError to have fields with:

  • A certitificate serial number (if that's a useful identifier?)
  • The now time passed in
  • The notBefore time
  • The notAfter time

I will need to parse these things myself anyway, so changing this is not strictly necessary for me — but it could perhaps be a nice little cleanup.

@mulmarta
Copy link
Contributor

Yeah definitely having the very large cred in the debug print is suboptimal. I guess time makes sense, as the most likely reason for failing.

@mgeisler
Copy link
Contributor Author

Yeah definitely having the very large cred in the debug print is suboptimal. I guess time makes sense, as the most likely reason for failing.

From what I see, it's the only place this enum variant is constructed, so it should be fine to adjust.

This is now 5 lines of code, but I feel they’re simpler lines than
what came before.
@mgeisler mgeisler force-pushed the refactor-verify-time branch from 5f073e8 to 5e8eefc Compare January 23, 2025 15:01
@mgeisler
Copy link
Contributor Author

Ups, I fixed the formatting mistake... I've somehow managed to disable rustfmt on my work computer 🤦‍♂️

@tomleavy
Copy link
Contributor

Yeah definitely having the very large cred in the debug print is suboptimal. I guess time makes sense, as the most likely reason for failing.

From what I see, it's the only place this enum variant is constructed, so it should be fine to adjust.

Merging this one, feel free to put up another one to change this

@tomleavy tomleavy merged commit 0e6faa4 into awslabs:1.x-main Jan 23, 2025
17 checks passed
@mgeisler
Copy link
Contributor Author

Merging this one, feel free to put up another one to change this

Thank you, that was what I was hoping.

@mgeisler mgeisler deleted the refactor-verify-time branch January 24, 2025 09:22
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.

3 participants