-
Notifications
You must be signed in to change notification settings - Fork 101
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
Accomodate pathLen when checking certificate #2853
Conversation
@rw8896 as well. |
Question: How the logic is tested? |
/* basic_constraints from certificate. */ | ||
uint8_t cert_basic_constraints[LIBSPDM_MAX_BASIC_CONSTRAINTS_CA_LEN]; | ||
/* basic_constraints from certificate. Add space for pathLen. */ | ||
uint8_t cert_basic_constraints[LIBSPDM_MAX_BASIC_CONSTRAINTS_CA_LEN + 10]; |
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.
How 10 is calculated? Is that enough?
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.
Both mbedtls and openssl seem to provide APIs to check if CA is true.
https://docs.openssl.org/3.0/man3/X509_check_ca/#copyright
https://github.com/Mbed-TLS/mbedtls/blob/development/include/mbedtls/x509_crt.h#L920
Maybe we could invent another libspdm_x509 API to return if CA is true instead of parsing the basic constraints raw bytes?
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.
How 10 is calculated? Is that enough?
It's probably more than enough, but let me check what the largest ASN.1 DER integer size is. I'll make a macro for it too.
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.
Both mbedtls and openssl seem to provide APIs to check if CA is true. https://docs.openssl.org/3.0/man3/X509_check_ca/#copyright https://github.com/Mbed-TLS/mbedtls/blob/development/include/mbedtls/x509_crt.h#L920
Maybe we could invent another libspdm_x509 API to return if CA is true instead of parsing the basic constraints raw bytes?
The benefit of having the logic in libspdm/library
is that the Integrator doesn't need to provide new functions / functionality as the SPDM specification adds or removes certificate fields. But yes, maybe for libspdm 4.0 we could have a basic certificate parsing interface where the Integrator / cryptography library handles most of the logic, but then also have a catch-all interface where the Integrator provides the raw ASN.1 DER to libspdm.
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.
Thank Steven. Current change looks good and yes, I think the enhancement could wait to be implemented in 4.0
Assuming libspdm/unit_test/test_spdm_responder/set_certificate_rsp.c Lines 543 to 547 in 525ba87
|
Fix DMTF#2831. Signed-off-by: Steven Bellock <[email protected]>
cbff9e5
to
5c6706e
Compare
LGTM |
Sorry that I missed this. Replied okay in the original thread. |
Fix #2831.
Signed-off-by: Steven Bellock [email protected]