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.

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

Dual-API hash dispatch strategy #6549

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Nov 7, 2022

Architecture document for dispatching hash operations to the optimal backend: legacy or PSA.

To be extended to ciphers later. That can follow the same principle, but there will be extra complications. I think we should implement hash first and work out any snags before we go further with the design of cipher.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added needs-work needs-design-approval component-crypto Crypto primitives and low-level interfaces labels Nov 7, 2022
@gilles-peskine-arm gilles-peskine-arm marked this pull request as draft November 7, 2022 21:31
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

#### Compile-time availability determination

Can we determine how to dispatch at compile time?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the wrong question, or at least, it's being asked too early. The important question is: what does code calling existing APIs expect?

  • Code in the legacy domain expects that all legacy mechanisms are available at all times.
  • Code in the PSA domain expect that all PSA mechanisms are available after psa_crypto_init, and (sometimes) that mechanisms not indicated as available through PSA_WANT_xxx are rejected.
  • Code in the mixed domain needs to conform to the expectations of its callers.

A useful conclusion is that the union of PSA and legacy is always correct for the hybrid domain, regardless of the details of how it's done under the hood:

  • When the union is called by legacy code, mechanisms that have a legacy implementation are available, regardless of the PSA state.
  • When the union is called by PSA code, PSA is guaranteed to have been initialized, and mechanisms that have a PSA implementation are available.

Backward compatibility does not require making any promises regarding the runtime availability of PSA-only mechanisms when the union is called by legacy code, provided that legacy code does not care about negative availability.

Finish working out the RSA-PSS example in terms of what it implies about the
interface. The key takeaway is that a mixed-domain module must support
algorithms if they are available through either interface, and that's all
there is to it. The details of how dispatch is done don't matter, what
matters is only the availability, and it's just the disjunction of
availabilities.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm marked this pull request as ready for review November 25, 2022 22:05
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work labels Nov 25, 2022
@gilles-peskine-arm gilles-peskine-arm changed the title Dual-API hash and cipher dispatch strategy Dual-API hash dispatch strategy Nov 25, 2022
@mpg mpg linked an issue Dec 1, 2022 that may be closed by this pull request

#### Get rid of the hash_info module

The hash_info module is redundant with MD light. Move `mbedtls_md_error_from_psa` to `md.c`, defined only when `MBEDTLS_MD_SOME_PSA` is defined. The rest is no longer used.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the function mbedtls_hash_info_md_from_psa is also used inside PSA code when it's calling legacy interfaces (RSA, ECDSA). It's the dual of psa_alg_of_md_info and we need both functions in our code base at least until 4.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fortunately, if we optimize type conversions as above, this function will be quite trivial as well.

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

I haven't finished reviewing, but I'm still releasing my comments so far. I'll start again with line 404.

docs/architecture/psa-migration/md-cipher-dispatch.md Outdated Show resolved Hide resolved
docs/architecture/psa-migration/md-cipher-dispatch.md Outdated Show resolved Hide resolved
docs/architecture/psa-migration/md-cipher-dispatch.md Outdated Show resolved Hide resolved
docs/architecture/psa-migration/md-cipher-dispatch.md Outdated Show resolved Hide resolved
No intended meaning change.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
It's a rare scenario, but it's currently possible: if you use
mbedtls_cipher_xxx() to encrypt the communication between the application
and the crypto service, changing those functions to call PSA will break your
system.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@mpg
Copy link
Contributor

mpg commented Feb 8, 2023

I've added the legacy-implies-PSA requirement.

Thanks.

I don't think I should remove any MBEDTLS_PSA_CRYPTO_CLIENT considerations from the document. We aren't aiming to make it all work, just to preserve the current functional behavior.

I've added the legacy-implies-PSA requirement.

Thanks.

I don't think I should remove any MBEDTLS_PSA_CRYPTO_CLIENT considerations from the document. We aren't aiming to make it all work, just to preserve the current functional behavior.

Points of agreement:

  • I'm OK with keeping MBEDTLS_PSA_CRYPTO_CLIENT in the analysis sections of the document.
  • Obviously anything that already works needs to keep working.
  • We aren't aiming to make it all work.

Point of disagreement or misunderstanding: I don't think we should add any support for MBEDTLS_PSA_CRYPTO_CLIENT in the new MD (light): ie, we should only dispatch to PSA when MBEDTLS_PSA_CRYPTO_C is defined (and the hash is accelerated). Looking at the document, I think that's what it currently says, but the comments paint a different picture, specifically https://github.com/Mbed-TLS/mbedtls/pull/6549/files#r1032788788, https://github.com/Mbed-TLS/mbedtls/pull/6549/files#r1047051934 and https://github.com/Mbed-TLS/mbedtls/pull/6549/files#r1054190582 (some of them by me, some by you). So basically what I'm saying now is I think we should disregard those comments and keep the document as it is.

So, I'm going to go ahead and mark those discussions as resolved, and then approve the PR, because I'm OK with the document as it is, just not with the modifications suggested by those comments.

mpg
mpg previously approved these changes Feb 8, 2023
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm
Copy link
Contributor Author

I don't think we should add any support for MBEDTLS_PSA_CRYPTO_CLIENT in the new MD (light): ie, we should only dispatch to PSA when MBEDTLS_PSA_CRYPTO_C is defined (and the hash is accelerated)

I think we should do that eventually but not part of the first batch of work.

Looking at the document, I think that's what it currently says

Indeed. But I think it should capture at least the gist of what should happen with client-only builds in the future. So I'll add that, but as a separate section.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added priority-high High priority - will be reviewed soon and removed needs-design-approval needs-reviewer This PR needs someone to pick it up for review labels Feb 8, 2023
@gilles-peskine-arm
Copy link
Contributor Author

I'm removing “needs design approval” since this has Manuel's approval.

mpg
mpg previously approved these changes Feb 8, 2023
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

AndrzejKurek
AndrzejKurek previously approved these changes Feb 10, 2023
Copy link
Contributor

@AndrzejKurek AndrzejKurek left a comment

Choose a reason for hiding this comment

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

Looks good, I only have minor questions. I think that the #6977 prototype should show any loose ends.


In this work, we want two things:

* Make non-covered modules call PSA, but only [when this will actually work](#why-psa-is-not-always-possible). This effectively brings those modules to a partial use-PSA behavior regardless of whether the option is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds a bit strange, since it means that even if we disable MBEDTLS_USE_PSA_CRYPTO, we still aim to use it (so it's useless);
If what you mean is that if MBEDTLS_USE_PSA_CRYPTO is disabled, hence it will not actually work, it's not that transparent here.

Copy link
Contributor

@mpg mpg Feb 10, 2023

Choose a reason for hiding this comment

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

Yes, PSA will be used in some places even if MBEDTLS_USE_PSA_CRYPTO is disabled. I agree that's a bit unintuitive so perhaps we can update the documentation of that macro:

  • when it is enabled, the modules that obey it (PK, X.509, TLS 1.2/shared) will use PSA Crypto whenever they can and more importantly, the user promises to call psa_crypto_init() before calling any function from those modules;
  • when it is disabled, any module may or may not use PSA Crypto at their discretion, but since the user has made no promise to call psa_crypto_init() before calling functions from legacy modules, it's up to these modules to check whether calling PSA crypto is safe or not.

docs/architecture/psa-migration/md-cipher-dispatch.md Outdated Show resolved Hide resolved
docs/architecture/psa-migration/md-cipher-dispatch.md Outdated Show resolved Hide resolved
docs/architecture/psa-migration/md-cipher-dispatch.md Outdated Show resolved Hide resolved

There is no strong reason to allow mechanisms available through legacy but not PSA when `MBEDTLS_PSA_CRYPTO_C` is enabled. This would only save at best a very small amount of code size in the PSA dispatch code. This may be more desirable when `MBEDTLS_PSA_CRYPTO_CLIENT` is enabled (having a mechanism available only locally and not in the crypto service), but we do not have an explicit request for this and it would be entirely reasonable to forbid it.

In this analysis, we have not found a compelling reason to require all legacy mechanisms to also be available through PSA. However, this can simplify both the implementation and the use of dispatch code thanks to some simplifying properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also want to differentiate between PSA->drivers, and PSA->software, when both are present? Again, probably irrelevant for hashes, but I would assume that key location will handle that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even with keys, legacy interfaces are always passing keys in plaintext, so all that matters is volatile, transparent keys (lifetime=0).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Feb 15, 2023
@mpg mpg merged commit 6778ddf into Mbed-TLS:development Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces priority-high High priority - will be reviewed soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Architecture: simplify dual-API hash dispatch
3 participants