Re: [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections

[Laszlo Ersek <lersek@redhat.com>]

CC Jordan and Gary

On 01/17/17 04:33, Jiaxin Wu wrote:
> v2:
> * Rename the flag.
> 
> This flag is used to overwrite the PcdAllowHttpConnections
> value, then the platform can make a decision whether to allow
> HTTP connections or not.
> 
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Kinney Michael D <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  Nt32Pkg/Nt32Pkg.dsc | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Nt32Pkg/Nt32Pkg.dsc b/Nt32Pkg/Nt32Pkg.dsc
> index 134afb8..88b1ea9 100644
> --- a/Nt32Pkg/Nt32Pkg.dsc
> +++ b/Nt32Pkg/Nt32Pkg.dsc
> @@ -2,11 +2,11 @@
>  # EFI/Framework Emulation Platform with UEFI HII interface supported.
>  #
>  # The Emulation Platform can be used to debug individual modules, prior to creating
>  #    a real platform. This also provides an example for how an DSC is created.
>  #
> -# Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
>  # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.<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
> @@ -57,11 +57,21 @@
>    #
>    # Note: TLS feature highly depends on the OpenSSL building. To enable this 
>    #       feature, please follow the instructions found in the file "Patch-HOWTO.txt" 
>    #       located in CryptoPkg\Library\OpensslLib to enable the OpenSSL building first.
>    #
> -  DEFINE TLS_ENABLE      = FALSE
> +  DEFINE TLS_ENABLE = FALSE
> +  
> +  #
> +  # Indicates whether HTTP connections (i.e., unsecured) are permitted or not.
> +  # -D FLAG=VALUE
> +  #
> +  # Note: If ALLOW_HTTP_CONNECTIONS is TRUE, HTTP connections is allowed. Both 
> +  #       the "https://" and "http://" URI schemes are permitted. Otherwise, HTTP 
> +  #       connections is denied. Only the "https://" URI scheme is permitted.
> +  #
> +  DEFINE ALLOW_HTTP_CONNECTIONS = TRUE
>  
>  ################################################################################
>  #
>  # SKU Identification section - list of all SKU IDs supported by this
>  #                              Platform.
> @@ -252,10 +262,14 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>  !if $(SECURE_BOOT_ENABLE) == TRUE || $(TLS_ENABLE) == TRUE
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>  !endif
>  
> +!if $(ALLOW_HTTP_CONNECTIONS) == TRUE
> +  gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
> +!endif
> +
>  !ifndef $(USE_OLD_SHELL)
>    gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdShellFile|{ 0x83, 0xA5, 0x04, 0x7C, 0x3E, 0x9E, 0x1C, 0x4F, 0xAD, 0x65, 0xE0, 0x52, 0x68, 0xD0, 0xB4, 0xD1 }
>  !endif
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
> 

Does the following combination make sense?

  TLS_ENABLE=FALSE and ALLOW_HTTP_CONNECTIONS=FALSE

In this case, only the https:// scheme would be accepted, however the
TLS facility that underlies HTTPS is missing. I think this would render
the HTTP stack useless. Is that correct?

I'm asking mainly for OVMF's sake. (I have nothing against this patch in
Nt32Pkg.) Namely, in OvmfPkg, I would dislike the additional complexity
of an ALLOW_HTTP_CONNECTIONS build flag. Instead, I think we should set
PcdAllowHttpConnections to TRUE, whenever HTTP_BOOT_ENABLE is defined
(and we shouldn't override the DEC default otherwise).

This would result in HTTP working with just -D HTTP_BOOT_ENABLE, and
both HTTP and HTTPS working with -D HTTP_BOOT_ENABLE -D TLS_ENABLE. I
don't see any downsides to always permitting HTTP in OVMF.

Thoughts?

If everyone agrees, then Jiaxin, can you please append a third patch for
OvmfPkg, which sets PcdAllowHttpConnections to TRUE whenever
HTTP_BOOT_ENABLE is TRUE?

(Note that in "OvmfPkgIa32X64.dsc", the setting should likely go under
[PcdsFixedAtBuild.X64].)

Thanks!
Laszlo