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

[krishna t]

On Sun, May 1, 2022 at 9:12 PM Jouni Malinen <j at w1.fi> wrote:
>
> 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.
Yes, this is unused, I can remove this, and as this is the only change
no need for a separate commit.
>
>
> > 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.
Sorry, I wasn't too sure about this and have not tried to contact the ESP
team.  As they own the code, I cannot change the license type, and
basic googling suggested that we can use a mix of Apache2.0
and BSD as long as we keep the original license as is. Though some
claim the resulting code has to be Apache2.0 as it is less permissive
than BSD.
>
> 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.
So, IIUC, using SPDX ID's implies license text, no? I can surely add the
license file if that satisfies the needs. And also, I guess I should add a note
at the top that I have modified these.
>
> > +// 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.
Yes, this is copied as is, and that post is licensed under
https://creativecommons.org/licenses/by-sa/4.0/ (mentioned in the share link)
 and mentioning the source should satisfy the license IIUC. I can change
"Lifted from" to "original source" and move them to a separate file with SPDX
ID for those two functions?
And anyways, these changes are not strictly needed, they were made
to pass the hwsim tests.So, can use a simple switch case and avoid this
algother.
>
> > 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.
Sorry, these are local changes, will be removed.
>
> > 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?
Sorry, I was preparing another patch, spilled into here, will be removed.
>
> > @@ -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?
Sorry, these are local changes, will be removed.
> > @@ -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.
Okay. I will use $(error) as we do for other checks.
> --
> Jouni Malinen                                            PGP id EFC895FA