From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web08.10902.1605632925305223824 for ; Tue, 17 Nov 2020 09:08:45 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=epli46Pz; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605632924; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kOaRJTqTP0bXFDGYIXnJOwRjuu8tlPQp4PguCqHKCQo=; b=epli46Pz0dZ4Cl/9MoJB2lUPZ+tBtJuPad1OgKMuiFuBXMjNtYofQC4utA1mRb/LGctXhB P8F4gC/xxqw+D1JnFPtDW0tN2cn7y/IhqtkxxxsYbG6tosjiN87XiGqt4yXxkLyC1Mo6RI dK5ZNMZy6SqpUM/QQNR5iNI/HvRKQnk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-222-wXanZMNoMdaQLvUBs3FQLg-1; Tue, 17 Nov 2020 12:08:42 -0500 X-MC-Unique: wXanZMNoMdaQLvUBs3FQLg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 069061800D42; Tue, 17 Nov 2020 17:08:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-97.ams2.redhat.com [10.36.112.97]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3CEF05B4A3; Tue, 17 Nov 2020 17:08:39 +0000 (UTC) Subject: Re: [edk2-devel] [NETWORK_HTTP_ENABLE PATCH 1/1] NetworkPkg: Add NETWORK_HTTP_ENABLE macro To: "Chang, Abner (HPS SW/FW Technologist)" , "devel@edk2.groups.io" Cc: Maciej Rabeda , Jiaxin Wu , Siyuan Fu , "Wang, Nickle (HPS SW)" , "O'Hanley, Peter (EXL)" References: <20201111131927.21323-1-abner.chang@hpe.com> <20201111131927.21323-2-abner.chang@hpe.com> <9357a533-108e-b4c3-6aa8-3f9dcea0846c@redhat.com> From: "Laszlo Ersek" Message-ID: <0a6b860d-5e45-20bb-0f44-42c4ac99f178@redhat.com> Date: Tue, 17 Nov 2020 18:08:38 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Hi Abner, On 11/16/20 03:32, Chang, Abner (HPS SW/FW Technologist) wrote: > > >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> Laszlo Ersek >> Sent: Thursday, November 12, 2020 5:22 AM >> To: Chang, Abner (HPS SW/FW Technologist) ; >> devel@edk2.groups.io >> Cc: Maciej Rabeda ; Jiaxin Wu >> ; Siyuan Fu ; Wang, Nickle (HPS >> SW) ; O'Hanley, Peter (EXL) >> >> 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.
>>> +# (C) Copyright 2020 Hewlett Packard Enterprise Development LP
>>> # >>> # 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