We have two common.h files with duplicated content

[gilles-peskine-arm]

Since 90ca414, we have both library/common.h and tf-psa-crypto/core/common.h, with identical content (if we remember to keep them in synch). This is a problem for two reasons.

Having duplicated code is intrinsically problematic for maintenance. (The reason I found out about the duplication is that I changed one copy and struggled to understand why my changes apparently had no effect.)

In addition, having two files with the same name is problematic because it can break the build for users who don't use our build scripts and just treat our source code as a bunch of .c and .h files. This is not uncommon and we do want to support it (even if it's a best-effort which we can't really test).

What I think we should do is:

• Keep common.h in tf-psa-crypto/core — it should have been moved rather than copied. (Note: we may want to git rm tf-psa-crypto/core/common.h; git commit; git mv library/common.h tf-psa-crypto/core; git commit so that git understands that patches to tf-psa-crypto/core/common.h need to be backported to library/common.h.)
• Include common.h in library/x509_internal.h and library/ssl_misc.h, and make sure every x509/ssl source file includes the respective internal header.

(This doesn't allow building Mbed TLS without TF-PSA-Crypto, but that is not an objective of Mbed TLS 4.x.)

[Harry-Ramsey]

I cannot find library/ssl_internal.h was there another file with a similar name that you meant or should the SSL files contain an #include "x509_internal.h" statement?

[gilles-peskine-arm]

Oh, sorry, I meant ssl_misc.h (the header file that all (non-generated) SSL library modules include).

[ronald-cron-arm]

There is one thing that can be a problem here. I wanted to comment on that for some time but have not found the time to do so. common.h includes build_info.h as mbedtls/build_info.h. The build systems in Mbed TLS and TF-PSA-Crypto are different and thus their build_info.h are going to be different: for exemple Mbed TLS build_info.h should include mbedtls_config.h for the configuration of TLS and x509, not the TF-PSA-Crypto build_info.h. Thus as for common.h I was rather thinking about having two common.h: mbebtls_common.h and tf_psa_crypto_common.h (not sure about the names) that would be quite small. mbedtls_common.h would contain #include "mbedtls/build_info.h, tf_psa_crypto_common.h would contain #include "psa/build_info.h. Most of the content of the current common.h would be split out to "thematic" includes like we have already done witj alignment.h or just a common_misc.h? @gilles-peskine-arm what do you think about that?

[gilles-peskine-arm]

@ronald-cron-arm I think we have basically the same base idea: each repository has a “common” header file, they have different names, they each include their respective build_info, and the mbedtls common header includes the tf-psa-crypto header.

Then you propose to also split out the crypto common file thematically, and I propose to also distinguish between x509 and tls common. I have no strong opinion on either of these topics.

[ronald-cron-arm]

Thanks for your response, I think I understand better the proposal now but regarding

include common.h in library/x509_internal.h and library/ssl_misc.h, and make sure every x509/ssl source file includes the respective internal header.

I am not sure to fully follow.

Thus currently in ssl_client.c we have:

#include "common.h"                                                             
                                                                                
#if defined(MBEDTLS_SSL_CLI_C)                                                  
#if defined(MBEDTLS_SSL_PROTO_TLS1_3) || defined(MBEDTLS_SSL_PROTO_TLS1_2)      
                                                                                
#include <string.h>                                                             
                                                                                
#include "debug_internal.h"                                                     
#include "mbedtls/error.h"                                                      
#include "mbedtls/platform.h"                                                   
                                                                                
#include "ssl_client.h"                                                         
#include "ssl_misc.h"                                                           
#include "ssl_tls13_keys.h"                                                     
#include "ssl_debug_helpers.h"
...

Is the proposal to move to:

#if defined(MBEDTLS_SSL_CLI_C)                                                  
#if defined(MBEDTLS_SSL_PROTO_TLS1_3) || defined(MBEDTLS_SSL_PROTO_TLS1_2)      
                                                                                
#include <string.h>                                                             
                                                                                
#include "debug_internal.h"                                                     
#include "mbedtls/error.h"                                                      
#include "mbedtls/platform.h"                                                   
                                                                                
#include "ssl_client.h"                                                         
#include "ssl_misc.h"                                                           
#include "ssl_tls13_keys.h"                                                     
#include "ssl_debug_helpers.h" 
...

or

#include "ssl_misc.h"                                                             
                                                                                
#if defined(MBEDTLS_SSL_CLI_C)                                                  
#if defined(MBEDTLS_SSL_PROTO_TLS1_3) || defined(MBEDTLS_SSL_PROTO_TLS1_2)      
                                                                                
#include <string.h>                                                             
                                                                                
#include "debug_internal.h"                                                     
#include "mbedtls/error.h"                                                      
#include "mbedtls/platform.h"                                                   
                                                                                
#include "ssl_client.h"                                                                                                                  
#include "ssl_tls13_keys.h"                                                     
#include "ssl_debug_helpers.h" 
...

in both cases we will hit some troubles I'd say.

[gilles-peskine-arm]

I propose to move #include "ssl_misc.h" to the top, replacing #include "common.h". What troubles? In principle it should work, but of course I know the real world is sometimes not as simple as we'd like.

[ronald-cron-arm]

Well sometimes when you change the order of includes you have some surprises and I am not completely confident in the sanity of the header inclusions in the library. But we can try it. Interestingly ssl_misc.h already includes common.h, this would need to be moved at the top of the file in replacement of #include "mbedtls/build_info.h I'd say.

[gilles-peskine-arm]

The main gotcha I can think of wrt include order is that sometimes we have ifdefs before including the config, so they go through the “not defined” path even when the configuration defines that symbol. But that is precisely a problem we won't run into, since we're ensuring that build_info.h gets included before anything else happens.

[ronald-cron-arm]

@gilles-peskine-arm thanks for the discussion. @Harry-Ramsey I think I understand now where we want to go with this issue. Hopefully the discussion above will be helpful for you as well.

[Harry-Ramsey]

It would seem that changing the header order with x509_internal.h and ssl_misc.h results in issues of undefined structs, members or functions. This will require a bit more investigation than first thought and may require a bit of a redesigned solution.

[ronald-cron-arm]

Then I would suggest to go for a less disruptive change. In Mbed TLS (not tf-psa-crypto), replace #include "common.h" with #include "mbedtls_common.h" with mbedtls_common.h including common.h located in tf-psa-crypto/include/core.