From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.nue.novell.com (smtp.nue.novell.com [195.135.221.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 210DB81E10 for ; Wed, 18 Jan 2017 01:28:19 -0800 (PST) Received: from nwb-ext-pat.microfocus.com ([10.120.13.103]) by smtp.nue.novell.com with ESMTP (TLS encrypted); Wed, 18 Jan 2017 10:28:18 +0100 Received: from GaryWorkstation (nwb-a10-snat.microfocus.com [10.120.13.202]) by nwb-ext-pat.microfocus.com with ESMTP (TLS encrypted); Wed, 18 Jan 2017 09:27:49 +0000 Date: Wed, 18 Jan 2017 17:27:42 +0800 From: Gary Lin To: Laszlo Ersek Cc: "Wu, Jiaxin" , "edk2-devel@ml01.01.org" , "Ni, Ruiyu" , "Ye, Ting" , "Justen, Jordan L" , "Kinney, Michael D" , "Fu, Siyuan" Message-ID: <20170118092742.n3vp6moruovgtzkl@GaryWorkstation> References: <1484623992-52988-1-git-send-email-jiaxin.wu@intel.com> <1484623992-52988-3-git-send-email-jiaxin.wu@intel.com> <885aab27-f660-768b-59da-4d3e33f099ec@redhat.com> <895558F6EA4E3B41AC93A00D163B7274162948A8@SHSMSX103.ccr.corp.intel.com> <9cacd428-ac00-bab8-2329-6ea78c389dd2@redhat.com> MIME-Version: 1.0 In-Reply-To: <9cacd428-ac00-bab8-2329-6ea78c389dd2@redhat.com> User-Agent: Mutt/1.6.2 (2016-07-01) Subject: Re: [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Jan 2017 09:28:21 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > >>> Cc: Fu Siyuan > >>> Cc: Ruiyu Ni > >>> Cc: Laszlo Ersek > >>> Cc: Kinney Michael D > >>> Contributed-under: TianoCore Contribution Agreement 1.0 > >>> Signed-off-by: Wu Jiaxin > >>> --- > >>> 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.
> >>> +# Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> >>> # Copyright (c) 2015, Hewlett-Packard Development Company, L.P.
> >>> # (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> >>> # > >>> # 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 >