Hi both,
[...]
> > > > We need much more granularity here, and to re-think some existing > > symbols too perhaps. What we should be able to do is pick mbedTLS
or
> > "legacy SW implementation" or "HW implementation" for the various > > algorithms, and that in turn can have some higher level grouping
to it.
> > This should then negate a bunch of the Makefile work you're doing
as we
> > won't have CONFIG_SHA256 enabled as we'll have CONFIG_MBEDTLS_LIB_SHA256 > > or whatever enabled. > > > > I think we should use CONFIG_MBEDTLS_LIB_[CRYPTO,X509,TLS] for
high-level
> grouping. > Underneath, the CONFIG_SHA[1,256,512] switches (and other crypto
options)
> can be > used as sub build options in both MbedTLS and "legacy libs". > > Take hash as an example, if the users prefer to use MbedTLS other
than
> "legacy libs" for > hash operation, CONFIG_MBEDTLS_LIB_CRYPTO should be defined as the
main
> switch > (the users can still prefer to use "legacy libs" for X509 by > keeping CONFIG_MBEDTLS_LIB_X509 > disabled). > Then enable the algorithms they need (e.g. CONFIG_SHA256) - the
algorithm
> options works > for both MbedTLS and "legacy libs". > > HW implementations with MbedTLS (aka, Alternative algorithms in
MbedTLS)
is > another > topic which is not covered in this patch set (It needs to migrate
each
> vendor's solution under > MbedTLS alternative algorithm). > Current patch set is focused on SW implementation with MbedTLS.
The "easy" problem with what's in v3 is that X509 and CRYPTO are select'd under the main heading.
Not sure if I get what you mentioned, currently all MbedTLS options are under Library routines > Security support Do you think we should keep them in other places?
The harder problem is that we intentionally have granularity for SHA256, SHA512, etc, etc and all of that goes away with the current Kconfig option if you select mbedTLS.
We
need to bring that back. And we shouldn't need to have all of the ifneq statements in Makefiles because both CONFIG_SHA256 and CONFIG_MBEDLTS_LIB_CRYPTO_SHA256 will not be true (Or possibly, CONFIG_SHA256 gates things U-Boot's internal API for sha256'ing something and CONFIG_LEGACY_SHA256 controls building lib/sha256.c).
I think we should not introduce new ones like CONFIG_LEGACY_,
CONFIG_SHA[1,256,512] should be used no matter whether MbedTLS is enabled or not. I understand your concern, I will bring CONFIG_SHA[1,256,512] back when MbedTLS is enabled. Those options should control the options in the
MbedTLS
default config file.
My concern is that we do not have the correct level of granularity, and that can partly be seen by the number of ifneq(...) statements being added around already conditional logic. We should have almost none of those, in the end, is what I'm saying. We have a mechanism for configuring the build, Kconfig, and that should drive the decisions as much as possible.
The `ifneq(ONFIG_MBEDTLS_LIB_*)` statements are due to the fact that we still need lib/Makefile and lib/crypto/Makefile when building hash and x509 stuffs with MbedTLS enabled. To address this, I guess we have to first refactor all "legacy libs" that will be replaced by MbedTLS: Move md5, sha* from lib to to a new dir lib/hash and move public_key, rsapubkey*, rsa_helper, x509*, pkcs7*,mscode* from lib/crypto to a new dir lib/x509. When they are all independent modules with separated Makefile, we can remove `ifneq(ONFIG_MBEDTLS_LIB_*)` and all can be driven in lib/Makefile.
Is that something you expect? If yes I can do this for v4, or put it into another prerequisite/refactor series.
We should not need to do that because we should not have CONFIG_SHA256 set if we are not building lib/sha256.c at that stage, is what I'm saying. CONFIG_LEGACY_SHA256 should control it and CONFIG_SHA256 should control the API and CONFIG_MBEDTLS_LIB_CRYPTO_SHA256 should control the mbedTLS version, and this should either expand on, or if needed update/rework, the mechanism that lets us also have say CONFIG_ARMV8_CE_SHA256 for using that HW based version of the support.
But in this case we have to introduce two new Kconfigs for each algorithm - CONFIG_LEGACY_<alg> and CONFIG_MBEDTLS_LIB_CRYPTO_<alg>. CONFIG_<alg> is still there, so we have totally 3 Kconfigs for one hash alg which seems adding too much complexity.
I haven't looked at your v4 yet, but I think Tom is on the right path here. Yes, it might be a bit more work, but in the long run, it's going to be easier to maintain both in Makefiles and code.
I think Tom is saying The API should be gated by CONFIG_SHA1, CONFIG_SHA256 etc the individual implementation musty carry their own symbols e.g CONFIG_MBEDTLS_SHA256 CONFIG_LEGACY_SHA256 etc.
If any of those are selected you turn on CONFIG_SHA256
Thanks /Ilias
Moreover, this means allowing the user to select algorithms mix up with MbedTLS and legacy ones, for example, user can select both CONFIG_LEGACY_SHA1 and CONFIG_MBEDTLS_LIB_CRYPTO_SHA256 at the same time. It means to build both legacy lib and MbedTLS which does not make sense.
Why we don't have the CONFIG_MBEDTLS_LIB_CRYPTO as the higher level switch and keep CONFIG_<alg> as it was to select the algorithms? We still have the same granularity with this I think - and yes I will put CONFIG_<alg> in the MbedTLS config file so that it can control only the selected algorithms to be built. The only downside I can see here is adding a few `ifneq(ONFIG_MBEDTLS_LIB_*)` in lib/Makefile and lib/crypto/Makefile, but all the rest of things are straightforward and clean.
Thanks. Regards, Raymond