Re: [edk2-devel] [PATCH 0/8] CryptoPkg: Retire the deprecate function

["Siyuan, Fu" <siyuan.fu@intel.com>]

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.yao@intel.com>
> Sent: 2020年3月27日 12:59
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; Gao, Zhichao
> <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>;
> Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin
> <jiaxin.wu@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.fu@intel.com>
> > Sent: Friday, March 27, 2020 11:07 AM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Gao,
> Zhichao
> > <zhichao.gao@intel.com>
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> <xiaoyux.lu@intel.com>;
> > Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin
> > <jiaxin.wu@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.yao@intel.com>
> > > Sent: 2020年3月27日 10:58
> > > To: devel@edk2.groups.io; Fu, Siyuan <siyuan.fu@intel.com>; Gao, Zhichao
> > > <zhichao.gao@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > <xiaoyux.lu@intel.com>;
> > > Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin
> > > <jiaxin.wu@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.gao@intel.com>; devel@edk2.groups.io
> > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > > <xiaoyux.lu@intel.com>;
> > > > Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin
> > > > <jiaxin.wu@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.gao@intel.com>
> > > > > Sent: 2020年3月27日 9:56
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX
> > > > <xiaoyux.lu@intel.com>;
> > > > > Maciej Rabeda <maciej.rabeda@linux.intel.com>; Wu, Jiaxin
> > > > > <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@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.wang@intel.com>
> > > > > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > > > > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> > > > > Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> > > > > Cc: Siyuan Fu <siyuan.fu@intel.com>
> > > > > Signed-off-by: Zhichao Gao <zhichao.gao@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
> > > >
> > > >
> > > >