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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Library is static interface. Protocol is dynamic interface. That is key difference.

My understanding for a *private protocol* is that: one module in the package to produce. The other module in the same packet to consume. Both are at runtime. That brings zero impact to other module.
It is not the case here. I hope we can clearly document what private protocol mean.
I feel it is a public one now, instead of private, because it brings runtime impact - even worse than build time impact.

A developer can fix the build time break easily, but runtime break requires more debugging effort.

Here is my thought:
1) We need update this protocol to remove the deprecated algorithm. I do not see the value to keep them.
2) We need clarify the position of this protocol. What we should do, if we need add a new algo and deprecate an old one.
3) If required, we need redesign this protocol. I have strong feeling on that.



> -----Original Message-----
> From: Fu, Siyuan <siyuan.fu@intel.com>
> Sent: Friday, March 27, 2020 2:04 PM
> 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
> 
> 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.yao@intel.com>
> > Sent: 2020年3月27日 13:51
> > 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
> >
> > 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.fu@intel.com>
> > > Sent: Friday, March 27, 2020 1:43 PM
> > > 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
> > >
> > > 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
> > > > > > >
> > > > > > >
> > > > > > >