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.

Sign up for GitHub

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

Adapt SHA256 dependency in generate_pkcs7_tests.py #46

Closed
wants to merge 2 commits into from
Closed

Adapt SHA256 dependency in generate_pkcs7_tests.py #46

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
209abd0
Adapt SHA256 dependency in generate_pkcs7_tests.py
ronald-cron-arm Sep 13, 2024
ffb9756
Refine proposal
ronald-cron-arm Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions scripts/generate_pkcs7_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@

import sys
from os.path import exists
from mbedtls_framework.build_tree import is_mbedtls_3_6
from mbedtls_framework import test_case

PKCS7_TEST_FILE = "../suites/test_suite_pkcs7.data"
PKCS7_TEST_FILE = "tests/suites/test_suite_pkcs7.data"

class Test: # pylint: disable=too-few-public-methods
"""
Expand All @@ -37,7 +39,15 @@ class TestData:
Take in test_suite_pkcs7.data file.
Allow for new tests to be added.
"""
mandatory_dep = "MBEDTLS_MD_CAN_SHA256"
# In Mbed TLS 3.6, PKCS7 code belongs to the legacy domain
# (see transition-guards.md for more information) thus use
# MBEDTLS_MD_CAN_SHA256 dependency symbol instead of PSA_WANT_ALG_SHA_256.
mandatory_dep = "MBEDTLS_MD_CAN_SHA256" if is_mbedtls_3_6() else "PSA_WANT_ALG_SHA_256"

# OR helper function but it does not help with the difficulty to decide
Copy link
Contributor

Choose a reason for hiding this comment

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

For the choice of having a helper function or not: I think it mainly depends of what common patterns we have, how many instances of these patterns, and whether we're likely to have more instances in the future. My impression is:

  • We have a small number of patterns. At least hashes, maybe PK algorithms?
  • The number of instances is small, but larger than 1.
  • There's a significant chance that we'll add more instances during the lifetime of 3.6, as we improve test coverage for X.509 and TLS.
  • Furthermore the people adding these instances may be unfamiliar with 3.6 complexities, so we should make it discoverable. An optional argument on the PSA-related function is more discoverable than a pattern used in some other test script.

So I lean towards auxiliary functions.

# if some legacy domain code is involved or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

The caller knows what domain it's in. For example PKCS7 is in the USE_PSA_CRYPTO domain.

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 the difficult part to me actually. I thought it was the legacy domain.

Copy link
Contributor

Choose a reason for hiding this comment

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

All of X.509, TLS and PK are in the USE domain. USE and legacy are equivalent for hashes though: both use MBEDTLS_MD_CAN_xxx.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the difficult part to me actually.

I think that's the difficult part for everyone :) I was involved in creating these macros and I still need to check from time to time!

All of X.509, TLS and PK are in the USE domain.

I think some parts or TLS 1.3 (those that are not shared with 1.2) are not in the USE domain, but instead always use PSA. But for PK and X.509 yes.

# mandatory_dep = test_case.legacy_domain_hash_dependency_symbol("PSA_ALG_SHA_256")
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like it's an extension of psa_information.psa_want_symbol. Maybe even an optional argument
psa_information.psa_want_symbol('PSA_ALG_SHA_256', psa_information.MBEDTLS36.USE_PSA_CRYPTO)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The optional argument would be the domain of the code for which we want to express the dependency on a given hash, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Though on second thoughts the function should not be called psa_want_symbol, because that name already implies a PSA dependency. A new function in test_case.py does sound better, with a domain argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said on Slack, I think for hashes we can distinguish the domain with:

  • things in the PSA or pure-TLS-1.3 domain directly use PSA_WANT or call psa_want_symbol;
  • things in the legacy or USE domain use a new helper function.
    (Which is the current state of the PR.)

However that only works because hashes use the same macro for the legacy and USE domains. For other things we're not going to be that lucky. We could have two functions there, but then things wouldn't be uniform.

So, I think I have a preference for a function with a domain argument. Then we don't even need to separate by type of mechanisms (hashes vs cipher vs etc.)

For example:

def psa_or_36_feature_macro(psa_alg, domain36):
    if domain36 = DOMAIN36_PSA or not build_tree.is_mbedtls_3_6():
        return psa_information.psa_want_symbol(psa_alg)

    if psa_alg in hashes_for_36:
        return hashes_for_36[psa_alg]

    if domain36 = DOMAIN36_LEGACY and psa_alg in foo_for_36_legacy:
        return foo_for_36_legacy[psa_alg]
    if domain36 = DOMAIN36_USE_PSA and psa_alg in foo_for_36_use_psa:
        return foo_for_36_use_psa[psa_alg]
    # etc for each family of algs

    raise ValueError

(Or we could have a single dict mapping (psa_alg, domain36) to 3.6 feature macro (which would then need to have duplicated entries for hashes) or any other implementation strategy.)

What I mean is we can have a single function that acts as a drop-in replacement for psa_want_symbol() except it has an extra argument for the 3.6 domain, and I think this has my preference at the moment.


test_name = "PKCS7 Parse Failure Invalid ASN1"
test_function = "pkcs7_asn1_fail:"
def __init__(self, file_name):
Expand Down
35 changes: 35 additions & 0 deletions scripts/mbedtls_framework/test_case.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
import sys
from typing import Iterable, List, Optional

from . import build_tree
from . import crypto_knowledge
from . import psa_information
from . import typing_util

def hex_string(data: bytes) -> str:
Expand Down Expand Up @@ -89,3 +92,35 @@ def write_data_file(filename: str,
tc.write(out)
out.write('\n# End of automatically generated file.\n')
os.replace(tempfile, filename)

def legacy_domain_hash_dependency_symbol(psa_hash_alg: str) -> str:
"""Get the dependency symbol for an hash algorithm when the algorithm
support is needed by legacy domain code.

psa_hash_alg PSA macro of an hash algorithm (e.g. PSA_ALG_SHA_256)

If invoked in the context of Mbed TLS 4.x or TF-PSA-Crypto, the function
just returns the PSA_WANT_ prefixed symbol that corresponds to
psa_hash_alg.

If invoked in the context of Mbed TLS 3.6, the function returns the
MBEDTLS_MD_CAN_ prefixed symbol that corresponds to psa_hash_alg.

The function should be used only for dependencies of legacy domain code.
Otherwise, just use the PSA_WANT_ prefixed symbols. For more information
about the legacy domain and MBEDTLS_MD_CAN_ prefixed symbols, see
transition-guards.md.
"""
info = psa_information.Information()
Copy link
Contributor

Choose a reason for hiding this comment

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

Gathering information is costly (parsing several files) and should be cached.

But I'm not convinced this actually needs any information about PSA, as opposed to ad hoc text-based rules. We have a fixed set of algorithms that require adptation for 3.6.

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 that's possible, we could just have a dictionary with PSA_ALG_SHA_... as keys and MBEDTLS_MD_CAN_ ... as values. I am fine with that.


if psa_hash_alg not in info.constructors.algorithms:
raise Exception("{} is not a known PSA algorithm macro.".format(psa_hash_alg))

algorithm = crypto_knowledge.Algorithm(psa_hash_alg)
if not algorithm.can_do(crypto_knowledge.AlgorithmCategory.HASH):
raise Exception("{} is not an hash PSA algorithm macro.".format(psa_hash_alg))

if build_tree.is_mbedtls_3_6():
return "MBEDTLS_MD_CAN_" + psa_hash_alg[8:].replace("_", "")
Copy link
Contributor

@mpg mpg Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not quite correct: for SHA3 we want to keep the underscore. I think using a dict would be easier and more readable.

else:
return psa_information.psa_want_symbol(psa_hash_alg)