[PATCH] Add support for mbedtls crypto library for STA mode

[Jouni Malinen]

On Sun, Apr 24, 2022 at 09:40:34PM +0530, krish271828 at gmail.com wrote:
> This is ideal for low footprint embedded systems, the patch adds support
> for STA mode, the support is minimal, only mandatory stuff is added
> 
>   * DH group19 only for SAE
>   * OWE, DPP and PASN are not supported as ECDH support is yet to be
>     added.
>   * EAP-TEAP is not supported yet
> 
> This implementation is based on work of ESP team [1], with below changes
>   * decouple from esp_wifi
>   * avoid any changes to core wpa_supplicant (esp. for WPA3)
>   * port to 3.1.0 mbedtls
> 
> Tested WPA2/WPA3 associations personal/enterprise. Also, add a
> configuration file with mbedtls for build tests.
> 
> Code auto-formatted using wpa_supplicant/binder/.clang-format.
> 
> [1] - https://github.com/espressif/esp-idf/tree/master/components/wpa_supplicant

> diff --git a/src/crypto/crypto.h b/src/crypto/crypto.h
> +int crypto_bignum_bits(const struct crypto_bignum *a);

This does not seem to be used anywhere. I would prefer to separate
crypto.h additions into their own patch to keep things clearer. That
said, if this is not really going to be used anywhere, it does not look
helpful to add this function at this point.

> diff --git a/src/crypto/crypto_mbedtls-bignum.c b/src/crypto/crypto_mbedtls-bignum.c
> +/*
> + * SPDX-FileCopyrightText: 2015-2021 Espressif Systems (Shanghai) CO LTD
> + *
> + * SPDX-License-Identifier: Apache-2.0
> + */

Is there particular need for using the Apache license for these new
files? I would prefer to use the same BSD license for all of the
hostap.git implementation and there would need to be strong
justification for using something different.

This patch would seem to make hostap.git contents not comply with the
license requirements ("You must give any other recipients of the Work or
Derivative Works a copy of this License;" and the License text is not
included here), so even if I were to accept a different license to be
used, I could not accept something that would not meet the actual
license requirements.

> +// Lifted from https://stackoverflow.com/a/47117431
> +char *strremove(char *str, const char *sub)
...

> +// Lifted from: https://stackoverflow.com/a/779960
> +// You must free the result if result is non-NULL.
> +char *str_replace(char *orig, char *rep, char *with)
...

Can you please clarify what you mean with "lifted from"? I read this as
"taken without explicit permission and without identifying under which
license this is used". If that is accurate, I cannot accept such
contribution due to unclear license situation.

> diff --git a/tests/build/run-build-tests.sh b/tests/build/run-build-tests.sh
> @@ -8,13 +8,10 @@ popd > /dev/null
>  echo "Build test directory: $DIR"
>  echo
>  
> -for i in build-hostapd-*.config; do
> -    ./build-hostapd.sh $DIR $i
> -done
> -
> -for i in build-wpa_supplicant-*.config; do
> +for i in build-wpa_supplicant-mbedtls-3.1.0.config; do
>      ./build-wpa_supplicant.sh $DIR $i
>  done
>  
>  echo
>  echo "Build test directory: $DIR"
> +cat $DIR/wpa_supplicant-mbedtls-3.1.0.log-*

These changes are clearly not appropriate. It is fine to add a new test
config, but the test script must not be broken for other uses.

> diff --git a/wpa_supplicant/Makefile b/wpa_supplicant/Makefile
> @@ -35,7 +35,7 @@ export INCDIR ?= /usr/local/include
>  export BINDIR ?= /usr/local/sbin
>  PKG_CONFIG ?= pkg-config
>  
> -CFLAGS += $(EXTRA_CFLAGS)
> +CFLAGS += $(EXTRA_CFLAGS) -Werror

How is this connected to this particular patch?

> @@ -145,6 +145,7 @@ ifndef CONFIG_ELOOP
>  CONFIG_ELOOP=eloop
>  endif
>  OBJS += ../src/utils/$(CONFIG_ELOOP).o
> +OBJS_p += ../src/utils/$(CONFIG_ELOOP).o
>  OBJS_c += ../src/utils/$(CONFIG_ELOOP).o

Why is this needed?

> @@ -258,9 +259,12 @@ ifdef CONFIG_SAE
>  CFLAGS += -DCONFIG_SAE
>  OBJS += ../src/common/sae.o
>  ifdef CONFIG_SAE_PK
> +# NO ECDH support yet
> +ifneq ($(CONFIG_TLS), mbedtls)
>  CFLAGS += -DCONFIG_SAE_PK
>  OBJS += ../src/common/sae_pk.o
>  endif
> +endif

I'd prefer the build to fail instead of hide the fact that explicitly
enabled build option could not be included, i.e., I would not add this
type of exceptions into Makefile.

-- 
Jouni Malinen                                            PGP id EFC895FA