add Slot management - #3637
Conversation
9a3c5e5 to
5aee579
Compare
alistair23
left a comment
There was a problem hiding this comment.
It looks like there are some issues with syncing the data back to the current SPDM state
| static libspdm_slot_management_sample_bank_t | ||
| m_slot_management_bank[LIBSPDM_SAMPLE_SLOT_MANAGEMENT_BANK_COUNT]; | ||
| static uint8_t m_slot_management_bank_count = 0; | ||
| static bool m_slot_management_bank_initialized = false; |
There was a problem hiding this comment.
This seems really un-ideal. We need a bunch of state that has to be stored as a global variable as we can't store it in the actual SPDM context
There was a problem hiding this comment.
I understand. But that is current design for key_pair info as well.
I feel that slot_management and key_pair_info are tangled together.
It is really hard to put everything into SPDM context, unless we totally redesign them.
But I am open for suggestions, since 4.0 can have incompatible change.
I just thought that we had discussed before in the meeting - which part is in spdm-context, which part is left to integrator.
But we have not written down that clearly.
I think it is time for us to record the design principle in doc dir.
There was a problem hiding this comment.
I understand. But that is current design for key_pair info as well.
I feel that slot_management and key_pair_info are tangled together.
I would argue that doesn't mean we can't improve things here and then improve things with key_pair afterwards.
It is really hard to put everything into SPDM context, unless we totally redesign them.
We can pretty easily allow the implementation to store a void * in the context (if that isn't already supported?), then at least it doesn't have to be a global.
But I am open for suggestions, since 4.0 can have incompatible change.
#3629 :)
There was a problem hiding this comment.
I would argue that doesn't mean we can't improve things here and then improve things with key_pair afterwards.
Sure, we can improve the key_pair afterwards. I am not against it.
I point it to ensure that, when the new solution is designed, we consider all parts, instead of just part of it.
Also, there is a restriction. The HAL API cannot be changed in minor version update.
That means, if we want to change HAL for key_pair_info, we should either change now (in 4.0), or much later (in 5.0).
We can pretty easily allow the implementation to store a void * in the context (if that isn't already supported?), then at least it doesn't have to be a global.
Again, I cannot say good or bad, before I see a full solution.
Please present the whole solution, and how you plan to use void *.
There was a problem hiding this comment.
Again, I cannot say good or bad, before I see a full solution. Please present the whole solution, and how you plan to use void *.
LIBSPDM_DATA_APP_CONTEXT_DATA already does this. These globals should at least be moved under a LIBSPDM_DATA_APP_CONTEXT_DATA struct
| /* Erase: mark the slot's certificate chain as removed. Subsequent GetBankInfo/ | ||
| * GetBankDetails/GetCertificateChain for this (Bank, slot) will report it as empty. */ | ||
| slot->erased = true; |
There was a problem hiding this comment.
But what if this bank is the actual bank in use? We now need to also push this back to libspdm if this bank is in use, based on the algorithms negotiated
There was a problem hiding this comment.
That is good point.
Same question for key_pair_info.
| bool libspdm_write_slot_management_bank( | ||
| void *spdm_context, | ||
| uint8_t bank_id, | ||
| uint8_t operation, | ||
| uint32_t select_asym_algo, | ||
| uint32_t select_pqc_asym_algo) |
There was a problem hiding this comment.
What if the responder requires a reset? We have no way to propagate that up
There was a problem hiding this comment.
Good point.
Will consider that. Maybe use the same way as others reset_required command.
There was a problem hiding this comment.
This is fixed now. See the new libspdm_write_slot_management_bank() API.
| /* The certificate chain is selected by the Bank's algorithm and the Bank-local SlotID. It | ||
| * is read on demand and does not need to be provisioned into the SPDM context. */ | ||
| if (!libspdm_slot_management_read_slot_cert_chain( | ||
| context, bank, slot, &slot_cert_chain, &slot_cert_chain_size)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This won't work though. If you are using a SLOT_MANAGEMENT command to read the current in use bank it's possible that the chain was set using SetCertificate. So you need to check with the SPDM context to ensure it hasn't been changed.
There was a problem hiding this comment.
libspdm_write_certificate_to_nvm() is supposed to do this now, as it takes a bank_id, but it isn't wired up in the example. The GET_CSR helper also isn't wired up to use these management commands.
I understand that this is just a sample, but as it is the slot management doesn't look like it'll work correctly. If someone does some SLOT_MANAGEMENT on an active bank (which appears to be allowed in the spec) it will fall apart.
I do feel that managing the banks manually is really tricky (hence #3629)
There was a problem hiding this comment.
In general, I agree with you that it is tricky, especially when we touch the active bank.
I think the current code for multi-key/key_info is also broken. For example, #3638.
SetCert is NOT synced to local context yet. For example, #873.
And SlotState is not fully supported. For example, #3645.
To address your concern and current limitation, I think we need a clear link between: slot/bank/algo/key_pair, and that involves multiple commands such as DIGEST/CERT/SET_CERT/KEY_PAIR_INFO/SLOT_MANAGEMENT.
Then we need ensure one change can be propagated to other setting.
(It is like a small database.)
The problem of #3629 is just a very small piece of a whole solution.
But I have no idea on what the whole solution looks like.
That is why I asked for a design review to understand more.
Or, please submit a full solution of slot_management for review, then I can understand how it works.
There was a problem hiding this comment.
Yeah, #3629 is the first step. It's split out to make it easy to review and rebase (it already needs a rebase). I'm a little worried about keeping it rebased on master, as it touches lots of code.
I can work on more features, but I wanted at least a "this looks ok" before I spend more time on it.
I'm also not sure what a design review is.
I can pull some of the slot management out of this PR and add at least an example of how it could work with #3629 next week
There was a problem hiding this comment.
I can work on more features, but I wanted at least a "this looks ok" before I spend more time on it.
I have to say honestly, that I cannot say "this looks ok" before I see a full picture of the solution.
For me, it is hard to approve partial of the solution, without understanding the whole solution.
It is dead-lock.
I'm also not sure what a design review is.
Design review means to describe the whole solution, instead of a piece of it.
Then people can ask question and provide feedback, even before you make implementation.
You can use PPT, you can use pseudo-code, or you can submit a full solution.
The form does not matter. It is content that matters, and that is the full solution in your mind.
03b204b to
e286e37
Compare
|
@alistair23 If you created PPT or some pseudo-code to describe the whole design, you may create a https://github.com/DMTF/libspdm/discussions item. Then we can have more discussion for non-code content. I have created #3646 for my analysis and description. Feel free to ask, if you have any question on design review. |
3456320 to
556b561
Compare
53d2dc8 to
4226ccb
Compare
Add the SPDM 1.4 SLOT_MANAGEMENT / SLOT_MANAGEMENT_RESP request and response structures, the SubCode value defines, the SLOT_MGMT_CAP extended capability flag, the SPDM_MAX_BANK_COUNT define (BankID is 0 to 239), and the SubCode-dependent structures for all defined SubCodes: SupportedSubCodes, SlotAddress, BankInfo/BankElement, BankDetails/SlotElement, GetCertificateChain, ManageBank, ManageSlot, GetCSR, CSR, and SetCertificate. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
Add the responder HAL hooks for the SLOT_MANAGEMENT SubCodes: libspdm_read_slot_management_supported_subcodes, libspdm_read_slot_management_bank_info (writes the BankElements directly into the response buffer, so the responder imposes no fixed maximum Bank count), libspdm_read_slot_management_bank_details (per-slot info for a Bank, including the certificate chain digest), libspdm_read_slot_management_certificate_chain (per Bank+slot; the certificate chain is selected by the Bank's configured algorithm and need not be provisioned into the SPDM context), and the write hooks libspdm_write_slot_management_bank (ManageBank) and libspdm_write_slot_management_slot (ManageSlot). The SetCertificate SubCode reuses the existing SET_CERTIFICATE HAL hook libspdm_update_local_cert_chain. Per DSP0274 Table 147 the SetCertificate Certificate field is a full certificate chain and the SubCode conforms to SET_CERTIFICATE, so no separate raw-write hook is introduced. To address a Bank, libspdm_update_local_cert_chain gains a bank_id parameter, passed as a pointer: NULL for the legacy SET_CERTIFICATE flow (no Bank concept), or a pointer to the BankID for SLOT_MANAGEMENT. A pointer avoids a reserved sentinel value in the public HAL. The GetCSR SubCode reuses the existing GET_CSR HAL hook (libspdm_gen_csr) unchanged, with no bank_id parameter: the CSR is generated from the connection's negotiated algorithm, so Bank addressing would be redundant to the integrator. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
Add the LIBSPDM_ENABLE_CAPABILITY_SLOT_MGMT_CAP config switch, the internal common-lib declaration, and the SLOT_MANAGEMENT / SLOT_MANAGEMENT_RESP command-name map entries. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
Add the requester functions for the SLOT_MANAGEMENT SubCodes: libspdm_slot_management_get_supported_subcodes, _get_bank_info, _get_bank_details, _get_certificate_chain, _manage_bank, _manage_slot, _get_csr, and _set_certificate. Each sends SLOT_MANAGEMENT with its SubCode and parses the SLOT_MANAGEMENT_RESP, gated by SLOT_MGMT_CAP. The GetCSR and SetCertificate APIs mirror libspdm_get_csr_ex and libspdm_set_certificate, with the added ability to address a Bank; _set_certificate also takes a KeyPairID (Table 147), zero when MULTI_KEY_CONN_RSP is false and non-zero otherwise. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
da1f939 to
5251b32
Compare
Add libspdm_get_response_slot_management, which validates the request and dispatches on the SLOT_MANAGEMENT SubCode to a dedicated per-SubCode handler: SupportedSubCodes, GetBankInfo, GetBankDetails, GetCertificateChain, ManageBank, ManageSlot, GetCSR, and SetCertificate. Other SubCodes return ERROR(UnsupportedRequest). GetCertificateChain reads the certificate chain through the HAL, and the GetBankDetails slot digest is provided by the HAL over that same chain. SetCertificate conforms to SET_CERTIFICATE (DSP0274 Table 142). It rejects an unknown BankID up front with ERROR(InvalidRequest) (like the other SubCodes), then applies the same KeyPairID / CertModel validation base SET_CERTIFICATE does: with MULTI_KEY_CONN_RSP the KeyPairID shall be non-zero and CertModel a valid non-NONE model, otherwise both shall be zero and the effective model is derived from ALIAS_CERT_CAP. It verifies the certificate chain with the same libspdm_set_cert_verify_certchain() helper base uses (made non-static for reuse, gated by LIBSPDM_CERT_PARSE_SUPPORT; a failed verify returns ERROR(Unspecified) and retains the existing certificate), then reuses the SET_CERTIFICATE HAL hook libspdm_update_local_cert_chain, passing a pointer to the addressed BankID (the legacy SET_CERTIFICATE responder passes NULL); a HAL write failure is mapped Busy -> BUSY, otherwise -> Unspecified, matching the base SET_CERTIFICATE install path. GetCSR reuses the GET_CSR HAL hook libspdm_gen_csr unchanged (no Bank parameter; the CSR is generated from the negotiated algorithm). Register the handler in the responder dispatch table, gated by SLOT_MGMT_CAP. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
Add a sample implementation of the SLOT_MANAGEMENT HAL hooks. The Banks are modeled per the specification: a Bank is the set of certificate slots that use one asymmetric algorithm, and each slot is associated with a key pair (KeyPairID). The Banks are derived from the key pairs reported by GET_KEY_PAIR_INFO, grouped by their configured algorithm, so a Bank can contain multiple slots whose key pairs all use the Bank's algorithm. A Bank records its key pair's algorithm capabilities (the algorithms it could be configured to), not just its current algorithm. The Bank's certificate chain and slot digest are read on demand via the per-slot certificate read, selected by the Bank's algorithm (traditional or PQC); a slot is reported only if its certificate chain is readable. Per-slot KeyPairID is reported when MULTI_KEY_CONN_RSP is set. BankDetails reports the Bank's stored AsymAlgoCapabilities/PqcAsymAlgoCapabilities, with AvailableAsymAlgo/AvailablePqcAsymAlgo being those Capabilities minus any algorithm already assigned to another Bank (DSP0274 Table 151). ManageBank ConfigAlgo is an idempotent no-op for the Bank's own algorithm; a different algorithm is accepted only if the device can back it and -- as a backstop to the responder's Select-in-Available check -- it is not already assigned to another Bank, and the Bank's slots are unprovisioned (else InvalidState). ManageSlot Erase marks a slot's certificate chain removed. ConfigAlgo is reported, the Selected attribute is computed from the negotiated algorithm, and the supported SubCode bit map and Bank attributes are overridable globals. libspdm_update_local_cert_chain gains the bank_id pointer parameter. The sample keeps libspdm_write_certificate_to_nvm static (a per-(Bank,slot) NVM file) and, via slot_management_internal.h, classifies the addressed Bank: the legacy flow (bank_id NULL) and a write to the currently selected Bank behave exactly like base SET_CERTIFICATE (legacy NVM file plus in-memory local_cert_chain_provision refresh), while a write to a non-selected Bank is stored only in that Bank's NVM file and is not reflected in the SPDM context. An unknown Bank fails. Null stubs are also added/updated. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
Add requester tests for the SLOT_MANAGEMENT SubCodes: SupportedSubCodes (success and ERROR paths), GetBankInfo, GetBankDetails, GetCertificateChain, ManageBank, ManageSlot, GetCSR, and SetCertificate. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
Add responder tests for the SLOT_MANAGEMENT SubCodes: SupportedSubCodes (success and error paths), GetBankInfo, GetBankDetails, GetCertificateChain, ManageBank (incl. consistency with GET_KEY_PAIR_INFO), ManageSlot (Erase), GetCSR, and SetCertificate. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
| /* An all-zero ConfigAlgo (no bit set in SelectAsymAlgo or SelectPqcAsymAlgo) selects no | ||
| * algorithm. Per the "no more than one" reading, it is accepted -- not rejected -- and CLEARS | ||
| * the Bank's algorithm (returns it to the "not yet selected" state). The Bank's slots were | ||
| * erased above, so this configuration change succeeds with a SLOT_MANAGEMENT_RESP (no response | ||
| * structure). */ |
There was a problem hiding this comment.
This is incorrect, as discussed here: https://github.com/DMTF/SPDM-WG/issues/4558
| * consistent with the CurrentAsymAlgo "not yet selected" state. An all-zero selection is | ||
| * therefore accepted: it clears the Bank's algorithm, returning it to the not-yet-selected | ||
| * state, which -- like any Bank configuration change -- requires the Bank's slots to be | ||
| * unprovisioned.) More than one bit total is malformed and rejected. */ |
There was a problem hiding this comment.
The spec is unclear, but this is not correct and will be updated in the future: https://github.com/DMTF/SPDM-WG/issues/4558
|
#3668 has been updated to include the fixes commented on above as well |
Add doc/slot_management.md describing the SPDM 1.4 SLOT_MANAGEMENT feature in libspdm: the Algorithm/Bank/Slot/KeyPair concepts and their relationships, how to enable SLOT_MGMT_CAP, the requester API, the responder HAL hooks, and the sample implementation. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
Add doc/slot_management_database.md, a reference data model for the device certificate/key/slot store, expressed as C structures. It normalizes the state shared by GET_DIGESTS, GET_CERTIFICATE, SET_CERTIFICATE, GET_CSR, GET_KEY_PAIR_INFO/SET_KEY_PAIR_INFO, and the SLOT_MANAGEMENT SubCodes into one authoritative copy of each fact (banks, slots, key pairs, slot-key associations, certificate chains, CSRs), so a single write propagates to every command's view. The document defines the four slot states, maps every field of the relevant DSP0274 wire structures to its schema source, and shows how the model resolves the recorded gaps (DMTF#3638, DMTF#3645). It is a device-backend data model, not an SPDM wire change. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com> Assisted-by: Claude Code:claude-opus-4-8
There was a problem hiding this comment.
When ManageBank(ConfigAlgo) or ManageSlot(Erase) successfully modifies a slot or bank that is currently associated with an active SPDM session (or the currently selected connection_info.current_bank), does libspdm automatically terminate those active sessions (libspdm_session_state_not_started) or invalidate cached transcript hashes?
If session cleanup is currently left to the HAL implementation inside libspdm_write_slot_management_*, should the core responder explicitly drop active sessions when the active bank's cryptographic capabilities change to guarantee state machine integrity per SPDM 1.4 rules?
Fix #3131
The spdm-emu update is at DMTF/spdm-emu#496.
The spdm-dump update is at DMTF/spdm-dump#140.