Add SPDM 1.3 new feature:get_key_pair_info #2771

Wenxing-hou · 2024-07-15T08:35:59Z

Refer the issue: #2293

jyao1 · 2024-07-15T08:50:12Z

library/spdm_requester_lib/libspdm_req_get_key_pair_info.c

+        return LIBSPDM_STATUS_UNSUPPORTED_CAP;
+    }
+
+    if ((key_pair_id == 0) || (key_pair_id > SPDM_MAX_SLOT_COUNT)) {


@steven-bellock, do you agree this check: key_pair_id > SPDM_MAX_SLOT_COUNT ?

The number of KeyPairIds can be larger than the number of certificate slots. But we can run that by the SPDM Working Group to ensure that's what was intended in the specification.

library/spdm_responder_lib/libspdm_rsp_key_pair_info.c

include/internal/libspdm_common_lib.h

include/library/spdm_lib_config.h

include/industry_standard/spdm.h

include/internal/libspdm_common_lib.h

include/library/spdm_lib_config.h

steven-bellock · 2024-07-17T18:08:07Z

library/spdm_common_lib/libspdm_com_context_data.c

@@ -503,6 +503,7 @@ libspdm_return_t libspdm_set_data(void *spdm_context, libspdm_data_type_t data_t
            return LIBSPDM_STATUS_INVALID_PARAMETER;


The check at line 499 should be > LIBSPDM_MAX_KEY_PAIR_COUNT.

libspdm/library/spdm_common_lib/libspdm_com_context_data.c

Lines 494 to 506 in 504fd45

case LIBSPDM_DATA_LOCAL_KEY_PAIR_ID:

if (parameter->location != LIBSPDM_DATA_LOCATION_LOCAL) {

return LIBSPDM_STATUS_INVALID_PARAMETER;

}

slot_id = parameter->additional_data[0];

if (slot_id >= SPDM_MAX_SLOT_COUNT) {

return LIBSPDM_STATUS_INVALID_PARAMETER;

}

if (data_size != sizeof(spdm_key_pair_id_t)) {

return LIBSPDM_STATUS_INVALID_PARAMETER;

}

context->local_context.local_key_pair_id[slot_id] = *(spdm_key_pair_id_t *)data;

break;

This set data operation is just to create a association between slot_id and key_pair_id.

Do you mean we need add a check for the data? The set data(key pair id) should < LIBSPDM_MAX_KEY_PAIR_COUNT.

library/spdm_requester_lib/libspdm_req_get_key_pair_info.c

steven-bellock · 2024-07-18T15:25:26Z

For the Requester API I'm not sure there's much value to storing the KEY_PAIR_INFO info into the spdm_context. The Requester already gets its relevant key usage information from GET_DIGESTS. KEY_PAIR_INFO is really for SET_KEY_PAIR_INFO, so I could see libspdm_get_key_pair_info looking like

libspdm_get_key_pair_info(
  void *spdm_context,
  const uint32_t *session_id,
  uint8_t key_pair_id,
  uint8_t *total_key_pairs,
  uint16_t *capabilities
  .
  .
  )

the values returned can then be evaluated and used in libspdm_set_key_pair_info.

jyao1 · 2024-07-22T01:54:18Z

The keypairinfo should be saved to device NVS.

jyao1 · 2024-07-25T01:57:02Z

os_stub/spdm_device_secret_lib_sample/lib.c

+    libspdm_key_pair_info_t key_pair_info[LIBSPDM_MAX_KEY_PAIR_COUNT];
+
+    /*provisioned key pair info*/
+    uint8_t public_key_info_rsa2048[] = {0x30, 0x0D, 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF7,


needs to add all algorithms.

steven-bellock

I filed https://github.com/DMTF/SPDM-WG/issues/3543. There may be more checks if the Responder's SET_KEY_PAIR_INFO_CAP == 0.

unit_test/test_spdm_requester/get_key_pair_info.c

unit_test/test_spdm_responder/key_pair_info.c

steven-bellock · 2024-08-06T20:52:56Z

include/hal/library/responder/key_pair_info.h

+ * @param  spdm_context               A pointer to the SPDM context.
+ * @param  key_pair_id                Indicate which key pair ID's information to retrieve.
+ *
+ * @param  total_key_pairs            Indicate the total number of key pairs on the responder.


As was clarified in https://github.com/DMTF/SPDM-WG/issues/3526#issuecomment-2233350803, total_key_pairs is fixed. As such it can be stored in the spdm_context and removed from this function.

Hi. The total_key_pair is still returned in the function.
Because the key pair info is provisioned by the responder device, the key pair number should be consistent.

We'll see what @jyao1's opinion is when he gets back.

Can we design a new API named libspdm_get_total_key_pairs() ?

Then, we don't need total_key_pairs parameter in this function.

Can we design a new API named libspdm_get_total_key_pairs() ?

If it's going to be returned in a function then it should be in libspdm_read_key_pair_info. What I, and the SPDM specification, is saying is that since total_key_pairs is fixed at the beginning of the connection and cannot change then just put it in the spdm_context.

I am fine with that. We can add LIBSPDM_DATA_TOTAL_KEY_PAIRS_NUMBER data type.

Just LIBSPDM_DATA_TOTAL_KEY_PAIRS.

Just LIBSPDM_DATA_TOTAL_KEY_PAIRS.

This will cause misunderstanding. LIBSPDM_DATA_TOTAL_KEY_PAIRS may be interpreted as total key pairs information.

Thanks. I have fixed the feedback. Please review the comparing:
https://github.com/DMTF/libspdm/compare/6fc54f2033535b9f78a1bfe0faf519e5c9396ba3..6a04b803a95ec3406e09a6d13e81e1530012a14c

steven-bellock · 2024-08-07T17:08:49Z

As was clarified in https://github.com/DMTF/SPDM-WG/issues/3543#issuecomment-2273487285 if the Responder's SET_KEY_PAIR_INFO_CAP == 0 then KEY_PAIR_INFO.Capabilities must be 0 and the Requester can check that. In addition the check

    if ((spdm_response->capabilities & SPDM_KEY_PAIR_CAP_SHAREABLE_CAP) == 0) {
        if (!libspdm_onehot0(spdm_response->assoc_cert_slot_mask)) {
            status = LIBSPDM_STATUS_INVALID_MSG_FIELD;
            goto receive_done;
        }
    }

needs to be removed.

Wenxing-hou · 2024-08-08T09:47:23Z

As was clarified in DMTF/SPDM-WG#3543 (comment) if the Responder's SET_KEY_PAIR_INFO_CAP == 0 then KEY_PAIR_INFO.Capabilities must be 0 and the Requester can check that. In addition the check
    if ((spdm_response->capabilities & SPDM_KEY_PAIR_CAP_SHAREABLE_CAP) == 0) {
        if (!libspdm_onehot0(spdm_response->assoc_cert_slot_mask)) {
            status = LIBSPDM_STATUS_INVALID_MSG_FIELD;
            goto receive_done;
        }
    }
needs to be removed.

Yes. I have added the check:

    /*If responder doesn't support SET_KEY_PAIR_INFO_CAP,the capabilities should be 0*/
    if ((!libspdm_is_capabilities_flag_supported(
            spdm_context, true, 0,
            SPDM_GET_CAPABILITIES_RESPONSE_FLAGS_SET_KEY_PAIR_INFO_CAP)) &&
        ((spdm_response->capabilities & SPDM_KEY_PAIR_CAP_MASK) != 0)) {
        status = LIBSPDM_STATUS_INVALID_MSG_FIELD;
        goto receive_done;
    }

And I have deleted the assoc_cert_slot_mask check in get_key_pair_info.

steven-bellock · 2024-08-19T17:59:34Z

doc/user_guide.md

@@ -421,6 +421,12 @@ Refer to spdm_server_init() in [spdm_responder.c](https://github.com/DMTF/spdm-e

   1.7, if PSK is required, optionally deploy PSK Hint in the call to libspdm_start_session().

+   1.8, if responder can support multi key pairs, the total_key_pairs need be set.


if Responder sets GET_KEY_PAIR_INFO_CAP then LIBSPDM_DATA_TOTAL_KEY_PAIRS must be set.

Thanks. I have fixed it.

steven-bellock · 2024-08-19T18:16:33Z

library/spdm_responder_lib/libspdm_rsp_key_pair_info.c

+
+    total_key_pairs = spdm_context->local_context.total_key_pairs;
+    key_pair_id = spdm_request->key_pair_id;
+    if ((key_pair_id == 0) || (key_pair_id >= total_key_pairs)) {


key_pair_id > total_key_pairs

Thanks. I have fixed it.

steven-bellock · 2024-08-19T18:17:22Z

library/spdm_responder_lib/libspdm_rsp_key_pair_info.c

@@ -0,0 +1,157 @@
+/**


File should be libspdm_rsp_get_key_pair_info.c.

Thanks. I have changed the file name.

Refer the issue:DMTF#2293 Signed-off-by: Wenxing Hou <[email protected]>

Signed-off-by: Wenxing Hou <[email protected]>

steven-bellock

Seems like the pull request is in a good place to start testing.

Wenxing-hou marked this pull request as draft July 15, 2024 08:44

Wenxing-hou force-pushed the add_get_key_pair branch from 4008d36 to 6c88236 Compare July 15, 2024 08:45