implement ShowExtendedSystemInfo() in version.c by gojimmypi · Pull Request #6149 · wolfSSL/wolfssl

gojimmypi · 2023-03-01T16:17:29Z

Description

Adds a platform-specific ShowExtendedSystemInfo() function.

This code is otherwise duplicated among many examples, cluttering up main in each.

The version details are expected to be generated from CMake tasks, such as the benchmark example.

I plan to add additional "interesting" application information as appropriate.

Allows displaying of build-time info like this at runtime:

I (421) Version: Extended Version and Platform Information
I (428) Version: CONFIG_IDF_TARGET = esp32
I (432) Version: LIBWOLFSSL_VERSION_STRING = 5.5.4
I (438) Version: LIBWOLFSSL_VERSION_GIT_HASH = 6dfd9506fad46aef391f8d0ba842d8f676659efe
I (446) Version: LIBWOLFSSL_VERSION_GIT_SHORT_HASH = 6dfd9506f
I (453) Version: LIBWOLFSSL_VERSION_GIT_HASH_DATE = 'Tue Feb 28 17:57:16 2023 -0800'
I (461) Version: CONFIG_ESP32_DEFAULT_CPU_FREQ_MHZ = 160 MHz
I (468) Version: Xthal_have_ccount = 1
I (472) Version: ESP32WROOM32_CRYPT is enabled.

Fixes zd#

Testing

Tested in Espressif ESP-IDF. Linux apps will compile, but the version.c is not yet included in the makefiles and otherwise has no additional information for that platform. Future PRs are expected to add platform and environment-specific details.

Edit 3/14/23:

A script to fully test:

make clean
make distclean
./autogen.sh

# enable-all test  (`version-extended-info` is enabled by default)
#./configure CC=clang  --enable-all CFLAGS=-DHAVE_STACK_SIZE && make && ./wolfcrypt/test/testwolfcrypt

# Sample  --disable-version-extended-info
./configure CC=clang --enable-trackmemory --enable-ed25519 --disable-version-extended-info CFLAGS=-DHAVE_STACK_SIZE && make && ./wolfcrypt/test/testwolfcrypt

# Sample -enable-version-extended-info
# ./configure CC=clang --enable-trackmemory --enable-ed25519 --enable-version-extended-info CFLAGS=-DHAVE_STACK_SIZE && make && ./wolfcrypt/test/testwolfcrypt

echo "nm ./src/.libs/libwolfssl_la-version.o"
nm ./src/.libs/libwolfssl_la-version.o

Now includes the options --disable-version-extended-info or --enable-version-extended-info for non-embedded builds.

Disabled

Commandline version will look like this for testwolfcrypt output with --disable-version-extended-info

[... snip ...]
  CCLD     examples/server/server
  CCLD     testsuite/testsuite.test
  CCLD     tests/unit.test
make[2]: Leaving directory '/mnt/c/workspace/wolfssl-gojimmypi'
make[1]: Leaving directory '/mnt/c/workspace/wolfssl-gojimmypi'
------------------------------------------------------------------------------
 wolfSSL version 5.5.4
------------------------------------------------------------------------------
error    test passed!
MEMORY   test passed!
base64   test passed!
asn      test passed!
[... snip ...]

Enabled

Commandline version will look like this for testwolfcrypt output with --enable-version-extended-info

[... snip ...]
  CCLD     examples/echoserver/echoserver
  CCLD     examples/server/server
  CCLD     testsuite/testsuite.test
  CCLD     tests/unit.test
make[2]: Leaving directory '/mnt/c/workspace/wolfssl-gojimmypi'
make[1]: Leaving directory '/mnt/c/workspace/wolfssl-gojimmypi'
Extended Version and Platform Information.
LIBWOLFSSL_VERSION_STRING = 5.5.4
LIBWOLFSSL_VERSION_HEX = 5005004
NOT SINGLE_THREADED
------------------------------------------------------------------------------
 wolfSSL version 5.5.4
------------------------------------------------------------------------------
error    test passed!
MEMORY   test passed!
base64   test passed!
asn      test passed!
[... snip ...]

and the usual:

./configure CC=clang --enable-trackmemory CFLAGS=-DHAVE_STACK_SIZE && make && ./wolfcrypt/test/testwolfcrypt

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

bandi13

This is a great start. It'd be better if this would be a bit more generic so that other platforms (ARM, x86, etc.) would make use of this function as well. Perhaps adding in a call to this for our binaries used in the examples would help when the support team receives a log.

bandi13 · 2023-03-01T16:34:45Z

src/version.c

Can you put this outside of the defined(WOLFSSL_ESPIDF)? That way other platforms could use it as well.

Might want to call the function ShowExtendedSystemInfo since you're printing out clock speeds and capabilities of the part as well.

Both good suggestions. Function is now ShowExtendedSystemInfo() and a bit more friendly to other platforms.

bandi13 · 2023-03-01T16:38:09Z

src/version.c

You should use a macro to print the lines. Then have a section above like so:

#if defined(WOLFSSL_ESPIDF)
#define VERSION_PRINTF(...) ESP_LOGI(TAG, ...)
#else
#define VERSION_PRINTF(...)
// or #warn "No printing capability
#endif

I wouldn't be surprised if we already have a macro like this somewhere

We may, but I put in a local VERSION_PRINTF

bandi13 · 2023-03-01T16:38:55Z

src/version.c

This stuff would be great in a general use case, for example if we compile with '--enable-debug' it would be nice to see this printed

Putting this stuff in a separate function like 'gitVersionInfo' might be better organized.

I think the end user can choose the --enable-debug option in their code around their call to ShowExtendedSystemInfo() .

I did however put things into separate functions for readability. (e.g. ShowExtendedSystemInfo_git())

bandi13 · 2023-03-01T16:40:09Z

src/version.c

Maybe then you could put the ESP32 stuff in a separate function that is guarded by WOLFSSL_ESPIDF?

Yup: see ShowExtendedSystemInfo_platform_espressif()

bandi13 · 2023-03-01T16:42:41Z

wolfssl/version.h.in

Shouldn't the definition of this come from configure.ac (i.e.: WOLFSSL_LIBRARY_VERSION)?

oh yes, good catch.

gojimmypi · 2023-03-02T19:41:52Z

@bandi13 all good suggestions. See latest update.

bandi13

There are some minor changes requested.

bandi13 · 2023-03-02T19:51:05Z

src/version.c

This needs "{}" for instance if we have in code:

if(false) WOLFSSL_VERSION_PRINTF("stuff");

then, that would always print a newline.

ah yes, good catch.

This has been implemented. Let me know if any other changes are needed.

bandi13 · 2023-03-02T19:59:43Z

src/version.c

This seems error prone when new parts come out we would have to be aware and add it to the list (not to mention propagate to customers). Instead, you should have an IF block on parts that do support ESP32WROOM32_CRYPT.

agreed. changed logic to known supported targets, everything else is an error

bandi13 · 2023-03-02T20:00:33Z

src/version.c

Don't put in unnecessary conditions. When we add support we can add those targets.

agreed. changed logic to known supported targets, everything else is an error

bandi13 · 2023-03-02T20:06:51Z

src/version.c

Negate this condition around so there is no empty block

well, no... it was actually a TODO. I instead put in a HWM from stack allocation address.

bandi13 · 2023-03-02T20:07:41Z

src/version.c

Remove extra empty line

bandi13 · 2023-03-02T20:07:46Z

src/version.c

Remove extra empty line

bandi13 · 2023-03-02T20:07:54Z

src/version.c

Remove extra empty line

bandi13 · 2023-03-02T20:08:02Z

src/version.c

Remove extra empty lines

bandi13 · 2023-03-02T20:10:01Z

src/version.c

Don't need this block

agreed. changed logic to known supported targets, everything else is an error

gojimmypi · 2023-03-04T22:34:20Z

This is currently only for CMake projects, such as Espressif.

For makefiles, I tried adding this to configure.ac at line 8519:

AM_CONDITIONAL([BUILD_VERSION],[test "$ENABLED_VERSION" = "yes"])

and this in src/include.am at line 23:

EXTRA_DIST += src/version.c

and this at line 29:

if BUILD_VERSION
src_libwolfssl@LIBSUFFIX@_la_SOURCES += src/version.c
endif

and tried this at line 56

if !BUILD_FIPS
src_libwolfssl@LIBSUFFIX@_la_SOURCES += src/version.c
endif !BUILD_FIPS

But the linker still fails with undefined reference to ShowExtendedSystemInfo

I tried adding a src/version.c \ line right above the wolfio.c:

if !BUILD_CRYPTONLY
# ssl files
src_libwolfssl@LIBSUFFIX@_la_SOURCES += \
               src/version.c \
               src/internal.c \
               src/wolfio.c \
               src/keys.c \
               src/ssl.c \
               src/tls.c

but then multiple definition error:

/usr/bin/ld: src/.libs/libwolfssl_la-version.o: in function `ShowExtendedSystemInfo':
version.c:(.text+0x0): multiple definition of `ShowExtendedSystemInfo'; src/.libs/libwolfssl_la-version.o:version.c:(.text+0x0): first defined here
collect2: error: ld returned 1 exit status

If I then remove the above if !BUILD_FIPS version section and leave the line above src/wolfio.c \ for version.c, then I'm back to undefined.

I tried these steps from @kaleb-himes

modify stuff
run make clean then make distclean (confirm that version.h doesn’t exist)
./autogen.sh
./configure
make

As there's not much interesting for other platforms, perhaps makefile support can be added in a future PR.

Here's the updated output for Espressif

I (403) Version Info: Extended Version and Platform Information.
I (413) Version Info: LIBWOLFSSL_VERSION_STRING = 5.5.4
I (423) Version Info: LIBWOLFSSL_VERSION_HEX = 5005004
I (423) Version Info: LIBWOLFSSL_VERSION_GIT_ORIGIN = https://github.com/gojimmypi/wolfssl.git
I (433) Version Info: LIBWOLFSSL_VERSION_GIT_BRANCH = ExtendedVersion
I (443) Version Info: LIBWOLFSSL_VERSION_GIT_HASH = eb736fd08f79b18510d44f5e24210954c2ffb316
I (453) Version Info: LIBWOLFSSL_VERSION_GIT_SHORT_HASH = eb736fd08
I (463) Version Info: LIBWOLFSSL_VERSION_GIT_HASH_DATE = 'Sat Mar 4 13:17:29 2023 -0800'
I (463) Version Info: CONFIG_IDF_TARGET = esp32
I (473) Version Info: CONFIG_ESP32_DEFAULT_CPU_FREQ_MHZ: 160 MHz
I (483) Version Info: Xthal_have_ccount: 1
I (483) Version Info: CONFIG_MAIN_TASK_STACK_SIZE: 55000
I (493) Version Info: CONFIG_ESP_MAIN_TASK_STACK_SIZE: 55000
I (493) Version Info: CONFIG_TIMER_TASK_STACK_SIZE: 3584
I (503) Version Info: CONFIG_TIMER_TASK_STACK_DEPTH: 2048
I (513) Version Info: Stack HWM: 3ffc135f
I (513) Version Info: ESP32WROOM32_CRYPT is enabled for ESP32.
I (523) Version Info: SINGLE_THREADED

bandi13 · 2023-03-06T14:36:05Z

There has to be a way for this to go in our regular build flow. Especially the 'git' information is super useful for all platforms. As we've discussed before, it would help at the minimum the support team whereby they have the exact version information as part of the logs. Not having this be part of configure.ac is saying there's some configuration issue somewhere. I think your block in include.am should go towards the end of the file outside of the FIPS boundary and after the library file definition. Otherwise it may get omitted or overwritten by subsequent statements in the file.

gojimmypi · 2023-03-06T16:27:40Z

There has to be a way for this to go in our regular build flow. Not having this be part of configure.ac ...

We can always add that later in a separate PR. Just because I've not yet been successful does not mean I've given up.

I think your block in include.am should go towards the end of the file outside of the FIPS boundary and after the library file definition.

Good suggestion. I've tried that as well. Clearly I'm missing something.

Especially the 'git' information is super useful for all platforms.

I completely agree.

Otherwise, any objection to merging this PR? I'd like to remove all the duplicate code from the Espressif examples soon.

gojimmypi · 2023-03-09T05:36:48Z

Jenkins retest this please

gojimmypi · 2023-03-20T23:52:12Z

I have some proposed changes, not yet applied here as related to #6123 introspection.

gojimmypi · 2023-03-23T17:57:26Z

Jenkins retest this please

gojimmypi · 2023-03-23T20:43:21Z

Jenkins retest this please

bandi13 · 2023-03-28T17:28:19Z

Jenkins retest this please

gojimmypi · 2023-03-30T00:40:09Z

I believe I have all build issue resolved. The last problematic one was needing to add the !defined(WOLFCRYPT_ONLY) for the new ShowExtendedSystemInfo in test.c:

#if defined(HAVE_VERSION_EXTENDED_INFO) && !defined(WOLFCRYPT_ONLY)
        ShowExtendedSystemInfo();
#endif

I'm still getting a No space left on device error in PRB-generic-config-parser that I cannot explain, but seems unlikely related to the changes to the 6 files in this PR.

*edit: here's a script to check the previously problematic build:

make clean
make distclean
./autogen.sh
./configure --enable-aesgcm=table --enable-all-crypto --enable-cryptonly --enable-crypttests --disable-asm --disable-fastmath && make && ./wolfcrypt/test/testwolfcrypt

dgarske

I like what you are trying to do here, but I don't like the solution. This is very ESP centric and should not be in the library. It should be in the ESP specific code or even the ESP port code. Why not extend the existing HAVE_WC_INTROSPECTION?

dgarske · 2023-04-04T16:34:40Z

src/include.am

 EXTRA_DIST += src/ssl_asn1.c
 EXTRA_DIST += src/ssl_bn.c
 EXTRA_DIST += src/ssl_misc.c
+EXTRA_DIST += src/version.c


Adding a new files requires updating all the other build platforms like CMake, Visual Studio projects, etc. Please don't do this action unless a new file is absolutely required.

This shouldn't be a reason not to separate out functionality into smaller files. Our files are too big as it is. Instead, maybe update the other build platforms.

This is very ESP centric

Indeed this is very ESP centric at the moment, as I've been doing mostly Espressif development. My plan is to have an infrastructure in place to allow others to add their own platform-specific details as needed & desired. When doing so, and only as needed, the respective build & make platforms could be updated.

Although Espressif-specific now, I do see there being significant value in having a template of common system information for all platforms and all products. That would seem to be a valuable resource for customers to provide when asking for support.

It should be in the ESP specific code or even the ESP port code

I see your point here on the Espressif-specific details. I envisioned all of the "hardware / configuration overview" details being in a single file. As hardware typically has a version as well, that's another reason I was thinking of the version.c as a home. for this information.

If instead we had something way down in the ./port/expressif, what structure would you use? Perhaps a hardware specific ShowHardwareConfiguration() that is implemented for each platform, then called as appropriate from a single function elsewhere?

don't [add version.c] unless a new file is absolutely required

It was quite a learning curve to tease that file into the builds, so I definitely respect that comment. Still, iIntuitively for me, it seems to make sense to have this information in the version.c file. That seems to be a universal file & the most global across all wolfSSL libraries.

Why not extend the existing HAVE_WC_INTROSPECTION?

This is an excellent question. Although similar in concept, some of the information is mutually exclusive from introspection. @douzzer pointed out that: "HAVE_WC_INTROSPECTION requires that the configuration and build time artifacts, particularly the date/time of build and git parameters, be excluded from the build". For example: some of the information such as git hash & date may be updated, but the binary build itself should not actually change.

Given this, if you still feel strongly that I should not add the version.c, I'm happy to implement this functionality elsewhere. Any preference? Perhaps wolfcrypt/logging.c would be a appropriate? I certainly agree with @bandi13 about the big files. I'm in favor of more specific, smaller files when possible.

dgarske · 2023-04-04T16:36:08Z

wolfssl/version.h.in

 extern "C" {
 #endif

+#ifdef HAVE_CONFIG_H


These config.h and settings.h go in the .c, not the .h.

I added the config.h and settings.h using the model from ssl.h; in particular I needed to include visibility.h in order to use the WOLFSSL_API macro, without which, the linker could not find ShowExtendedSystemInfo(). Is there a preferred way of doing this?

dgarske · 2023-04-04T16:36:32Z

src/version.c

+
+#endif /* NO_VERSION_EXTENDED_INFO */
+
+#ifdef __cplusplus


The __cplusplus logic only goes in the .h. It is already there, just remove in .c.

gojimmypi · 2023-04-06T09:41:34Z

This PR will be mothballed for the time being.

I've moved the ShowExtendedSystemInfo() to the wolfcrypt/src/port/Espressif/esp32_util.c

The functionality can still be used in main.c Espressif code like this:

#ifdef HAVE_VERSION_EXTENDED_INFO
    esp_ShowExtendedSystemInfo();
#endif

There's a new ALLOW_BINARY_MISMATCH_INTROSPECTION option in user_settings.h that will allow for binary-mismatched introspection: essentially allowing all the introspection features to be displayed, along with all of the ShowExtendedSystemInfo details.

PR coming soon from my WIP ED25519_SHA2_fix branch.

See also #6234 Espressif Imporvements summary.

bandi13 · 2023-04-06T14:02:04Z

configure.ac

+
+if test "$VERSION_EXTENDED_INFO" = "no"
+then
+    AM_CFLAGS="$AM_CFLAGS -DNO_VERSION_EXTENDED_INFO"


You probably don't need this as I don't think you use NO_VERSION_EXTENDED_INFO anywhere. Better to have the affirmative form of options, so that we can avoid things like #ifndef NO_VERSION_ to check that the extended info exists.

gojimmypi · 2024-06-18T17:06:52Z

We decided not to implement the extended version information in version.c at this time.

There is however some extended information in the esp32_util.c for the Espressif embedded target.

Closing this lingering draft PR....

gojimmypi added the enhancement label Mar 1, 2023

gojimmypi requested review from bandi13 and dgarske March 1, 2023 16:17

gojimmypi self-assigned this Mar 1, 2023

bandi13 suggested changes Mar 1, 2023

View reviewed changes

gojimmypi mentioned this pull request Mar 1, 2023

TLS handshake fails for 4096 bit CA on ESP32s3 #6108

Closed

gojimmypi force-pushed the ExtendedVersion branch from 9b2e1b4 to fb77459 Compare March 2, 2023 19:34

bandi13 suggested changes Mar 2, 2023

View reviewed changes

gojimmypi force-pushed the ExtendedVersion branch from fb77459 to eb736fd Compare March 4, 2023 21:17

gojimmypi changed the title ~~implement ShowExtendedVersionInfo()~~ implement ShowExtendedSystemInfo() in version.c Mar 7, 2023

gojimmypi force-pushed the ExtendedVersion branch from eb736fd to 3103a88 Compare March 15, 2023 01:04

gojimmypi requested a review from douzzer March 15, 2023 01:05

gojimmypi assigned douzzer Mar 15, 2023

gojimmypi force-pushed the ExtendedVersion branch 2 times, most recently from d745786 to e44abef Compare March 15, 2023 02:00

gojimmypi marked this pull request as draft March 20, 2023 23:52

gojimmypi force-pushed the ExtendedVersion branch from 6e3eb61 to 7991c06 Compare March 21, 2023 17:12

gojimmypi marked this pull request as ready for review March 21, 2023 17:32

gojimmypi mentioned this pull request Mar 24, 2023

ESP32 Espressif Improvements - Roadmap Summary #6234

Closed

72 tasks

gojimmypi force-pushed the ExtendedVersion branch 2 times, most recently from 1297153 to 736e30f Compare March 28, 2023 23:16

gojimmypi marked this pull request as draft March 29, 2023 08:19

gojimmypi force-pushed the ExtendedVersion branch 3 times, most recently from 8d7e91c to 98bfc9c Compare March 29, 2023 23:20

gojimmypi marked this pull request as ready for review March 30, 2023 00:40

gojimmypi requested a review from bandi13 March 30, 2023 00:40

gojimmypi mentioned this pull request Apr 4, 2023

Software-only support for recent esp32 targets #6256

Closed

4 tasks

gojimmypi force-pushed the ExtendedVersion branch from b6ec6d3 to afb7b8d Compare April 4, 2023 12:32

implement ShowExtendedSystemInfo() via version-extended-info for v5.6

aeebb9f

gojimmypi force-pushed the ExtendedVersion branch from afb7b8d to aeebb9f Compare April 4, 2023 13:16

dgarske self-assigned this Apr 4, 2023

dgarske requested changes Apr 4, 2023

View reviewed changes

dgarske unassigned dgarske and douzzer Apr 4, 2023

gojimmypi marked this pull request as draft April 4, 2023 19:19

gojimmypi removed request for bandi13 and douzzer April 6, 2023 10:28

bandi13 reviewed Apr 6, 2023

View reviewed changes

gojimmypi closed this Jun 18, 2024

Conversation

gojimmypi commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Edit 3/14/23:

Disabled

Enabled

Checklist

Uh oh!

bandi13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gojimmypi commented Mar 2, 2023

Uh oh!

bandi13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gojimmypi commented Mar 1, 2023 •

edited

Loading

gojimmypi commented Mar 30, 2023 •

edited

Loading