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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

> Subject: Re: [edk2] [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP
> connections
> 
> 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.PcdResetOnMemoryTypeInformationChan
> ge|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?
> 

Laszlo,

For my perspective, I think it also make sense since the platform owner make the decision to disable the HTTP connections. In such a case, if TLS is not enabled, HTTP stack should be useless since HTTP connections have been disabled.




> 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?
> 

The default value of PcdAllowHttpConnections is crucial to ensure the real platform security by default. So, we set the default value to FALSE.

In order to facilitate control (Just like Nt32), platform owner can define the flag to make the decision whether allow the HTTP connections.

For Nt32 simulation platform, the default value of flag ALLOW_HTTP_CONNECTIONS is TRUE. For OVMF, we can also define the flag with the TRUE value, which would achieve your purpose that HTTP working with just -D HTTP_BOOT_ENABLE and both HTTP and HTTPS working with -D HTTP_BOOT_ENABLE -D TLS_ENABLE.



> 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?
> 

Laszlo,

As I talked above and according your requirement, we have the below update choice:

1) The flag definition (ALLOW_HTTP_CONNECTIONS) with TRUE value to allow the HTTP connections (the same to NT32).
    
    DEFINE ALLOW_HTTP_CONNECTIONS = TRUE
    !if $(ALLOW_HTTP_CONNECTIONS) == TRUE
       gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
    !endif

2) Sets PcdAllowHttpConnections to TRUE whenever HTTP_BOOT_ENABLE is TRUE
    !if $( HTTP_BOOT_ENABLE) == TRUE
       gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
    !endif

For 1), Flexible control! 
For 2), we have no way to stop the HTTP connections while HTTPS is allowed. That means no HTTP connections control switch.

I still prefer 1), but that's depends on you since you are the OVMF platform owner:).

What's your opinion?

Thanks,
Jiaxin




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