[edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro
Abner Chang
abner.chang at hpe.com
Wed Nov 18 03:14:06 UTC 2020
Hi Laszlo,
> -----Original Message-----
> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, November 18, 2020 1:09 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
>
> 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.
I got your point now, NETWORK_HTTP_BOOT_ENABLE default set to FALSE makes sense.
>
> 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
Yes.
> >> TRUE.
> >>
> >>> + DEFINE NETWORK_HTTP_ENABLE = TRUE
> >>
> >> (4) See (2), this should be FALSE.
Yes
> >>
> >>> +!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.
Yes, but I will break down this long condition check into two blocks for the readability.
!if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
!if (($(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 or NETWORK_HTTP_ENABLE is set to TRUE!"
!endif
!endif
> >>
> >> 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.
Ah, your point is that’s not reasonable to have both NETWORK_TLS_ENABLE and NETWORK_ALLOW_HTTP_CONNECTIONS set to FALSE on edk2. But this seems to me this scenario falls into the change for (5),
!if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
!if (($(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 or NETWORK_HTTP_ENABLE is set to TRUE!"
!endif
!endif
Or you would like to have the different error messages?
My point was the macro would be looked as below after this patch,
!if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
NetworkPkg/DnsDxe/DnsDxe.inf
NetworkPkg/HttpDxe/HttpDxe.inf
NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
!endif
There could be the use case (with NETWORK_HTTP_BOOT_ENABLE = FALSE) that the implementation just requires HttpUtilitiesDxe to manipulate the HTTP headers but not really transferring HTTP request/response through HTTP protocol. HTTP payloads may transferred over the in-band transport.
Above condition check breaks this use case. This also reminds me that the change for (5) also breaks this use case.
How about we just leave it unchanged for (5), only handle NETWORK_HTTP_BOOT_ENABLE case. Or, we create another macro for NETWORK_HTTP_UTILITY but it seems to me too much.
However, I would like to have another macro for NetworkPkg/DnsDxe/DnsDxe.inf because not all of HTTP connections requires DNS.
How do you think?
Abner
>
> Laszlo
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67668): https://edk2.groups.io/g/devel/message/67668
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]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list