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 <[email protected]> > >>> Cc: Fu Siyuan <[email protected]> > >>> Cc: Ruiyu Ni <[email protected]> > >>> Cc: Laszlo Ersek <[email protected]> > >>> Cc: Kinney Michael D <[email protected]> > >>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>> Signed-off-by: Wu Jiaxin <[email protected]> > >>> --- > >>> 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 > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
