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

[Laszlo Ersek <lersek@redhat.com>]

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.

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
>