-
Notifications
You must be signed in to change notification settings - Fork 0
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 more explicit auto id tests #11
base: main
Are you sure you want to change the base?
Conversation
PR Description updated to latest commit (9f1e74d) |
PR Review 🔍
Code feedback:
|
PR Code Suggestions ✨
|
9f1e74d
to
d39145e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, nothing blocking!
# for child certificate, auto_id of the issuer is included in the data | ||
child_certificate = self_issuer.issue_certificate(self_issuer.create_and_sign_csr("child")) | ||
|
||
assert CertificateManager.get_certificate_auto_id(child_certificate) == blake2b_256(CertificateManager.get_certificate_auto_id( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These assertions are a bit long, consider a slight refactor
assert CertificateManager.get_certificate_auto_id(child_certificate) == blake2b_256(CertificateManager.get_certificate_auto_id( | |
computed_auto_id = blake2b_256(CertificateManager.get_certificate_auto_id(certificate).encode() + self_issuer.get_subject_common_name(child_certificate.subject).encode()).hex() | |
assert CertificateManager.get_certificate_auto_id(child_certificate) == computed_auto_id |
child_certificate = self_issuer.issue_certificate(self_issuer.create_and_sign_csr("child")) | ||
|
||
assert CertificateManager.get_certificate_auto_id(child_certificate) == blake2b_256(CertificateManager.get_certificate_auto_id( | ||
certificate).encode() + self_issuer.get_subject_common_name(child_certificate.subject).encode()).hex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
certificate).encode() + self_issuer.get_subject_common_name(child_certificate.subject).encode()).hex() |
|
||
issuer_auto_id = CertificateManager.get_certificate_auto_id(certificate) | ||
assert issuer_auto_id == blake2b_256(self_issuer.get_subject_common_name(certificate.subject).encode()).hex() | ||
assert issuer_auto_id == "8d2143d76615c515b5cc88fa7806aef268edeea87571c8f8b21a19f77b9993ba" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't familiar where the hardcoded hash was coming from, until I've done a quick Googling. While this is not a huge problem now, may be confusing in a long run.
I would suggest to introduce two constants "8d2143d76615c515b5cc88fa7806aef268edeea87571c8f8b21a19f77b9993ba" and "d1575259a7e413f51e24ece5e353f5015fb0e39e6738b2044a3d7a8c3bc11114" closer to beginning of the file or function, and highlight that they're coming from the blake2b_256 library.
Maybe something like EXPECTED_SELF_ISSUED_CERT_BLAKE2B_HASH and EXPECTED_CHILD_CERT_BLAKE2B_HASH could work good, wdyt?
PR Type
enhancement, tests
Description
test_auto_id_deterministic
to ensure deterministic auto ID generation for certificates using theblake2b_256
hashing function.Changes walkthrough 📝
test_certificate_manager.py
Add Deterministic Auto ID Generation Tests for Certificates
tests/auto_identity_tests/test_certificate_manager.py
blake2b_256
hashing function.test_auto_id_deterministic
to verify deterministicauto ID generation for certificates.
both issuer and child certificates using
blake2b_256
.