test_psa_constant_names: support key agreement, better code structure #324
Conversation
FOO(BAR) is an expression, not a name. Pack expression generation into a method. No behavior change.
No need to split lines, or remove whitespace after removing whitespace. No behavior change.
No behavior change.
No behavior change.
No behavior change.
gilles-peskine-arm
added
enhancement
| c, e = do_test(options, inputs, type_word, names) | ||
| for type_word in ['status', 'algorithm', 'ecc_curve', 'dh_group', | ||
| 'key_type', 'key_usage']: | ||
| c, e = do_test(options, inputs, type_word) |
There was a problem hiding this comment.
In commit "Move the type_word->name_set mapping into its own method", which says it has no change on behavior, we don't need to call get_names() anywhere? For example, while passing a names parameter to do_test()
| values = run_c(options, type_word, expressions) | ||
| return expressions, values | ||
|
|
||
| def do_test(options, inputs, type_word): |
There was a problem hiding this comment.
In commit "Move value collection into its own function", which says it makes no change to behavior, don't we need to update all callers of do_test()? We are changing the parameters do_test() makes in a commit that doesn't change the callers, so I don't see how this wouldn't change behavior.
No behavior change.
No behavior change.
No behavior change.
This makes the structure of the code more apparent. No behavior change.
No behavior change.
No behavior change.
No behavior change.
PSA_ECC_CURVE_xxx and PSA_DH_GROUP_xxx were not collected from headers, only from test suites.
Insist that test cases must only use macro names that are declared in a header. This may catch errors such as not parsing the intended files. Make this check easily overridden in a derived class.
Key agreement algorithms were excluded back when they were constructed with a macro conveying the key agreement itself taking the KDF as an argument, because that was hard to support. Now the encoding has changed and key agreement algorithms are constructed with PSA_ALG_KEY_AGREEMENT taking two arguments, one that identifies the raw key agreement and one that identifies the KDF. This is easy to process, so add support.
gilles-peskine-arm
force-pushed
the
psa-test_psa_constant_names-refactor_and_ka
branch
from
cc72675 to
d2cea9f
Compare
|
I rewrote the history in two places:
|
| # Keep the expression as one to test as an algorithm. | ||
| function = 'other_algorithm' | ||
| if function in self.table_by_test_function: | ||
| sets.append(self.table_by_test_function[function]) |
There was a problem hiding this comment.
Do we want to call accept_test_case_line() if function was not in self.table_by_test_function? I see accept_test_case_line() throws an exception instead of returning False for not accepted test cases.
There was a problem hiding this comment.
Good question. If we add a function to test_suite_psa_crypto_metadata, what do we want to happen here? I don't think the current behavior is correct: a new function whose name does not end in _algorithm would undergo the argument validity check without contributing to the data tested by this script. I'll change the code to insist on the function being partially known (i.e. either in the table or ending in _algorithm): better fail with an exception than silently not test something that we might reasonably believe is tested.
There was a problem hiding this comment.
I've changed the code to insist on knowing the function name. This uncovered a lack of coverage for some expressions involving a wildcard.
When gathering test cases from test_suite_psa_crypto_metadata, look up the test function explicitly. This way test_psa_constant_names will error out if we add a new test function that needs coverage here. This change highlights an omission in the previous version: asymmetric_signature_wildcard was silently ignored as a source of algorithm expressions to test. Fix that.
gilles-peskine-arm
removed
the
needs: review
* ARMmbed#321: Replace config.pl by config.py * ARMmbed#322: Update Mbed Crypto with latest Mbed TLS changes as of 2019-11-15 * ARMmbed#308: Small performance improvement of mbedtls_mpi_div_mpi() * ARMmbed#324: test_psa_constant_names: support key agreement, better code structure * ARMmbed#320: Link to the PSA crypto portal page from README.md * ARMmbed#293: Always gather MBEDTLS_ENTROPY_BLOCK_SIZE bytes of entropy * ARMmbed#310: Clarify test descriptions in test_suite_memory_buffer_alloc * ARMmbed#307: Add ASN.1 ENUMERATED tag support * ARMmbed#328: Remove dependency of crypto_values.h on crypto_extra.h * ARMmbed#325: Rename psa_asymmetric_{sign_verify} to psa_{sign,verify}_hash Missed listing in the previous submodule update: * ARMmbed#304: Make sure Asan failures are detected in 'make test'
This pull request consists mostly of changes to
test_psa_constant_names.py:test_psa_constant_names.py. I initially did them in preparation for reusing major parts of the code in another script. This other script is not stable yet and I'm not sure if I need it after all. Since the refactorings are at worst harmless and at best useful to make the code easier to maintain, I'm submitting them.test_suite_psa_crypto_metadata, add a few more test cases for KDF.