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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Jiewen,

The purpose of private includes is to keep modules/lib
in *other* packages from using interfaces that are the
package with the private interface does not want other
packages to use and does not want to have to coordinate
with other packages if that package owner decides to
make changes to those private interfaces. 

For modules/libs within package that do use private
includes, the package owner gets to decide how to 
maintain the interfaces in the private includes to
support those modules/libs.

For example, the CryptoPkg has modules that are
intended to be used as pre-built binaries, so the
CryptoPkg owner needs to make sure the maintenance
of the private includes supports both the source and
binary use cases.

The private Protocol/PPI interfaces in the CryptoPkg
were designed to support future expansion.  The first
API in the Protocol/PPI is GetVersion().  The version 
value returned can be used to have different layouts 
of fields in the Protocol/PPI.  In order to support
backwards compatibility, APIs are added to the end
of the Protocol/PPI structure as the version value 
is increased.  You will notice that there is an X509
service that was added further down than the initial
grouping.  This is just an example of how the CryptoPkg
is maintaining a private interface for binary use cases.
Other packages may choose alternate techniques.
 
Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Yao, Jiewen
> Sent: Thursday, March 26, 2020 9:59 PM
> 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/Incl
> ude/Protocol/Hash2.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_EncryptIni
> t_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/P
> rivate/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
> > > >
> > > >
> > > >
> 
> 
>