Query Mbed TLS configuration from ssl_client2 and ssl_server2 #2105

andresag01 · 2018-10-17T09:02:07Z

Description

This PR:

The problem was that the detection of the current configuration in ssl-opt.sh used to be based on a fragile combination of calls to grep and sed which did not work in some cases and is not very portable. This PR replaces that mechanism by extending the command line interface for ssl_client2 and ssl_server2 to allow querying the current Mbed TLS configuration. For example, to check if MBEDLTS_AES_C was enabled at compile time, we simply need to run the command:

programs/ssl/ssl_server2 query_config=MBEDTLS_AES_C

A return value of 0 means that the feature is enabled and 1 means it was not. If the macro has a non empty expansion, this will be printed to stdout.

Status

READY

Requires Backporting

[edited by mpg] Backporting required to 2.16 as this fixes issues in the tests - #2389
No need to backport for 2.7 as the issues were only introduced later (2.13).

Todos

Tests
Documentation
Changelog updated
Backported

Additional comments

This PR adds a dependency of ssl_client2 and ssl_server2 on the file query_config.c, so I am not sure whether the visualc files also need to be updated. However, I do not have a system with visual studio to test this.
I tested this on OS X and there are 4 ssl-opt.sh tests failing on my system, but the failures are also present in development.

andresag01 · 2018-10-17T09:03:57Z

scripts/config.pl

@@ -46,7 +46,7 @@
 my $usage = <<EOU;
 $0 [-f <file> | --file <file>] [-o | --force]
                   [set <symbol> <value> | unset <symbol> | get <symbol> |
-                        full | realfull | baremetal]
+                        full | realfull | baremetal | all]


Adding this option was necessary to automate the generation of the query_config.c file because the previous options did not uncomment the module defines in the config.h

hanno-becker · 2018-10-17T15:54:33Z

scripts/config.pl

+    if ($action eq "full" || $action eq "realfull" || $action eq "all" || $action eq "baremetal" ) {
+        if ($action ne "all" && $line =~ /name SECTION: Module configuration options/) {
+            $done = 1;
+        } elsif ($line =~ /name SECTION: Customisation configuration options/) {


Maybe I'm blind, but how is the new cold functionally different from the old?

Recall that the aim of this PR is to add functionality in the programs/ that allows the CI scripts to query a Mbed TLS compile-time configuration option. If a queried config is defined, then the query program will return 0 to the environment and print the expansion of that macro (if any, for example MBEDTLS_AES_C does not expand to anything).

The idea of this PR is not only to write the C code for that, but also to add a script to automatically generate that code to reduce the maintenance burden. However, for the generate_query_config.pl script to work correctly, we assume that the default Mbed TLS config (i.e. include/mbedtls/config.h) has definitions (commented or uncommented, it does not matter) for every Mbed TLS config in the public API (note that this is what already happens, we just need to use the information). To generate the C code for the querying, the script does the following:

Uncomment all the #define in the default config

Run the C preprocessor on the default config to print all the defined macros

Taking care to exclude the #include "check_config.h" line and the multiple inclusion define.

Parse the preprocessor output to extract all the Mbed TLS compile time configs and generate the C source for the query program.

The problem is that the old functionality (i.e. full, realfull, baremetal) operates from the beginning of the config.h until this line. That means that if you run something like ./scripts/config.pl realfull you will not enable macros such as MBEDTLS_MPI_WINDOW_SIZE in the config file. This is what we want for the purposes of documentation, etc, but does not work well with the generate script. Theall option uncomments every define in the file without exceptions.

An alternative is to just avoid the preprocessor altogether and use a regex in perl to get the definitions. Originally, I used the preprocessor because I also wanted to extract the macro expansion, and that is harder to get neatly with regexes (hence why so many problems with ssl-opt.sh in OS X). However, I ended up not using the expansion for anything, but I like the idea of using the preprocessor because the output is quite neat and well defined. I have no particular preference with either approach, so I am happy to remove it if we really do not want this extra all option in the config.pl.

hanno-becker · 2018-10-17T15:58:39Z

scripts/generate_query_config.pl

+    }
+}
+
+# Read the fill format file into a string


Typo: fill -> full

hanno-becker

Overall, the PR looks very good to me, and I only spotted a small typo in the query config generation script. Apart from that, it's not clear to me why the configuration querying was introduced as part of ssl_client2 and ssl_server2, and not as a separate program in the programs/test folder. @andresag01, could you clarify that?

andresag01 · 2018-10-22T20:29:01Z

@hanno-arm: Thanks for the review.

Apart from that, it's not clear to me why the configuration querying was introduced as part of ssl_client2 and ssl_server2, and not as a separate program in the programs/test folder.

I personally do not mind whether it is a separate program or part of ssl_client2 or ssl_server2. This is what @sbutcher-arm suggested and I don't have any objections to either approach. As you can see, the changes for ssl_server2 and ssl_client2 are very small and the query code is fully contained in the query_config.c file. Adding a new app would take slightly more code, but is also very straight forward.

I also replied to the other comment in your review. I will wait for a response before any rework.

andresag01 · 2018-10-23T18:58:25Z

@hanno-arm: I have reworked the PR to remove the changes to config.pl and to not use the C preprocessor.

k-stachowiak

@andresag01 I think it is a good execution of the underlying concept, but I have some design related questions. Could you, please, clarify the things I pointed out?

k-stachowiak · 2018-10-24T15:12:50Z

programs/ssl/ssl_client2.c

@@ -358,6 +362,8 @@ struct options
    int etm;                    /* negotiate encrypt then mac?              */
 } opt;

+int query_config( const char *config );


I assume that this line has been put here and in the other program in order not to introduce a header file, which may mess with the concept of the public API. Nevertheless, this looks a bit odd, and I would opt for a cleaner solution. I think that @hanno-arm's suggestion to put the code in a separate program would solve the issue in a simpler way. I would also think, how this change impacts the value of the program as an example - maybe we should avoid addind non-critical complexity to the example programs.

This is here because I did not think it was necessary to add a header file because of a 1 line function declaration. I am not particularly worried about the API because this is an example application, not the library.

I think that @hanno-arm's suggestion to put the code in a separate program would solve the issue in a simpler way.

Please see the discussion with @sbutcher-arm and @hanno-arm regarding stand-alone app vs ssl_client2/ssl_server2 extension.

k-stachowiak · 2018-10-24T15:14:19Z

tests/ssl-opt.sh

+    #
+    # Note that if the configuration is not defined or is defined to nothing,
+    # the output of this function will be an empty string.
+    ${P_SRV} "query_config=${1}"


If only the server is used for the query_config functionality, why was it also added to the client example?

Please see the discussion of stand-alone app vs ssl_client2/ssl_server2.

simonbutcher · 2018-10-25T22:21:13Z

Apart from that, it's not clear to me why the configuration querying was introduced as part of ssl_client2 and ssl_server2, and not as a separate program in the programs/test folder.

This is my preference. The idea being, if presented with ssl_client2 and ssl_server2 as binaries, you can query their build configurations if you're using them. If we have a separate application, it has to be with those binaries to be usable.

Our samples applications can have a life outside of just being built within the source directory. We deliver Mbed TLS and that's how we deal with it everyday, but some users actually use these sample programs. So from that point of view, I think it makes a better feature of ssl_client2/ssl_server2then as a standalone test application.

hanno-becker · 2018-10-26T08:03:33Z

This is my preference. The idea being, if presented with ssl_client2 and ssl_server2 as binaries, you can query their build configurations if you're using them. If we have a separate application, it has to be with those binaries to be usable.

@sbutcher-arm I see your point, but for use of Mbed TLS outside of ssl_server2/ssl_client2 I think it's unclear why you would use server/client example applications to query the configuration.

I am quite strongly in favour of a separate application for configuration querying (which doesn't mean we can't also have it as a command line parameter in ssl_server2/ssl_client2), but it's not a blocker for me, and in the end it's @sbutcher-arm's decision anyway.

simonbutcher · 2018-10-26T08:41:22Z

I think it's unclear why you would use server/client example applications to query the configuration.

Because you're presented with a binary, you don't know who built it or its origins, and you want to know how it was configured.

hanno-becker · 2018-10-26T08:47:49Z

Because you're presented with a binary, you don't know who built it or its origins, and you want to know how it was configured.

Yes, but I meant outside of the use through ssl_server2 and ssl_client2.

andresag01 · 2018-10-26T09:11:50Z

@hanno-arm @sbutcher-arm: As I mentioned before, I personally dont have a strong feeling about either approach. How about if we have the option to query the configuration through ssl_client2/ssl_server2 AND as a separate query_config application? Its not hard to do this and most of the code is going to be in a shared file anyways...

hanno-becker · 2018-10-26T09:17:13Z

@andresag01 I'd like that.

simonbutcher · 2018-10-26T15:22:07Z

@andresag01 - I'm ok with that approach, as long as we can do it quickly.

andresag01 · 2018-10-26T17:51:21Z

@sbutcher-arm @hanno-arm @k-stachowiak: I have created the stand-alone app (which I called query_compile_time_config to not collide with the name of the shared file query_config.c), hopefully this addresses the concerns raised.

Could you please review once again?

simonbutcher

I've made some observations on the PR, some of the comments, and the exitcode in the test app. None require rework, unless you have the time.

simonbutcher · 2018-10-28T21:44:31Z

programs/test/query_compile_time_config.c

+        return( MBEDTLS_EXIT_FAILURE );
+    }
+
+    return( query_config( argv[1] ) );


I don't like returning as our exitcode the return value of query_config(). Can we test for zero, and return MBEDTLS_EXIT_FAILURE/MBEDTLS_EXIT_FAILURE success?

I can do that, but the reason why I originally didnt is because that value is potentially different across operating systems/platforms. I wanted to have a well defined value that never changes, so I chose 0 and 1

Minor/none: AFAIK, for embedded devices compatibility, at least in some example programs, we prefer mbedtls_exit() to return(). I don't see it being a problem wrt. this PR though. We may raise an issue, general for all the affected programs, or maybe I have got it wrong, and for this program is doesn't matter.

simonbutcher · 2018-10-28T21:46:26Z

scripts/generate_query_config.pl

@@ -0,0 +1,62 @@
+#! /usr/bin/env perl


Okay, you've written this in Perl which is fine, but we're supposed to be transitioning from Perl to Python for the test scripts, so it should have really been in Python.

Please leave it as a it is, and don't rewrite it! This is just an observation.

simonbutcher · 2018-10-28T21:58:15Z

programs/ssl/query_config.c

@@ -0,0 +1,2465 @@
+/*
+ *  Query configuration information


Can we make this clearer? I'd suggest:

Query Mbed TLS Library compilation configurations from config.h

simonbutcher · 2018-10-28T22:00:07Z

programs/ssl/query_config.c

+#define mbedtls_printf printf
+#endif /* MBEDTLS_PLATFORM_C */
+
+/* Include all the headers with public APIs in case they modify any configs */


The headers don't (or shouldn't) 'modify any configs' but define defaults if they're not already defined. They shouldn't replace or redefine them.

Can we make the comment clearer?

simonbutcher · 2018-10-28T22:02:56Z

programs/ssl/query_config.c

+#define MACRO_EXPANSION_TO_STR(macro)   MACRO_NAME_TO_STR(macro)
+#define MACRO_NAME_TO_STR(macro)        strlen( #macro ) > 0 ? #macro "\n" : ""
+
+int query_config( const char *config )


This function looks like a lot of work. When we discussed adding this feature, I imagined we'd add one or two symbols to fix the current problems and not the entire set, so I wasn't expecting this function and the whole set!

I'm fine with what we have, but it does feel like a little overkill for the bug we had!

Well, my thinking was that it would be slightly irritating to have to modify this program manually whenever we want to add a new config or new tests...

You turned it around so quickly, the time spent on it doesn't matter, I guess you're just overachieving. ;)

programs/ssl/query_config.c

simonbutcher · 2018-10-28T22:13:35Z

Travis is failing! Two issues:

0.04s$ tests/scripts/check-generated-files.sh
'programs/ssl/query_config.c' was either modified or deleted by 'scripts/generate_query_config.pl'
The command "tests/scripts/check-generated-files.sh" exited with 1.

That's acceptable I guess as programs/ssl/query_config.c wasn't previously present.

This however looks like a problem:

/home/travis/build/ARMmbed/mbedtls/programs/ssl/query_config.c: In function ‘query_config’:
/home/travis/build/ARMmbed/mbedtls/programs/ssl/query_config.c:115:9: error: zero-length gnu_printf format string [-Werror=format-zero-length]
         mbedtls_printf( MACRO_EXPANSION_TO_STR( MBEDTLS_HAVE_ASM ) );
         ^
/home/travis/build/ARMmbed/mbedtls/programs/ssl/query_config.c:147:9: error: zero-length gnu_printf format string [-Werror=format-zero-length]
         mbedtls_printf( MACRO_EXPANSION_TO_STR( MBEDTLS_HAVE_TIME ) );
         ^
/home/travis/build/ARMmbed/mbedtls/programs/ssl/query_config.c:155:9: error: zero-length gnu_printf format string [-Werror=format-zero-length]
         mbedtls_printf( MACRO_EXPANSION_TO_STR( MBEDTLS_HAVE_TIME_DATE ) );
         ^

andresag01 · 2018-10-29T11:51:03Z

@sbutcher-arm: Thanks for reviewing. With regards to the failures:

I will look into the check-generated-files.pl failure.
The second failure is because apparently GCC does not like a print statement with an empty forma string i.e. mbedtls_printf( "" ). I will look into it, probably something like mbedtls_printf( "%s", "" ) fixes the problem.

k-stachowiak

I approve the code changes, but I tried building this PR in MSVC, and unfortunately it fails. There are two issues:

The projects that use the query_config( ) function don't include the query_config.c file, which results in linker failing with unresolved external symbol
Apparently (I don't know this area very well), MSVC's macro expansion works quite differently, and the double expansion of MACRO_EXPANSION_TO_STR macro results in a void result instead of "". OTOH, when the MACRO_NAME_TO_STR is used on MSVC for the same purpose, it works; at least on my setup.

andresag01 · 2019-02-07T10:41:10Z

@mpg: I rebased to resolve the conflicts.

mpg

Re-approving after rebase

mpg · 2019-02-07T11:20:26Z

@andresag01 The CI wants you to re-rerun scripts/generate_query_config.pl :)

mpg · 2019-02-08T11:34:46Z

@andresag01 Once you've updated the generated file, can you please:

ping @k-stachowiak and myself for re-review
check if the PR applies cleanly to 2.16 as well: if so, mention it in the description, else create a backport PR?
Thanks!

andresag01 · 2019-02-11T22:02:14Z

@mpg @k-stachowiak: I fixed the check-generated-files.pl problem. Also, the Mbed TLS 2.16 PR is at #2389

mpg

LGTM

mpg · 2019-02-12T09:48:24Z

2.16 backport is now ready for merge in #2389 - no other backport required, so removing "needs: backports"

simonbutcher · 2019-02-19T15:53:20Z

Reviewed, approved, passing CI, and backports ready, so promoting to 'ready for merge'.

Patater · 2019-02-21T16:46:26Z

This fails ./tests/scripts/check-generated-files.sh

'programs/ssl/query_config.c' was either modified or deleted by 'scripts/generate_query_config.pl'

Demoting to needs work.

Patater · 2019-02-21T17:02:44Z

I have a fix available that can be force pushed to this branch. It required rebasing to pick up PSA-related changes so those could be added to query_config, so a diff will be a bit more difficult.

Available here: https://github.com/Patater/mbedtls/tree/iotssl-2555-config-detection-2

andresag01 · 2019-02-21T20:48:23Z

@Patater: Agreed, the problem is that the branch that this PR was applied to contains more config options that were not in the version of the library that this was not based on. Would you like me to apply your patch to this PR? Note that this would cause the CI on this PR to fail. I think it would be best if the gatekeeper runs the generate files script after merging.

Patater · 2019-02-22T10:02:32Z

Applying my patch wouldn't work unless the base was also updated. I'd prefer to rebase this PR and include the updated generated files, so we can review that code. But, since this is auto-generated code, we can do it as part of gatekeeping.

andresag01 · 2019-02-24T20:14:23Z

@Patater: Thanks for clarifying.

andresag01 requested a review from simonbutcher October 17, 2018 09:02

andresag01 commented Oct 17, 2018

View reviewed changes

andresag01 requested a review from hanno-becker October 17, 2018 09:11

andresag01 added bug mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-platform Portability layer and build scripts needs-ci Needs to pass CI tests labels Oct 17, 2018

hanno-becker reviewed Oct 17, 2018

View reviewed changes

andresag01 force-pushed the iotssl-2555-config-detection branch from 4ebb58b to ad007c5 Compare October 23, 2018 18:56

andresag01 requested review from k-stachowiak and removed request for simonbutcher October 23, 2018 18:58

k-stachowiak reviewed Oct 24, 2018

View reviewed changes

simonbutcher reviewed Oct 28, 2018

View reviewed changes

simonbutcher added the needs-work label Oct 28, 2018

k-stachowiak suggested changes Oct 29, 2018

View reviewed changes

Fix check-generated-files.sh failure with query_config

17c53c5

andresag01 dismissed stale reviews from k-stachowiak and mpg via 17c53c5 February 7, 2019 10:40

andresag01 force-pushed the iotssl-2555-config-detection branch from ee1768e to 17c53c5 Compare February 7, 2019 10:40

andresag01 added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Feb 7, 2019

mpg previously approved these changes Feb 7, 2019

View reviewed changes

Update query config with VSNPRINTF macro

54efcb7

andresag01 dismissed mpg’s stale review via 54efcb7 February 11, 2019 21:56

mpg approved these changes Feb 12, 2019

View reviewed changes

mpg removed the needs-backports Backports are missing or are pending review and approval. label Feb 12, 2019

k-stachowiak approved these changes Feb 14, 2019

View reviewed changes

simonbutcher 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 19, 2019

Patater added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Feb 21, 2019

Patater added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Feb 22, 2019

Patater merged commit 54efcb7 into Mbed-TLS:development Feb 25, 2019

Query Mbed TLS configuration from ssl_client2 and ssl_server2 #2105

Query Mbed TLS configuration from ssl_client2 and ssl_server2 #2105

Conversation

andresag01 commented Oct 17, 2018 • edited by mpg Loading

Description

Status

Requires Backporting

Todos

Additional comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresag01 Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanno-becker left a comment

Choose a reason for hiding this comment

andresag01 commented Oct 22, 2018

andresag01 commented Oct 23, 2018

k-stachowiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbutcher commented Oct 25, 2018

hanno-becker commented Oct 26, 2018

simonbutcher commented Oct 26, 2018

hanno-becker commented Oct 26, 2018

andresag01 commented Oct 26, 2018

hanno-becker commented Oct 26, 2018

simonbutcher commented Oct 26, 2018

andresag01 commented Oct 26, 2018

simonbutcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbutcher commented Oct 28, 2018

andresag01 commented Oct 29, 2018

k-stachowiak left a comment

Choose a reason for hiding this comment

andresag01 commented Feb 7, 2019

mpg left a comment

Choose a reason for hiding this comment

mpg commented Feb 7, 2019

mpg commented Feb 8, 2019

andresag01 commented Feb 11, 2019

mpg left a comment

Choose a reason for hiding this comment

mpg commented Feb 12, 2019

simonbutcher commented Feb 19, 2019

Patater commented Feb 21, 2019

Patater commented Feb 21, 2019

andresag01 commented Feb 21, 2019

Patater commented Feb 22, 2019

andresag01 commented Feb 24, 2019

andresag01 commented Oct 17, 2018 •

edited by mpg

Loading

andresag01 Oct 22, 2018 •

edited

Loading