Remove redundant declarations and update docs

[bjwtaylor]

Description

Currently we have a duplicate definition warning caused by the same definition being in crypto.h and crypto_structs.h My understanding from the docs is crypto.h is the public API and crypto_structs.h is the backend implementation. If I have understood correctly there will always be a crypto_structs.h or similar definition and this will always be the definition we need the system to use. Therefore what I can tell this means crypto.h exists purely for documentation. I have therefore resolved the issue by guarding the duplicate definitions with DOXYGEN_ONLY. My understanding is this will remove them from compilation, however they will still be used in the API documentation.

Fix Mbed-TLS/mbedtls#10376

PR checklist

• changelog provided
• framework PR not required
• mbedtls development PR provided Mbed-TLS/mbedtls#remove redundant declarations mbedtls#10659
• mbedtls 3.6 PR not required because: No Backports
• tests not required because: No changes.

[gilles-peskine-arm]

The proposed change is not desirable.

It's not really clear to me what the goal is. If the goal is to compile with no warnings from gcc -Wredundant-decls, we need a different solution. That could be some header restructuring, or selectively disabling the warning using pragmas.

If we make our code clean with -Wredundant-decls, we need to add this to our build scripts so that it's enforced. However, it's not fully clear to me why this is desirable. We've never promised to pay attention to this warning and I don't see why it's useful.

[gilles-peskine-arm]

Marking some function prototypes as Doxygen-only doesn't make sense. The reason to have prototypes is so that the C compiler sees them.

The reason some static inline functions have a prototype in crypto.h and a definition in crypto_struct.h is that the prototypes are the same for everyone, but the function definitions are not. By default, TF-PSA-Crypto presents itself a library. But it can also be integrated as a client and server, in which case the integrator makes some changes:

• On the client side:
  • Define MBEDTLS_PSA_CRYPTO_CLIENT but not MBEDTLS_PSA_CRYPTO_C.
  • Edit crypto_struct.h to change the definitions of operation structures, typically to have an operation handle instead of cryptographic material.
  • Provide implementations of the PSA API functions (excluding the static inline functions that manipulate a local struct) that makes remote procedure calls to the server.
• On the server side:
  • Probably define MBEDTLS_PSA_CRYPTO_SPM and likely MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER.
  • Implement the server side of the remote procedure calls.

While the definitions of the struct constructors in crypto_struct.h are customizable, their prototypes are not. Hence the prototypes live in crypto.h. And the prototypes must be visible to the C compiler.

[gilles-peskine-arm]

That being said, is the current code really what it should be? No, there are two things that I never got around to fixing.

• psa_key_attributes_t is not supposed to be user-serviceable, unlike operation structures. So it should be in crypto_types.h, and its inline accessor functions should not be defined in crypto_struct.h.
• The definitions of the struct constructor functions (psa_xxx_init()) don't actually need to be in crypto_struct.h, because their textual definition is actually the same for everyone. They're defined in terms of the corresponding PSA_XXX_INIT macro. Only the macro needs to be customized if you customize the struct layout.

We could fix this. It would be an incompatible change for integrators who customize crypto_struct.h. We don't explicitly promise any backward compatibility for such custom integrations, so I think it would be ok to do what I just wrote in the development branch. But I don't think we should do it in an LTS branch unless there's a compelling reason, and I don't see a compelling reason.

[bjwtaylor]

OK, thanks for your comments. Let's put this one on hold pending design review of the original issue. There is also a related PR here: Mbed-TLS/mbedtls#10659. It's the same issue, but not the same solution. Let me know what you think about this one, maybe we just decide we don't want to fix these warnings?