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

[Laszlo Ersek <lersek@redhat.com>]

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?

Thanks!
Laszlo

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