[edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro

Tue Nov 17 17:08:38 UTC 2020

Hi Abner,

On 11/16/20 03:32, Chang, Abner (HPS SW/FW Technologist) wrote:
> 
> 
>> -----Original Message-----
>> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, November 12, 2020 5:22 AM
>> To: Chang, Abner (HPS SW/FW Technologist) <abner.chang at hpe.com>;
>> devel at edk2.groups.io
>> Cc: Maciej Rabeda <maciej.rabeda at linux.intel.com>; Jiaxin Wu
>> <jiaxin.wu at intel.com>; Siyuan Fu <siyuan.fu at intel.com>; Wang, Nickle (HPS
>> SW) <nickle.wang at hpe.com>; O'Hanley, Peter (EXL)
>> <peter.ohanley at hpe.com>
>> Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1]
>> NetworkPkg: Add NETWORK_HTTP_ENABLE macro
>>
>> On 11/11/20 14:19, Abner Chang wrote:

[...]

>>> diff --git a/NetworkPkg/NetworkDefines.dsc.inc
>>> b/NetworkPkg/NetworkDefines.dsc.inc
>>> index a442d1b157..6f274582a8 100644
>>> --- a/NetworkPkg/NetworkDefines.dsc.inc
>>> +++ b/NetworkPkg/NetworkDefines.dsc.inc
>>> @@ -15,12 +15,14 @@
>>>  #   DEFINE NETWORK_IP4_ENABLE             = TRUE
>>>  #   DEFINE NETWORK_IP6_ENABLE             = TRUE
>>>  #   DEFINE NETWORK_TLS_ENABLE             = TRUE
>>> +#   DEFINE NETWORK_HTTP_ENABLE            = TRUE
>>>  #   DEFINE NETWORK_HTTP_BOOT_ENABLE       = TRUE
>>
>> (2) I disagree; the default value for NETWORK_HTTP_ENABLE should be
>> FALSE.
>>
>> Existent platforms that consume "NetworkPkg/NetworkDefines.dsc.inc", or
>> the higher level "Network.dsc.inc", fall in one of the following categories:
>>
>> - They don't specify NETWORK_HTTP_BOOT_ENABLE at all. As a result, they
>> get the full HTTP stack.
>>
>> - They set NETWORK_HTTP_BOOT_ENABLE explicitly to TRUE. As a result,
>> they get the full HTTP stack.
>>
>> - They set NETWORK_HTTP_BOOT_ENABLE explicitly to FALSE. As a result,
>> they get *none* of the full HTTP stack. They don't get a *subset* of the
>> HTTP stack -- they get *none* of it.
>>
>> The last bullet explains why the NETWORK_HTTP_ENABLE default should be
>> FALSE.
> I don’t quite get the last scenario. If they set NETWORK_HTTP_BOOT_ENABLE to FALSE then NETWORK_HTTP_ENABLE is still TURE for other HTTP use cases.
> They can set NETWORK_HTTP_ENABLE to FALSE explicitly if they don’t even need HTTP.
> 

When we implement a new feature (or just a change) in core edk2, it's
best to keep platforms (especially out-of-tree platforms) completely
unaffected, *if* this is possible to do without major difficulties.

Consider a platform that currently contains, in its DSC file,

  DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE

and then an !include directive for "NetworkPkg/NetworkDefines.dsc.inc".

That particular platform permits its users to build it with the HTTP
Boot feature, but the default setting for that platform is to not
include HTTP Boot -- or in fact *ANY* of the HTTP infrastructure elements.

If your patch were applied as-is to core edk2, then such platforms would
suddenly start including *some* parts of the HTTP infrastructure by
default, without ever having asked for it.

> I think those network definitions were designed as default ON.

That default (from NetworkPkg) applies *unless* the platform sets a
different default. Consider "NetworkPkg/NetworkDefines.dsc.inc":

!ifndef NETWORK_HTTP_BOOT_ENABLE
  #
  # This flag is to enable or disable HTTP(S) boot feature.
  #
  DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE
!endif

The platform is welcome to set NETWORK_HTTP_BOOT_ENABLE to FALSE as the
platform default, before including "NetworkPkg/NetworkDefines.dsc.inc".
That doesn't mean the platform *never* wants to enable HTTP boot (users
may still override NETWORK_HTTP_BOOT_ENABLE to TRUE on the build command
line), it just means that the default for the platform is FALSE.

And your patch would break that, as it would sneak some unwanted
components into the default build of such platforms.

It's not good practice to say "they can still set the 'new flag' for
undoing the damage we're causing them". There should be *no damage* to
them in the first place (if that's possible to implement). The burden
should be on platforms that want to consume the new feature to
explicitly ask for the new feature.

> 
>>
>>
>> The new scenario should only be active if a platform explicitly sets *both*
>> NETWORK_HTTP_ENABLE=TRUE *and*
>> NETWORK_HTTP_BOOT_ENABLE=FALSE.
>>
>>
>>>  #   DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
>>>  #   DEFINE NETWORK_ISCSI_ENABLE           = TRUE
>>>  #   DEFINE NETWORK_VLAN_ENABLE            = TRUE
>>>  #
>>>  # Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>>> +# (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
>>>  #
>>>  #    SPDX-License-Identifier: BSD-2-Clause-Patent
>>>  #
>>> @@ -73,6 +75,13 @@
>>>    DEFINE NETWORK_TLS_ENABLE = TRUE
>>>  !endif
>>>
>>> +!ifndef NETWORK_HTTP_ENABLE
>>> +  #
>>> +  # This flag is to enable or disable HTTP(S) feature.
>>> +  #
>>
>> (3) The documentation here must explain that NETWORK_HTTP_ENABLE is
>> ignored (it has no effect whatsoever) if NETWORK_HTTP_BOOT_ENABLE is
>> TRUE.
>>
>>> +  DEFINE NETWORK_HTTP_ENABLE = TRUE
>>
>> (4) See (2), this should be FALSE.
>>
>>> +!endif
>>> +
>>>  !ifndef NETWORK_HTTP_BOOT_ENABLE
>>>    #
>>>    # This flag is to enable or disable HTTP(S) boot feature.
>>>
>>
>> (5) The following condition should be updated too:
>>
>>   !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) AND
>> ($(NETWORK_TLS_ENABLE) == FALSE) AND
>> ($(NETWORK_ALLOW_HTTP_CONNECTIONS) == FALSE)
>>     !error "Must enable TLS to support HTTPS, or allow unsecured HTTP
>> connection, if NETWORK_HTTP_BOOT_ENABLE is set to TRUE!"
>>   !endif
>>
>> That's because NETWORK_ALLOW_HTTP_CONNECTIONS controls
>> "PcdAllowHttpConnections", and this PCD is consumed by HttpDxe as well,
>> not just HttpBootDxe.
>>
>> Thus, the subcondition
>>
>>   ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE)
>>
>> should be replaced by
>>
>>   (($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR
>> ($(NETWORK_HTTP_ENABLE) == TRUE))
>>
>> because that condition describes whether HttpDxe will be included.
>>
>> Specifically, the following build config should be rejected:
>>
>>   NETWORK_HTTP_BOOT_ENABLE       = FALSE (manually set)
>>   NETWORK_HTTP_ENABLE            = TRUE  (manually set)
>>   NETWORK_TLS_ENABLE             = FALSE (manually set)
>>   NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE (default)
> What if the use case just requires HTTP Utility Protocol to produce and consume HTTP headers but not sending out through HTTP protocol, via in-band channel instead.  I don’t think we have to put the restrictions this one.

I don't understand, I'm sorry.

The above combination of flags means that:
- the user wants the HTTP base infrastructure, without HTTP Boot,
- *plus* they disable TLS,
- *plus* they forbid the HTTP base infrastructure for making (or even
accepting) plaintext HTTP connections.

The described scenario requires the HTTP base infrastructure to always
create (or enforce) encrypted (HTTPS) connections, *but* without relying
on TLS.

But that's a requirement that's impossible to satisfy: in edk2, the
*only* means for making or accepting HTTPS connections (regardless of
whether they are for Boot purposes or otherwise) is TLS.

So in this scenario, the build should abort at once.

Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67651): https://edk2.groups.io/g/devel/message/67651
Mute This Topic: https://groups.io/mt/78182924/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-