Jiewen, In my opinion it's NOT a provide protocol, although it's placed in the private include folder.
The intention of this protocol, the crypto DXE driver who produces it, and the set of PEI/Runtime/SMM BaseCryptoLib instances who consume it, is to support the modulization update of crypto service code. The library instance will be static linked to other consumers out of CryptoPkg, thus a change of the protocol interface will require the library to be updated simultaneously, which breaks the original intention - modulization update - of this protocol. I'm not saying we can't change a protocol definition, but we need to be clear about the impact. It's not described in the patch and I think the author may also not aware of that. If it's well described and everyone is OK with that, the protocol can be changed, even a public one. Best Regards Siyuan > -----Original Message----- > From: Yao, Jiewen <jiewen....@intel.com> > Sent: 2020年3月27日 13:51 > To: Fu, Siyuan <siyuan...@intel.com>; devel@edk2.groups.io; Gao, Zhichao > <zhichao....@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX <xiaoyux...@intel.com>; > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, Jiaxin > <jiaxin...@intel.com> > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate function > > Siyuan > If you are just talking *private interface*, it is OK. > > My concern is raised, when you say: we cannot change a private protocol. > That means, we have to keep the ugly interface forever. :-( > > I am feeling there is some wrong fundamentally. > My believe is: > If it is private, we can change. > If we cannot change, it is not private. > > Thank you > Yao Jiewen > > > -----Original Message----- > > From: Fu, Siyuan <siyuan...@intel.com> > > Sent: Friday, March 27, 2020 1:43 PM > > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; Gao, > Zhichao > > <zhichao....@intel.com> > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > <xiaoyux...@intel.com>; > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, Jiaxin > > <jiaxin...@intel.com> > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate > function > > > > Jiewen, > > > > I agree "abstract action not algorithm" is a good design principle, but I'm > > not > > sure > > If there is any plan to move this protocol to the public include so far. > > For this patch set, my feeling is it should at least do not modify the > > existing > > protocol definition, so the modulization update capability won't be broken. > > I'm also welcome to see if the protocol can be enhanced as you mentioned > > below. > > > > Best Regards > > Siyuan > > > > > -----Original Message----- > > > From: Yao, Jiewen <jiewen....@intel.com> > > > Sent: 2020年3月27日 12:59 > > > To: Fu, Siyuan <siyuan...@intel.com>; devel@edk2.groups.io; Gao, Zhichao > > > <zhichao....@intel.com> > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > <xiaoyux...@intel.com>; > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, Jiaxin > > > <jiaxin...@intel.com> > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate > > function > > > > > > Thanks Siyun. > > > I think probably we need discuss this more. > > > > > > 1) About private v.s. public. > > > > > > The benefit for private include is to isolate external interface and > > > internal > > > interface. > > > A package can keep updating its private interface without impact any other > > > packages. > > > However, in this case, a private interface update will bring binary > compatibility > > > issue with other package. > > > I am not sure it is acceptable or not. > > > > > > Mike > > > Do you have any comment? Is that the design goal of private interface - > > > just > > > keep source code compatibility, but not binary compatiblity? > > > > > > 2) About the protocol itself. > > > > > > One concern I have is that we *hardcode* the algorithm in protocol. > > > > > > We keeps adding new algorithm and removing old one. That means this > > protocol > > > interface is unstable. > > > > > > Today, we have defined SHA2 set, and deprecating SHA1 and older one. > > > Tomorrow we may need add SHA3 set. > > > Today, we have RSAPKCS1_15. Tomorrow we will have RSAPSS. > > > Some other new set of algorithms might be added later, such as AEAD, > GMAC. > > > > > > For a protocol definition, I think we need *abstract the action*, but not > > > *algorithm*. > > > One good example is the UEFI HASH2 Protocol. > > > > > > https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/Has > > > h2.h > > > It just tells you do the hash. You may add new algorithm GUID. > > > > > > Another good example is inside of openssl. Now it is using EVP style > > > cipher > > algo. > > > For example, > > > https://www.openssl.org/docs/man1.1.1/man3/EVP_EncryptInit_ex.html > > > https://www.openssl.org/docs/man1.1.0/man3/EVP_CIPHER_CTX_ctrl.html > > > The cipher itself is input as parameter. > > > > > > The benefit is that, if we want to deprecate an algorithm, the interface > > > can > be > > > unchanged. > > > Just the internal implementation can be changed. > > > The current PCD mechanism can still be applied to internal implementation. > > > > > > Can we get a chance to revisit/redesign the protocol API, when we move to > > > public include? > > > > > > Thank you > > > Yao Jiewen > > > > > > > -----Original Message----- > > > > From: Fu, Siyuan <siyuan...@intel.com> > > > > Sent: Friday, March 27, 2020 11:07 AM > > > > To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io; Gao, > > > Zhichao > > > > <zhichao....@intel.com> > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > > <xiaoyux...@intel.com>; > > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, Jiaxin > > > > <jiaxin...@intel.com> > > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate > > > function > > > > > > > > Hi, Jiewen > > > > > > > > Although the protocol is private, a corresponding BaseCryptoLib > > > > instance is > > > > not private, like PeiCryptLib.inf, RuntimeCryptLib, etc. These library > instances > > > > will be static linked to the consumer driver, for example an iSCSI > > > > network > > > driver. > > > > So actually it's not a "private" change inside CryptoPkg. > > > > > > > > The goal to provide a driver version of crypto service is to support > > > modulization > > > > FW update, which means the crypto driver may NOT be updated together > > with > > > > its consumer. A platform may choose to update the crypto service driver > > > > to > a > > > > new version with this patch, then all the BaseCryptoLib consumers will > > > > be > > > > messed. > > > > > > > > Best Regards > > > > Siyuan > > > > > > > > > -----Original Message----- > > > > > From: Yao, Jiewen <jiewen....@intel.com> > > > > > Sent: 2020年3月27日 10:58 > > > > > To: devel@edk2.groups.io; Fu, Siyuan <siyuan...@intel.com>; Gao, > > Zhichao > > > > > <zhichao....@intel.com> > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > > > <xiaoyux...@intel.com>; > > > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, Jiaxin > > > > > <jiaxin...@intel.com> > > > > > Subject: RE: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate > > > > function > > > > > > > > > > EDKII_CRYPTO_PROTOCOL is *private*. > > > > > > > > > > > > > > > https://github.com/tianocore/edk2/blob/master/CryptoPkg/Private/Protocol/C > > > > > rypto.h > > > > > > > > > > Why we cannot change? > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > > Siyuan, > > > > Fu > > > > > > Sent: Friday, March 27, 2020 10:47 AM > > > > > > To: Gao, Zhichao <zhichao....@intel.com>; devel@edk2.groups.io > > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > > > > <xiaoyux...@intel.com>; > > > > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, Jiaxin > > > > > > <jiaxin...@intel.com> > > > > > > Subject: Re: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the > > > > > > deprecate > > > > > function > > > > > > > > > > > > Hi, Zhichao > > > > > > > > > > > > We should never move/delete a member field of a previous defined > > > protocol > > > > > > Interface. Instead, these protocol APIs shall be kept and return an > > > > > > error > > > code > > > > > > If the function is retired. Otherwise the consumer driver may call > > > > > > into > an > > > > > > Incorrect function if it's build with different codebase/PCD > > > > > > settings with > > > the > > > > > > Crypto PEI/DXE/SMM driver. > > > > > > This comment applies to all the EDKII_CRYPTO_PROTOCOL related > > changes > > > > in > > > > > > your patch set. > > > > > > > > > > > > Best Regards > > > > > > Siyuan > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Gao, Zhichao <zhichao....@intel.com> > > > > > > > Sent: 2020年3月27日 9:56 > > > > > > > To: devel@edk2.groups.io > > > > > > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX > > > > > > <xiaoyux...@intel.com>; > > > > > > > Maciej Rabeda <maciej.rab...@linux.intel.com>; Wu, Jiaxin > > > > > > > <jiaxin...@intel.com>; Fu, Siyuan <siyuan...@intel.com> > > > > > > > Subject: [PATCH 0/8] CryptoPkg: Retire the deprecate function > > > > > > > > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1682 > > > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1898 > > > > > > > > > > > > > > MD4, AR4, Tdes, Aes Ecb mode, MD5 and SHA1 is not secure any > longer. > > > > > > > They are all deprecated. Edk2 would not support them any longer. > > > > > > > So remove them. > > > > > > > But uefi spec want to keep MD5 and SHA1 for backwards > compatibility. > > > > > > > So add two pcds to control the MD5 and SHA1 enablement. Set the > > pcds > > > > > > > default value to false to indicate they are deprecated. > > > > > > > > > > > > > > NetWorkPkg's iSCSI driver would consume the MD5 function, so > change > > > > > > > the md5 pcd to TURE when iSCSI is enabled. > > > > > > > > > > > > > > Cc: Jian J Wang <jian.j.w...@intel.com> > > > > > > > Cc: Xiaoyu Lu <xiaoyux...@intel.com> > > > > > > > Cc: Maciej Rabeda <maciej.rab...@linux.intel.com> > > > > > > > Cc: Jiaxin Wu <jiaxin...@intel.com> > > > > > > > Cc: Siyuan Fu <siyuan...@intel.com> > > > > > > > Signed-off-by: Zhichao Gao <zhichao....@intel.com> > > > > > > > > > > > > > > Zhichao Gao (8): > > > > > > > CryptoPkg/BaseCrpytLib: Retire MD4 algorithm > > > > > > > CryptoPkg/BaseCryptLib: Retire ARC4 algorithm > > > > > > > CryptoPkg/BaseCryptLib: Retire the Tdes algorithm > > > > > > > CryptoPkg/BaseCryptLib: Retire Aes Ecb mode algorithm > > > > > > > CryptoPkg/dec: Add pcds to avoid building the deprecated > > > > > > > function > > > > > > > NetWorkPkg/Pcd.inc: Enable the MD5 for iSCSI > > > > > > > Crypto/BaseCryptLib: Using pcd to control MD5 enablement > > > > > > > CryptoPkg/BaseCryptLib: Use Pcd to control the SHA1 enablement > > > > > > > > > > > > > > CryptoPkg/CryptoPkg.dec | 11 + > > > > > > > CryptoPkg/CryptoPkg.uni | 11 + > > > > > > > CryptoPkg/Driver/Crypto.c | 634 > > > > > > > +----------------- > > > > > > > CryptoPkg/Include/Library/BaseCryptLib.h | 548 > > > > > > > --------------- > > > > > > > .../Library/BaseCryptLib/BaseCryptLib.inf | 9 +- > > > > > > > .../Library/BaseCryptLib/Cipher/CryptAes.c | 114 ---- > > > > > > > .../BaseCryptLib/Cipher/CryptAesNull.c | 52 -- > > > > > > > .../Library/BaseCryptLib/Cipher/CryptArc4.c | 205 ------ > > > > > > > .../BaseCryptLib/Cipher/CryptArc4Null.c | 124 ---- > > > > > > > .../Library/BaseCryptLib/Cipher/CryptTdes.c | 364 ---------- > > > > > > > .../BaseCryptLib/Cipher/CryptTdesNull.c | 160 ----- > > > > > > > .../Library/BaseCryptLib/Hash/CryptMd4.c | 223 ------ > > > > > > > .../Library/BaseCryptLib/Hash/CryptMd4Null.c | 143 ---- > > > > > > > .../Library/BaseCryptLib/Hash/CryptMd5.c | 5 +- > > > > > > > .../Library/BaseCryptLib/Hmac/CryptHmacMd5.c | 3 + > > > > > > > .../BaseCryptLib/Hmac/CryptHmacMd5Null.c | 3 + > > > > > > > .../Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 3 + > > > > > > > .../BaseCryptLib/Hmac/CryptHmacSha1Null.c | 3 + > > > > > > > .../Library/BaseCryptLib/PeiCryptLib.inf | 13 +- > > > > > > > .../BaseCryptLib/Pk/CryptPkcs5Pbkdf2.c | 3 + > > > > > > > .../Library/BaseCryptLib/Pk/CryptRsaBasic.c | 5 + > > > > > > > .../Library/BaseCryptLib/Pk/CryptRsaExt.c | 5 + > > > > > > > .../Library/BaseCryptLib/RuntimeCryptLib.inf | 13 +- > > > > > > > .../Library/BaseCryptLib/SmmCryptLib.inf | 13 +- > > > > > > > .../BaseCryptLibNull/BaseCryptLibNull.inf | 3 - > > > > > > > .../BaseCryptLibNull/Cipher/CryptAesNull.c | 54 +- > > > > > > > .../BaseCryptLibNull/Cipher/CryptArc4Null.c | 124 ---- > > > > > > > .../BaseCryptLibNull/Cipher/CryptTdesNull.c | 160 ----- > > > > > > > .../BaseCryptLibNull/Hash/CryptMd4Null.c | 143 ---- > > > > > > > .../BaseCryptLibNull/Hash/CryptMd5Null.c | 3 + > > > > > > > .../BaseCryptLibNull/Hmac/CryptHmacMd5Null.c | 3 + > > > > > > > .../BaseCryptLibNull/Hmac/CryptHmacSha1Null.c | 4 +- > > > > > > > .../BaseCryptLibOnProtocolPpi/CryptLib.c | 604 > > > > > > > +---------------- > > > > > > > .../Library/BaseHashApiLib/BaseHashApiLib.c | 12 + > > > > > > > .../Library/BaseHashApiLib/BaseHashApiLib.inf | 1 + > > > > > > > CryptoPkg/Private/Protocol/Crypto.h | 583 > > > > > > > +--------------- > > > > > > > NetworkPkg/NetworkPcds.dsc.inc | 5 +- > > > > > > > 37 files changed, 145 insertions(+), 4221 deletions(-) > > > > > > > delete mode 100644 > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc4.c > > > > > > > delete mode 100644 > > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptArc4Null.c > > > > > > > delete mode 100644 > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdes.c > > > > > > > delete mode 100644 > > > > > CryptoPkg/Library/BaseCryptLib/Cipher/CryptTdesNull.c > > > > > > > delete mode 100644 > > CryptoPkg/Library/BaseCryptLib/Hash/CryptMd4.c > > > > > > > delete mode 100644 > > > > CryptoPkg/Library/BaseCryptLib/Hash/CryptMd4Null.c > > > > > > > delete mode 100644 > > > > > > > CryptoPkg/Library/BaseCryptLibNull/Cipher/CryptArc4Null.c > > > > > > > delete mode 100644 > > > > > > > CryptoPkg/Library/BaseCryptLibNull/Cipher/CryptTdesNull.c > > > > > > > delete mode 100644 > > > > > > CryptoPkg/Library/BaseCryptLibNull/Hash/CryptMd4Null.c > > > > > > > > > > > > > > -- > > > > > > > 2.21.0.windows.1 > > > > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56474): https://edk2.groups.io/g/devel/message/56474 Mute This Topic: https://groups.io/mt/72579461/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
