Re: [PATCH v2] OvmfPkg: Remove the flag control for the CryptoPkg libraries

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-01-16 12:33:20, Laszlo Ersek wrote:
> On 01/16/17 13:22, Jiaxin Wu wrote:
> > v2:
> > * Remove the flag for NetworkPkg/IScsiDxe
> > 
> > This patch is to remove the 'SECURE_BOOT_ENABLE' flag control for
> > the CryptoPkg librarie.
> > 
> > Not only the secure boot feature requires the CryptoPkg libraries
> > (e.g, OpensslLib, BaseCryptLib), but also ISCSI, IpSec and HTTPS/TLS
> > features. Those modules can be always included since no build performance
> > impacts if they are not consumed.
> > 
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Justen Jordan L <jordan.l.justen@intel.com>
> > Cc: Gary Lin <glin@suse.com>
> > Cc: Long Qin <qin.long@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> > ---
> >  OvmfPkg/OvmfPkgIa32.dsc    | 17 ++++++-----------
> >  OvmfPkg/OvmfPkgIa32X64.dsc | 17 ++++++-----------
> >  OvmfPkg/OvmfPkgX64.dsc     | 17 ++++++-----------
> >  3 files changed, 18 insertions(+), 33 deletions(-)
> 
> I disagree with this patch (assuming at least that I understand it
> correctly).
> 
> Namely,
> - unconditionally resolving OpensslLib in the DSC files, and
> - unconditionally consuming OpensslLib in modules that are
>   unconditionally included in the DSC files,
> 
> makes OpenSSL a hard requirement for building OVMF.
> 
> Given that OpenSSL is not distributed as part of the edk2 tree, and
> given that it's not even pulled in through an unmodified git submodule,
> this patch would prevent people, IIUC, from building OVMF without
> jumping through the hoops described in
> 
>   CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt
> 
> That's a bad thing, forcing people to download and patch OpenSSL even if
> they don't care about any of the dependent features. (It is perfectly
> possible to be uninterested in *all* of: Secure Boot, IpSec, HTTPS boot,
> and iSCSI, in a virtual machine.)
> 
> If OpenSSL were distributed as part of edk2, or if OpenSSL were
> presented as a plain (unmodified) git submodule in edk2, then I might agree.

I agree.

I'm not sure what the half-hearted support for OpenSSL in the EDK II
tree is about. Perhaps it is the license? (Isn't is always that when
it comes to OpenSSL?) If so, I wonder if other free software
alternatives have been considered.

There is also the build time and firmware space overhead of supporting
this. Since it is not a UEFI requirement, as Laszlo mentions it is
entirely possible to not care that it is missing from OVMF.

-Jordan

> 
> For now, perhaps we can introduce an OPENSSL_ENABLE build option.
> 
> - Features that require OpenSSL no matter what, such as
>   SECURE_BOOT_ENABLE, should auto-define OPENSSL_ENABLE.
> 
>   (I don't remember if the [Defines] section of the DSC file can set
>   macros conditionally, dependent on other macros, but I hope so.)
> 
> - Features that can utilize (but don't require) OpenSSL, such as
>   NETWORK_IP6_ENABLE and HTTP_BOOT_ENABLE, should provide conditional
>   DSC stanzas for both $(OPENSSL_ENABLE) == TRUE and == FALSE.
> 
> - The libraries and drivers that provide the crypto stuff (directly on
>   top of OpenSSL) should depend on OPENSSL_ENABLE.
> 
> In fact, looking at Gary's patch "OvmfPkg: Enable HTTPS for Ovmf" with
> TLS_ENABLE, it seems like we need another layer. HTTP_BOOT_ENABLE should
> not be customized for OPENSSL_ENABLE, but for TLS_ENABLE.
> 
> In summary:
> - SECURE_BOOT_ENABLE should auto-select OPENSSL_ENABLE.
> - TLS_ENABLE should auto-select OPENSSL_ENABLE.
> - NETWORK_IP6_ENABLE should be customized based on OPENSSL_ENABLE
>   (for the ISCSI driver).
> - HTTP_BOOT_ENABLE should be customized based on TLS_ENABLE.
> - OPENSSL_ENABLE should control the CryptoPkg modules that directly
>   wrap the OpenSSL functionality, for edk2.
> 
> As a result, the following build option combinations would be valid
> (listing some examples):
> 
> * -D SECURE_BOOT_ENABLE
> 
>   It would set OPENSSL_ENABLE. If OpenSSL is available, it would build
>   fine, otherwise it would break, as it should.
> 
> * -D NETWORK_IP6_ENABLE
> 
>   You get the IPv6 stack, but no secure ISCSI.
> 
> * -D NETWORK_IP6_ENABLE -D OPENSSL_ENABLE
> 
>   You get the IPv6 stack, with secure ISCSI. If OpenSSL is not
>   available, the build breaks, as it should.
> 
> * -D HTTP_BOOT_ENABLE
> 
>   You get HTTP boot, but not HTTPS boot.
> 
> * -D HTTP_BOOT_ENABLE -D OPENSSL_ENABLE <----- note that this is useless
> 
>   Same, no change.
> 
> * -D TLS_ENABLE
> 
>   Selects OPENSSL_ENABLE automatically. If OpenSSL is not available,
>   the build breaks. Otherwise, the TLS drivers are included in the fw
>   binary. They might not be used by any edk2 module, but some 3rd party
>   UEFI application (launched from the shell, eg.) could.
> 
> * -D HTTP_BOOT_ENABLE -D TLS_ENABLE
> 
>   HTTP and HTTPS boot becomes available. If OpenSSL is absent from the
>   tree, the build breaks.
> 
> * -D SECURE_BOOT_ENABLE -D HTTP_BOOT_ENABLE -D NETWORK_IP6_ENABLE
> 
>   You get Secure Boot, and secure ISCSI with IPv6, but not HTTPS
>   boot.
> 
> * -D SECURE_BOOT_ENABLE -D HTTP_BOOT_ENABLE -D TLS_ENABLE \
>   -D NETWORK_IP6_ENABLE
> 
>   You get everything.
> 
> My point is, if we touch these build flags, then we should go the whole
> way, and express their inter-dependencies precisely.
> 
> Thanks!
> Laszlo
> 
> > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> > index e97f7f0..6e53d9f 100644
> > --- a/OvmfPkg/OvmfPkgIa32.dsc
> > +++ b/OvmfPkg/OvmfPkgIa32.dsc
> > @@ -1,9 +1,9 @@
> >  ## @file
> >  #  EFI/Framework Open Virtual Machine Firmware (OVMF) platform
> >  #
> > -#  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> >  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> >  #
> >  #  This program and the accompanying materials
> >  #  are licensed and made available under the terms and conditions of the BSD License
> >  #  which accompanies this distribution. The full text of the license may be found at
> > @@ -139,14 +139,15 @@
> >  
> >    ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> >    LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> >    DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
> >  
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> > -  PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> >    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> >    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > +
> > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > +  PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> >    TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> >    AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> >  !if $(NETWORK_IP6_ENABLE) == TRUE
> >    TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
> >  !endif
> > @@ -164,13 +165,11 @@
> >    SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
> >    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> >    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
> >  
> >  [LibraryClasses.common]
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> >    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > -!endif
> >  
> >  [LibraryClasses.common.SEC]
> >    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
> >    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
> >  !ifdef $(DEBUG_ON_SERIAL_PORT)
> > @@ -256,13 +255,13 @@
> >    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> >  !else
> >    DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> >  !endif
> >    UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> > +
> >    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > -!endif
> > +
> >    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> >  
> >  [LibraryClasses.common.UEFI_DRIVER]
> >    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> >    TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> > @@ -698,16 +697,12 @@
> >    NetworkPkg/TcpDxe/TcpDxe.inf
> >    NetworkPkg/Udp6Dxe/Udp6Dxe.inf
> >    NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
> >    NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> >    NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> >    NetworkPkg/IScsiDxe/IScsiDxe.inf
> >  !else
> > -  MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
> > -!endif
> > -!else
> >    MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
> >    MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
> >    MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
> >  !endif
> >  !if $(HTTP_BOOT_ENABLE) == TRUE
> > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> > index 8e3e04c..15db2d5 100644
> > --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> > @@ -1,9 +1,9 @@
> >  ## @file
> >  #  EFI/Framework Open Virtual Machine Firmware (OVMF) platform
> >  #
> > -#  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> >  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> >  #
> >  #  This program and the accompanying materials
> >  #  are licensed and made available under the terms and conditions of the BSD License
> >  #  which accompanies this distribution. The full text of the license may be found at
> > @@ -144,14 +144,15 @@
> >  
> >    ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> >    LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> >    DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
> >  
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> > -  PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> >    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> >    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > +
> > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > +  PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> >    TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> >    AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> >  !if $(NETWORK_IP6_ENABLE) == TRUE
> >    TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
> >  !endif
> > @@ -169,13 +170,11 @@
> >    SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
> >    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> >    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
> >  
> >  [LibraryClasses.common]
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> >    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > -!endif
> >  
> >  [LibraryClasses.common.SEC]
> >    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
> >    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
> >  !ifdef $(DEBUG_ON_SERIAL_PORT)
> > @@ -261,13 +260,13 @@
> >    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> >  !else
> >    DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> >  !endif
> >    UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> > +
> >    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > -!endif
> > +
> >    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> >  
> >  [LibraryClasses.common.UEFI_DRIVER]
> >    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> >    TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> > @@ -707,16 +706,12 @@
> >    NetworkPkg/TcpDxe/TcpDxe.inf
> >    NetworkPkg/Udp6Dxe/Udp6Dxe.inf
> >    NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
> >    NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> >    NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> >    NetworkPkg/IScsiDxe/IScsiDxe.inf
> >  !else
> > -  MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
> > -!endif
> > -!else
> >    MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
> >    MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
> >    MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
> >  !endif
> >  !if $(HTTP_BOOT_ENABLE) == TRUE
> > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> > index 6ec3fe0..9c6bdc2 100644
> > --- a/OvmfPkg/OvmfPkgX64.dsc
> > +++ b/OvmfPkg/OvmfPkgX64.dsc
> > @@ -1,9 +1,9 @@
> >  ## @file
> >  #  EFI/Framework Open Virtual Machine Firmware (OVMF) platform
> >  #
> > -#  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> >  #  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> >  #
> >  #  This program and the accompanying materials
> >  #  are licensed and made available under the terms and conditions of the BSD License
> >  #  which accompanies this distribution. The full text of the license may be found at
> > @@ -144,14 +144,15 @@
> >  
> >    ResetSystemLib|OvmfPkg/Library/ResetSystemLib/ResetSystemLib.inf
> >    LocalApicLib|UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
> >    DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
> >  
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> > -  PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> >    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
> >    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> > +
> > +!if $(SECURE_BOOT_ENABLE) == TRUE
> > +  PlatformSecureLib|OvmfPkg/Library/PlatformSecureLib/PlatformSecureLib.inf
> >    TpmMeasurementLib|SecurityPkg/Library/DxeTpmMeasurementLib/DxeTpmMeasurementLib.inf
> >    AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> >  !if $(NETWORK_IP6_ENABLE) == TRUE
> >    TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
> >  !endif
> > @@ -169,13 +170,11 @@
> >    SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
> >    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
> >    XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
> >  
> >  [LibraryClasses.common]
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> >    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> > -!endif
> >  
> >  [LibraryClasses.common.SEC]
> >    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
> >    QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
> >  !ifdef $(DEBUG_ON_SERIAL_PORT)
> > @@ -261,13 +260,13 @@
> >    DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> >  !else
> >    DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
> >  !endif
> >    UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> > +
> >    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> > -!endif
> > +
> >    PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
> >  
> >  [LibraryClasses.common.UEFI_DRIVER]
> >    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> >    TimerLib|OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
> > @@ -705,16 +704,12 @@
> >    NetworkPkg/TcpDxe/TcpDxe.inf
> >    NetworkPkg/Udp6Dxe/Udp6Dxe.inf
> >    NetworkPkg/Dhcp6Dxe/Dhcp6Dxe.inf
> >    NetworkPkg/Mtftp6Dxe/Mtftp6Dxe.inf
> >    NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
> > -!if $(SECURE_BOOT_ENABLE) == TRUE
> >    NetworkPkg/IScsiDxe/IScsiDxe.inf
> >  !else
> > -  MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
> > -!endif
> > -!else
> >    MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
> >    MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
> >    MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
> >  !endif
> >  !if $(HTTP_BOOT_ENABLE) == TRUE
> > 
>