The breaking change would be in the future to move/make these structural changes (package /repo)  in the future after checking in this patch. Thanks Sean On 8/31/2023 11:45 AM, Michael D Kinney wrote: > > How is the current patch set a breaking change? > > Mike > > *From:* Sean Brogan > *Sent:* Thursday, August 31, 2023 10:52 AM > *To:* devel@edk2.groups.io; Kinney, Michael D > ; Yao, Jiewen ; Leif > Lindholm ; Hou, Wenxing > *Cc:* afish@apple.com > *Subject:* Re: [edk2-devel] [edk2/add_mbedtls PATCH 0/9] *** Add > HMAC/HKDF/RSA/HASH features based on Mbedtls *** > > replying to the whole chain. > > I am not encouraging importing the source directly but still trying to > isolate the "wrapper code" and the external mbedtls submodule management. > > I am advocating that the underlying crypto implementation is 100% > "hidden" from public include/dependency and the rest of the edk2 > tree.  I am advocating that crypto "releases" are in essence > independent of Edk2 stable tags (obviously a stable tag would still > have reference to version tested at that time) because crypto needs to > be updated more quickly and regularly and should have very minimal > breaking dependencies. > > Regarding Jiewen's options for layout my proposal would be more like > option 5.  :) > > Tianocore/Edk2: CryptoPkg > > * Header files for the crypto api of edk2(protocol, ppi, pcd, > library definitions). > * Modules that are underlying crypto library independent. > * Null libraries that support compile test of edk2 CI > > Tianocore/MbedTlsRepo: MbedTlsCryptoPkg > > * No public header files for consumption outside package. > * Wrapper code and modules to support edk2 crypto using mbedtls library. > * submodule tracking info for mbedtls upstream project > * tests > * release notes > * Security vulnerability management for mbedtls based work > > Tianocore/OpenSslRepo: OpenSslCryptoPkg > > * No public header files for consumption outside package. > * Wrapper code and modules to support edk2 crypto using openssl library. > * submodule tracking info for openssl upstream project > * tests > * release notes > * Security vulnerability management for OpenSSL based work > > I hope that helps explain. > > Regarding checking in and then moving later...well i am skeptical.  > This is a breaking change and once dependencies are formed, edk2 has > historically had challenges in making these kinds of changes. > > Thanks for consideration. > > Sean > > On 8/31/2023 10:24 AM, Michael D Kinney wrote: > > 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 > > Sent: Thursday, August 31, 2023 9:07 AM > > To: Kinney, Michael D ; Leif Lindholm > > ;devel@edk2.groups.io;spbrogan@outlook.com; > > Hou, Wenxing > > Cc:afish@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 > > Sent: Thursday, August 31, 2023 11:45 PM > > To: Leif Lindholm ; Yao, Jiewen > > ;devel@edk2.groups.io;spbrogan@outlook.com; > > Hou, > > Wenxing > > Cc:afish@apple.com; Kinney, Michael D > > 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 > > Sent: Thursday, August 31, 2023 4:15 AM > > To: Yao, Jiewen ;devel@edk2.groups.io; > > spbrogan@outlook.com; Hou, Wenxing > > Cc:afish@apple.com; Kinney, Michael D > > 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;spbrogan@outlook.com; Hou, Wenxing > > > > Cc:afish@apple.com;quic_llindhol@quicinc.com; Kinney, Michael D > > > > 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 On Behalf Of > > Sean > > Sent: Thursday, August 31, 2023 2:57 AM > > To:devel@edk2.groups.io; Hou, Wenxing > > 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 (#108207): https://edk2.groups.io/g/devel/message/108207 Mute This Topic: https://groups.io/mt/101048094/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-