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

Add parameters to libspdm_write_certificate_to_nvm #2901

Merged
merged 2 commits into from
Nov 20, 2024

Conversation

steven-bellock
Copy link
Contributor

Fix #2873.

@steven-bellock steven-bellock marked this pull request as ready for review November 18, 2024 01:03
@steven-bellock
Copy link
Contributor Author

@rw8896 as well.


uint32_t base_hash_algo, uint32_t base_asym_algo
#if LIBSPDM_SET_CERT_PARAMS
, bool *need_reset, bool *is_busy
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if we need is_busy for CSR API as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes probably. I can do that in a separate pull request, but let me rename LIBSPDM_SET_CERT_PARAMS to something more generic so that CSR can leverage it as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Can we file a separate issue? I feel #2893 only handles INVALID_REQUEST.

if ((fp_out = open(file_name, O_WRONLY | O_TRUNC)) == -1) {
printf("Unable to open file %s\n", file_name);
return false;

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to handle the (cert_chain == NULL) case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a separate issue. I just added

    if (g_set_cert_is_busy) {
        *is_busy = true;

and everything got adjusted. But yes, it looks like the "sample" implementation doesn't support erasing a certificate slot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the handling could be done in a separate change.

Copy link
Contributor

@rw8896 rw8896 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@steven-bellock steven-bellock merged commit 6652fc3 into DMTF:main Nov 20, 2024
97 checks passed
@steven-bellock steven-bellock deleted the fix-2873 branch November 20, 2024 22:25
* This macro will be removed when libspdm 4.0 is released.
*/
#ifndef LIBSPDM_SET_CERT_CSR_PARAMS
#define LIBSPDM_SET_CERT_CSR_PARAMS 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not be 1. I'll fix it in the pull request that fixes #2908.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this needs a separate pull request, as it is breaking builds.

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.

Add need_reset parameter to libspdm_write_certificate_to_nvm
3 participants