20231012-keylog-export-warning-fix

[douzzer]

configure.ac and src/tls.c: fix --enable-keylog-export to warn at configure time, then build cleanly.

tested with wolfssl-multi-test.sh ... check-self check-file-modes check-source-text check-shell-scripts check-configure all-gcc-c99.

[tmael]

Do we want the configuration process to halt with an error, requiring the user to take action?

I didn't notice the warning when using the following configuration.
./configure --enable-haproxy --enable-keylog-export
I had to check the config.log to spot this warning (Then, had to scroll ./configure back to spot the same warnings):
configure:18518: WARNING: Keylog export enabled -- Sensitive key data will be stored insecurely.

Not sure if I missed something but my suggested solution is just as good as this one except it doesn't affect non-haproxy users.

#ifndef WOLFSSL_HAPROXY
    #warning The SHOW_SECRETS and WOLFSSL_SSLKEYLOGFILE options should only be used for debugging and never in a production environment
#endif

[douzzer]

We don't want the config process to halt with an error here. The presumption is that when someone manually adds --enable-keylog-export they know what they're doing, and if they don't review the output from configure to see the warning (try with --quiet` and then it's the only thing printed), well, we've done what we can and it's good enough.

We also don't want to make this haproxy-specific because it isn't haproxy-specific, it's keylog-specific.

[douzzer]

note "CAVP self-test" failure is false positive on the poorly chosen branch name 20231012-keylog-export-warning-fix. all tests actually passed.

[douzzer]

oh I forgot to link #6852 to this.

[tmael]

The presumption is that when someone manually adds --enable-keylog-export they know what they're doing

Given this presumption, the proposed fix is acceptable. However, I have a minor suggestion Nitpick: I would have expected that option #3 below would be a better choice.

Before:
no warnings with ./configure
build fails for all users

After:
warnings in the log
builds cleanly for all users

Optional, after (with #ifndef WOLFSSL_HAPROXY guard):
warnings in the log
Build succeeds only for haproxy, while it fails for other users.

[douzzer]

We don't want --enable-keylog-export to fail to build for anyone -- the whole point of activating a feature through configure is to have the autotools config take care of setting things up for a successful build.

The haproxy people were understandably surprised when they found that this wasn't the case. That's not specific to haproxy -- anyone will be surprised if activating a feature through configure results in a configuration that can't build.

[tmael]

I thought I heard David say that we don't want someone to inadvertently enable it in a production environment. The specific purpose of the error is for someone to intentionally modify or edit the code to disable it. What I suggested only serves to mitigate potential harm (Ensure haproxy is satisfied without impacting anyone else.). I'll leave it up to you since @dgarske approved the PR, which seems to contradict what was discussed anyway.

[dgarske]

I thought I heard David say that we don't want someone to inadvertently enable it in a production environment. The specific purpose of the error is for someone to intentionally modify or edit the code to disable it. What I suggested only serves to mitigate potential harm (Ensure haproxy is satisfied without impacting anyone else.). I'll leave it up to you since @dgarske approved the PR, which seems to contradict what was discussed anyway.

There are valid reasons to support a way to build this feature without a warn->error. We just want it to be intentional. If it requires --enable-keylog-export or WOLFSSL_KEYLOG_EXPORT_WARNED explicitly to be set, I am okay with this. Ideally having this feature on won't automatically log the keys unless an API is called. Currently with TLS v1.2 and this happens automatically to WOLFSSL_SSLKEYLOGFILE_OUTPUT. If we can disable that default behavior then we can make the warning less obvious. Not to mention SHOW_SECRETS which puts(pmsBuf); it to the console...

[douzzer]

There are several motivations in play here. First, we don't want to take on technical debt around a special-case clause with no technical rationale (here, a warning issued, except for haproxy "because reasons") that will surprise in the future. Second, we can give haproxy what they want without a special-case clause, because they're autotools users -- i.e., we're retaining a codebase that will warn, one way or the other, when the feature is enabled. Third, there is plenty of precedent for other configurable features with security implications (--disable-harden and --enable-secure-renegotiation for example) that are just called out in the output from configure. We just keep them out of the default build and out of --enable-all and --enable-all-crypto. Notably, --enable-haproxy doesn't enable keylog export -- that has to be done with an explicit command line option.

[tmael]

Thanks, @douzzer and @dgarske, It appears that there is sufficient intent behind the changes.