Jiewen, Thanks. Option #1 makes more sense if it is the Mbedtls wrapper code.
I prefer Option #1. Breaking out into multiple repos also makes it hard to align Releases across multiple repos. We already have this as an unsolved problem for edk2-platforms repo, and I am not interested in adding more repos until we have a complete solution to do edk2-platforms releases in place. Mike > -----Original Message----- > From: Yao, Jiewen <jiewen....@intel.com> > Sent: Thursday, August 31, 2023 9:07 AM > To: Kinney, Michael D <michael.d.kin...@intel.com>; Leif Lindholm > <quic_llind...@quicinc.com>; devel@edk2.groups.io; spbro...@outlook.com; > Hou, Wenxing <wenxing....@intel.com> > Cc: af...@apple.com > Subject: RE: [edk2-devel] [edk2/add_mbedtls PATCH 0/9] *** Add > HMAC/HKDF/RSA/HASH features based on Mbedtls *** > > Hi Mike > We are using submodule for mbedtls in this patch. Copying source code is > not preferred. > > I think we are discussing multiple ways to layout the *mbedtls crypto > wrapper*. See following 4 options. > > Thank you > Yao, Jiewen > > > > -----Original Message----- > > From: Kinney, Michael D <michael.d.kin...@intel.com> > > Sent: Thursday, August 31, 2023 11:45 PM > > To: Leif Lindholm <quic_llind...@quicinc.com>; Yao, Jiewen > > <jiewen....@intel.com>; devel@edk2.groups.io; spbro...@outlook.com; > Hou, > > Wenxing <wenxing....@intel.com> > > Cc: af...@apple.com; Kinney, Michael D <michael.d.kin...@intel.com> > > Subject: RE: [edk2-devel] [edk2/add_mbedtls PATCH 0/9] *** Add > > HMAC/HKDF/RSA/HASH features based on Mbedtls *** > > > > I have not looked at the Mbedtls patches in detail yet, but I > > am curious if it is possible to add the mbedtls based library > > instances of the edk2 crypto libraries to the existing > > CryptoPkg and pull the mbedtls sources into the CryptoPkg using > > a submodule just like openssl? This way, platforms can choose > > either openssl or mbedtls library instances from CryptoPkg and > > all INFs would continue to only list CryptoPkg.dec. > > > > I think use of submodules makes the most sense for content that > > edk2 consumes as read-only and edk2 makes decisions to jump from > > one validated release to the next validated release of the submodule > > content. > > > > In general, we do not want to copy source from a different project > > into TianoCore repos because of the overhead to keep them in sync. > > An exception to this is something like cmocka where this was done > > for CI stability issues and the copy in TianoCore is an automated > > sync of the upstream repo. > > > > Thanks, > > > > Mike > > > > > > > -----Original Message----- > > > From: Leif Lindholm <quic_llind...@quicinc.com> > > > Sent: Thursday, August 31, 2023 4:15 AM > > > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; > > > spbro...@outlook.com; Hou, Wenxing <wenxing....@intel.com> > > > Cc: af...@apple.com; Kinney, Michael D <michael.d.kin...@intel.com> > > > Subject: Re: [edk2-devel] [edk2/add_mbedtls PATCH 0/9] *** Add > > > HMAC/HKDF/RSA/HASH features based on Mbedtls *** > > > > > > Like Sean, I'm very positive to the work - and I'm excited about the > > > opportunity to formalise the abstractions. > > > > > > But Sean is also asking to import the mbedTLS code outright instead > of > > > using submodules, which adds an additional dimension to the matrix > below. > > > > > > I'm not too concerned over the infrastructure change as such, but I > > > would prefer to not move the dial even further in the direction of > > > "upstream is a swarm of repositories". This adds complexity for new > > > developers. And submodules are way easier for upstream to track > external > > > projects through. At the cost of complicating the maintenance process > > > for released products. Which isn't great. > > > > > > Am I kicking the can too far down the road if I suggest we do some > > > brainstorming around ways to achieve this with the least amount of > > > friction for everyone at the plugfest in October? > > > > > > Regards, > > > > > > Leif > > > > > > On 2023-08-31 03:34, Yao, Jiewen wrote: > > > > Hi Sean/Andrew/Leif/Mike > > > > Now, I think we actually have multiple options to handle this: > > > > > > > > 1) CryptoPkg in edk2 repo (add MbedTls to existing CryptoPkg) > > > > > > > > 2) CryptoPkg in edk2 repo + a new MbedTlsCryptoPkg in edk2 repo > > > > > > > > 3) CryptoPkg in edk2 repo + MbedTlsCryptoPkg in a new repo > > > > > > > > 4) Move CryptoPkg from edk2 repo to OpensslCryptoPkg in a new repo > + > > > MbedTlsCryptoPkg in another new repo > > > > > > > > > > > > > > > > Current patch is for option 1). > > > > Sean's proposal is for option 4). > > > > > > > > I feel 4) is very aggressive. My worry is that it will involve many > > > infrastructure change such as CI, and all edk2 platforms. > > > > > > > > What about 2) or 3) ? > > > > > > > > Thank you > > > > Yao, Jiewen > > > > > > > > > > > >> -----Original Message----- > > > >> From: Yao, Jiewen > > > >> Sent: Thursday, August 31, 2023 8:10 AM > > > >> To: devel@edk2.groups.io; spbro...@outlook.com; Hou, Wenxing > > > >> <wenxing....@intel.com> > > > >> Cc: af...@apple.com; quic_llind...@quicinc.com; Kinney, Michael D > > > >> <michael.d.kin...@intel.com> > > > >> Subject: RE: [edk2-devel] [edk2/add_mbedtls PATCH 0/9] *** Add > > > >> HMAC/HKDF/RSA/HASH features based on Mbedtls *** > > > >> > > > >> Hi Sean > > > >> Thanks for the feedback. Personally, I don't have strong opinion > on this. > > > >> > > > >> Since this is a big change, I would like to have Steward member's > opinion. > > > >> > > > >> Hi Andrew/Leif/Mike > > > >> What do you think? > > > >> > > > >> Thank you > > > >> Yao, Jiewen > > > >> > > > >> > > > >>> -----Original Message----- > > > >>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > Sean > > > >>> Sent: Thursday, August 31, 2023 2:57 AM > > > >>> To: devel@edk2.groups.io; Hou, Wenxing <wenxing....@intel.com> > > > >>> Subject: Re: [edk2-devel] [edk2/add_mbedtls PATCH 0/9] *** Add > > > >>> HMAC/HKDF/RSA/HASH features based on Mbedtls *** > > > >>> > > > >>> I appreciate and really like this work to enable mbedtls but I > don't > > > >>> like the idea of adding another submodule to edk2. > > > >>> > > > >>> For a long time there has been discussion about formalizing the > > > >>> abstraction of the edk2 crypto api so that it would be practical > to > > > >>> implement edk2's crypto using various libraries. I propose we > remove > > > >>> openssl from the edk2 CryptoPkg and into the OpenSslCryptoPkg in > another > > > >>> new tianocore repository dedicated to OpenSsl. MbedTls could > then be > > > >>> checked into the MbedTlsCryptoPkg and added to another new > repository. > > > >>> This would also have the benefit of breaking the tight coupling > of edk2 > > > >>> stable tags from the crypto used in the code base (crypto has > more > > > >>> widely tracked vulnerabilities). > > > >>> > > > >>> Happy to discuss more if others have different ideas. > > > >>> > > > >>> Thanks > > > >>> > > > >>> Sean > > > >>> > > > >>> > > > >>> > > > >>> On 8/30/2023 12:52 AM, Wenxing Hou wrote: > > > >>>> *** Add BaseCryptLibMbedTls for CryptoPkg, which can be an > alternative > > > to > > > >>> OpenSSL in some scenarios. There are four features in the patch: > > > >>> HMAC/HKDF/RSA/HASH.*** > > > >>>> > > > >>>> Wenxing Hou (9): > > > >>>> CryptoPkg: Add mbedtls submodule for EDKII > > > >>>> CryptoPkg: Add mbedtls_config and MbedTlsLib.inf > > > >>>> CryptoPkg: Add HMAC functions based on Mbedtls > > > >>>> CryptoPkg: Add HKDF functions based on Mbedtls > > > >>>> CryptoPkg: Add RSA functions based on Mbedtls > > > >>>> CryptoPkg: Add all .inf files for BaseCryptLibMbedTls > > > >>>> CryptoPkg: Add Null functions for building pass > > > >>>> CryptoPkg: Add MD5/SHA1/SHA2 functions based on Mbedtls > > > >>>> CryptoPkg: Add Mbedtls submodule in CI > > > >>>> > > > >>>> .gitmodules | 3 + > > > >>>> .pytool/CISettings.py | 2 + > > > >>>> CryptoPkg/CryptoPkg.ci.yaml | 66 +- > > > >>>> CryptoPkg/CryptoPkg.dec | 4 + > > > >>>> CryptoPkg/CryptoPkgMbedTls.dsc | 280 ++ > > > >>>> .../BaseCryptLibMbedTls/BaseCryptLib.inf | 81 + > > > >>>> .../BaseCryptLibMbedTls/Bn/CryptBnNull.c | 520 +++ > > > >>>> .../Cipher/CryptAeadAesGcmNull.c | 100 + > > > >>>> .../BaseCryptLibMbedTls/Cipher/CryptAesNull.c | 159 + > > > >>>> .../BaseCryptLibMbedTls/Hash/CryptMd5.c | 234 + > > > >>>> .../BaseCryptLibMbedTls/Hash/CryptMd5Null.c | 163 + > > > >>>> .../Hash/CryptParallelHashNull.c | 40 + > > > >>>> .../BaseCryptLibMbedTls/Hash/CryptSha1.c | 234 + > > > >>>> .../BaseCryptLibMbedTls/Hash/CryptSha1Null.c | 166 + > > > >>>> .../BaseCryptLibMbedTls/Hash/CryptSha256.c | 227 + > > > >>>> .../Hash/CryptSha256Null.c | 162 + > > > >>>> .../BaseCryptLibMbedTls/Hash/CryptSha512.c | 447 ++ > > > >>>> .../Hash/CryptSha512Null.c | 275 ++ > > > >>>> .../BaseCryptLibMbedTls/Hash/CryptSm3Null.c | 164 + > > > >>>> .../BaseCryptLibMbedTls/Hmac/CryptHmac.c | 620 +++ > > > >>>> .../BaseCryptLibMbedTls/Hmac/CryptHmacNull.c | 359 ++ > > > >>>> .../BaseCryptLibMbedTls/InternalCryptLib.h | 44 + > > > >>>> .../BaseCryptLibMbedTls/Kdf/CryptHkdf.c | 372 ++ > > > >>>> .../BaseCryptLibMbedTls/Kdf/CryptHkdfNull.c | 192 + > > > >>>> .../BaseCryptLibMbedTls/PeiCryptLib.inf | 101 + > > > >>>> .../BaseCryptLibMbedTls/PeiCryptLib.uni | 25 + > > > >>>> .../BaseCryptLibMbedTls/Pem/CryptPemNull.c | 69 + > > > >>>> .../Pk/CryptAuthenticodeNull.c | 45 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptDhNull.c | 150 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptEcNull.c | 578 +++ > > > >>>> .../Pk/CryptPkcs1OaepNull.c | 51 + > > > >>>> .../Pk/CryptPkcs5Pbkdf2Null.c | 48 + > > > >>>> .../Pk/CryptPkcs7Internal.h | 83 + > > > >>>> .../Pk/CryptPkcs7SignNull.c | 53 + > > > >>>> .../Pk/CryptPkcs7VerifyEkuNull.c | 152 + > > > >>>> .../Pk/CryptPkcs7VerifyEkuRuntime.c | 56 + > > > >>>> .../Pk/CryptPkcs7VerifyNull.c | 163 + > > > >>>> .../Pk/CryptPkcs7VerifyRuntime.c | 38 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptRsaBasic.c | 268 ++ > > > >>>> .../Pk/CryptRsaBasicNull.c | 121 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptRsaExt.c | 337 ++ > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptRsaExtNull.c | 117 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptRsaPss.c | 164 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptRsaPssNull.c | 46 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptRsaPssSign.c | 231 + > > > >>>> .../Pk/CryptRsaPssSignNull.c | 60 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptTsNull.c | 42 + > > > >>>> .../BaseCryptLibMbedTls/Pk/CryptX509Null.c | 753 ++++ > > > >>>> .../BaseCryptLibMbedTls/Rand/CryptRandNull.c | 56 + > > > >>>> .../BaseCryptLibMbedTls/RuntimeCryptLib.inf | 92 + > > > >>>> .../BaseCryptLibMbedTls/RuntimeCryptLib.uni | 22 + > > > >>>> .../BaseCryptLibMbedTls/SecCryptLib.inf | 84 + > > > >>>> .../BaseCryptLibMbedTls/SecCryptLib.uni | 17 + > > > >>>> .../BaseCryptLibMbedTls/SmmCryptLib.inf | 92 + > > > >>>> .../BaseCryptLibMbedTls/SmmCryptLib.uni | 22 + > > > >>>> .../SysCall/ConstantTimeClock.c | 75 + > > > >>>> .../BaseCryptLibMbedTls/SysCall/CrtWrapper.c | 58 + > > > >>>> .../SysCall/RuntimeMemAllocation.c | 462 ++ > > > >>>> .../SysCall/TimerWrapper.c | 198 + > > > >>>> .../BaseCryptLibMbedTls/TestBaseCryptLib.inf | 78 + > > > >>>> CryptoPkg/Library/MbedTlsLib/CrtWrapper.c | 96 + > > > >>>> CryptoPkg/Library/MbedTlsLib/EcSm2Null.c | 495 +++ > > > >>>> .../Include/mbedtls/mbedtls_config.h | 3823 > > > +++++++++++++++++ > > > >>>> CryptoPkg/Library/MbedTlsLib/MbedTlsLib.inf | 173 + > > > >>>> .../Library/MbedTlsLib/MbedTlsLibFull.inf | 177 + > > > >>>> CryptoPkg/Library/MbedTlsLib/mbedtls | 1 + > > > >>>> 66 files changed, 14683 insertions(+), 3 deletions(-) > > > >>>> create mode 100644 CryptoPkg/CryptoPkgMbedTls.dsc > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/BaseCryptLib.inf > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Bn/CryptBnNull.c > > > >>>> create mode 100644 > > > >>> > CryptoPkg/Library/BaseCryptLibMbedTls/Cipher/CryptAeadAesGcmNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Cipher/CryptAesNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptMd5.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptMd5Null.c > > > >>>> create mode 100644 > > > >>> > CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptParallelHashNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptSha1.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptSha1Null.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptSha256.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptSha256Null.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptSha512.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptSha512Null.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hash/CryptSm3Null.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hmac/CryptHmac.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Hmac/CryptHmacNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/InternalCryptLib.h > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Kdf/CryptHkdf.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Kdf/CryptHkdfNull.c > > > >>>> create mode 100644 > > > >> CryptoPkg/Library/BaseCryptLibMbedTls/PeiCryptLib.inf > > > >>>> create mode 100644 > > > >> CryptoPkg/Library/BaseCryptLibMbedTls/PeiCryptLib.uni > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pem/CryptPemNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptAuthenticodeNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptDhNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptEcNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptPkcs1OaepNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptPkcs5Pbkdf2Null.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptPkcs7Internal.h > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptPkcs7SignNull.c > > > >>>> create mode 100644 > > > >>> > CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptPkcs7VerifyEkuNull.c > > > >>>> create mode 100644 > > > >>> > > CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptPkcs7VerifyEkuRuntime.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptPkcs7VerifyNull.c > > > >>>> create mode 100644 > > > >>> > CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptPkcs7VerifyRuntime.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptRsaBasic.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptRsaBasicNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptRsaExt.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptRsaExtNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptRsaPss.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptRsaPssNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptRsaPssSign.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptRsaPssSignNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptTsNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Pk/CryptX509Null.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/Rand/CryptRandNull.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/RuntimeCryptLib.inf > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/RuntimeCryptLib.uni > > > >>>> create mode 100644 > > > >> CryptoPkg/Library/BaseCryptLibMbedTls/SecCryptLib.inf > > > >>>> create mode 100644 > > > >> CryptoPkg/Library/BaseCryptLibMbedTls/SecCryptLib.uni > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/SmmCryptLib.inf > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/SmmCryptLib.uni > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/SysCall/ConstantTimeClock.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/SysCall/CrtWrapper.c > > > >>>> create mode 100644 > > > >>> > CryptoPkg/Library/BaseCryptLibMbedTls/SysCall/RuntimeMemAllocation.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/SysCall/TimerWrapper.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/BaseCryptLibMbedTls/TestBaseCryptLib.inf > > > >>>> create mode 100644 CryptoPkg/Library/MbedTlsLib/CrtWrapper.c > > > >>>> create mode 100644 CryptoPkg/Library/MbedTlsLib/EcSm2Null.c > > > >>>> create mode 100644 > > > >>> CryptoPkg/Library/MbedTlsLib/Include/mbedtls/mbedtls_config.h > > > >>>> create mode 100644 > CryptoPkg/Library/MbedTlsLib/MbedTlsLib.inf > > > >>>> create mode 100644 > CryptoPkg/Library/MbedTlsLib/MbedTlsLibFull.inf > > > >>>> create mode 160000 CryptoPkg/Library/MbedTlsLib/mbedtls > > > >>>> > > > >>> > > > >>> > > > >>> > > > >>> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108201): https://edk2.groups.io/g/devel/message/108201 Mute This Topic: https://groups.io/mt/101048094/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
