-
Notifications
You must be signed in to change notification settings - Fork 381
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
tpm2_getekcertificate: add support to high range NV indexes #3440
tpm2_getekcertificate: add support to high range NV indexes #3440
Conversation
47bf88c
to
ad1da5a
Compare
@loicsikidi your PR would change the behaviour of |
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.
If you would change the lookup part as follows:
ctx.rsa_ek_cert_nv_location = 0xffffffff;
ctx.ecc_ek_cert_nv_location = 0xffffffff;
for (i = 0; i < capability_data->data.handles.count; i++) {
TPMI_RH_NV_INDEX index = capability_data->data.handles.handle[i];
const ek_index_map *m = lookup_ek_index_map(index);
if (!m) {
continue;
}
if (m->key_type == KTYPE_RSA && index < ctx.rsa_ek_cert_nv_location) {
LOG_INFO("Found pre-provisioned RSA EK certificate at %u [type=%s]", index, m->name);
ctx.is_rsa_ek_cert_nv_location_defined = true;
ctx.rsa_ek_cert_nv_location = m->index;
}
if (m->key_type == KTYPE_ECC && index < ctx.ecc_ek_cert_nv_location) {
LOG_INFO("Found pre-provisioned ECC EK certificate at %u [type=%s]", index, m->name);
ctx.is_ecc_ek_cert_nv_location_defined = true;
ctx.ecc_ek_cert_nv_location = m->index;
}
}
it's ensured that the certificate would be written as previously. The high range certificates would only be written if no low level certificates exist. After your PR is merges I will add a PR with the -u option to enable saving the EK certificate based on the public EK.
I think that's a better solution than allowing e.g. four -o options to save all certificates.
Signed-off-by: loic.sikidi <[email protected]>
Signed-off-by: loic.sikidi <[email protected]>
ad1da5a
to
57263cf
Compare
@JuergenReppSIT I've added your suggestion and one more test to attest that if there are >= 2 certs of the same type => the low range one must be returned. Thanks for the feedback! I wasn't aware that some TPMs have more than one EK cert from the same family 😅 |
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.
@loicsikidi Thank you for the PR. I have already prepared the -u change.
@loicsikidi Sorry I was a bit hasty in approving the PR, because tests for the change I did require worked for me in my local docker imgages. The test getekcertificate.sh now fails with your current PR. I have to investigate what's the difference between your PR an my branch where the test runs without problems. |
@JuergenReppSIT I think that I've found the issue 💡 . The last test is failing because there are 2 "eligible" EK certs in NVRAM but there is only one ## Make sure that if there are several certificates of the same type, then the one belonging to low range has priority
openssl x509 -in ecc_ek_cert.bin -out ecc_low_range_ek_cert.der -outform DER
define_ek_cert_nv_index ecc_low_range_ek_cert.der $ECC_EK_CERT_NV_INDEX
tpm2 getekcertificate -o nv_ecc_ek_cert.der
diff nv_ecc_ek_cert.der ecc_low_range_ek_cert.der ERROR:
My bad..., I'll push a fix 🚀 |
Signed-off-by: loic.sikidi <[email protected]>
I think that we can retry 💪 |
@loicsikidi Thank you for the fix. |
Fixes #3435.
Note:
https://ekop.intel.com
(e.g. RSA 3072 or ECC P384)