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

[Gary Lin <glin@suse.com>]

On Wed, Jan 18, 2017 at 09:30:52AM +0100, Laszlo Ersek wrote:
> On 01/18/17 03:16, Wu, Jiaxin wrote:
> >> 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?
> 
> I agree that for a security-oriented approach, for a production
> firmware, both the DEC default *and* the separate ALLOW_HTTP_CONNECTIONS
> buid flag make sense.
> 
> For the default -D HTTP_BOOT_ENABLE build of upstream OVMF however, I
> think ease of use is more important. In a home or company or team
> intranet setting, booting virtual machines from plain HTTP is
> acceptable, I think; forcing users to set up HTTPS on the server side,
> and mess with keys, would be an inconvenience, in my opionion.
> 
> I guess we could introduce ALLOW_HTTP_CONNECTIONS with a TRUE default,
> but in general I try to minimize the number of different build flags
> (same way as MdeModulePkg seeks to minimize new PCDs); I think they
> quickly become confusing.
> 
> Serious users (like distros shipping OVMF) can flip the PCD in the DSC
> files anyway.
> 
> So, I prefer (2). Jordan, Gary, what do you guys think?
> 
(2) sounds reasonable to me. Maybe we can also explain the PCD in the
comment or README to help the user to make the decision.

Thanks,

Gary Lin

> Thanks!
> Laszlo
> 
> > 
> >> (Note that in "OvmfPkgIa32X64.dsc", the setting should likely go under
> >> [PcdsFixedAtBuild.X64].)
> >>
> >> Thanks!
> >> Laszlo
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>