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

[Krishna Chaitanya]

On Mon, May 2, 2022 at 12:05 AM krishna t <krish271828 at gmail.com> wrote:
>
> 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.
I was waiting for you ACK on licensing before submitting a new patch with
fixes for the others, sorry if that wasn't obvious. Else, I can submit a new
patch right away. Please suggest.