Conversation
|
Hello, And then, if you could base this from the "libacvp_2_3_0-throttle" branch instead of main, that would be needed too. Thanks again, |
xmamalou
force-pushed
the
main
branch
2 times, most recently
from
50bda28 to
60eff14
Compare
xmamalou
force-pushed
the
main
branch
6 times, most recently
from
9aca533 to
392d1d4
Compare
|
Hello, sorry for the long wait. I have done the changes as requested. Sorry again for the formatting, Emacs was more zealous with the autoformatting than I expected and I hadn't noticed it had changed the entire file. Best regards, |
|
Thanks! Taking a look now. |
| ACVP_ASCON_CUSSTRLEN_PARM, | ||
| } ACVP_ASCON_PARM; | ||
|
|
||
| typedef enum acvp_ascon_direction { |
There was a problem hiding this comment.
Can we name these "ACVP_ASCON_DIR_ENCRYPT" and "ACVP_ASCON_DIR_DECRYPT"? It would follow the naming scheme for other params a bit more closely.
I would also prefer having an "ACVP_ASCON_DIR_BOTH" instead of the extra append API in acvp_capabilities, and having a single param for this instead of the list. For example, see: ACVP_SYM_CIPH_DIR
| * @brief This struct holds data that represents a single test case for ASCON | ||
| * testing. This data is passed between libacvp and the crypto module. | ||
| */ | ||
| typedef struct acvp_ascon_tc_t { |
There was a problem hiding this comment.
Since we are using this struct for some fundamentally different test types, could we organize or comment these to indicate which fields are used for which test types? For example, see: acvp_ml_kem_tc_t
| bool nonce_masking; | ||
| ACVP_JSON_DOMAIN_OBJ msg_len; | ||
| ACVP_JSON_DOMAIN_OBJ out_len; | ||
| ACVP_JSON_DOMAIN_OBJ cusstr_len; |
There was a problem hiding this comment.
nit: maybe "custom_len"?
| return ACVP_SUCCESS; | ||
| } | ||
|
|
||
| ACVP_RESULT acvp_cap_ascon_append_value(ACVP_CTX *ctx, ACVP_CIPHER cipher, |
There was a problem hiding this comment.
See above comment regarding ACVP_ASCON_DIRECTION - I would rather have a "both" value than a separate API for this, like AES/TDES does
| return result; | ||
| } | ||
|
|
||
| ACVP_RESULT acvp_cap_ascon_set_value(ACVP_CTX *ctx, ACVP_CIPHER cipher, |
There was a problem hiding this comment.
Could we rename this to acvp_cap_ascon_set_parm? I agree "value" is a better naming convention, but I would rather it be consistent with the other APIs until we can change all of them.
This API should also be able to set individual values within a domain. Not all of the algorithms can do this currently, but I have added it when adjusting existing ones moving forward. For example, see acvp_cap_hmac_set_parm. It appends to the ACVP_JSON_DOMAIN_OBJ "values" list.
| return cap; | ||
| } | ||
|
|
||
| static ACVP_ASCON_CAP *allocate_ascon_cap(void) { |
There was a problem hiding this comment.
We should free this in acvp_free_test_session() in acvp.c
| JSON_Object *pl_obj = json_value_get_object(pl_value); | ||
| json_object_set_number(pl_obj, "min", cap->payload_len.min); | ||
| json_object_set_number(pl_obj, "max", cap->payload_len.max); | ||
| json_object_set_number(pl_obj, "increment", cap->payload_len.increment); |
There was a problem hiding this comment.
For all domains, see the comment about setting individual values in acvp_capabilities.c. See acvp_build_hmac_register_cap() for an example of how that list is added to the JSON.
| return rv; | ||
| } | ||
|
|
||
| static ACVP_RESULT acvp_ascon_aead128_init_tc( |
There was a problem hiding this comment.
Nit: Match existing code style for other _init_tc() APIs
| } | ||
| stc->payload_len = payload_len; | ||
|
|
||
| stc->key = calloc(1, ACVP_ASCON_KEY_BYTE_MAX); |
There was a problem hiding this comment.
Null check all calloc before calling hexstr_to_bin for extra code durability (a few more instances below this)
| } | ||
|
|
||
| // Create ACVP array for response | ||
| JSON_Array *reg_array = NULL; |
There was a problem hiding this comment.
For code consistency, I would prefer to declare all variables throughout this function at the top of the function with 0/null values, then define them later on. It also can help avoid some free() edge cases with the loops, if the code needs adjustments later on.
2 participants
Add handling for ASCON