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