* [PATCH v2 0/2] Enable the HTTP connections switch @ 2017-01-17 3:33 Jiaxin Wu 2017-01-17 3:33 ` [PATCH v2 1/2] NetworkPkg: Add PCD to enable " Jiaxin Wu 2017-01-17 3:33 ` [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections Jiaxin Wu 0 siblings, 2 replies; 10+ messages in thread From: Jiaxin Wu @ 2017-01-17 3:33 UTC (permalink / raw) To: edk2-devel Cc: Ye Ting, Fu Siyuan, Ruiyu Ni, Laszlo Ersek, Kinney Michael D, Wu Jiaxin v2: * Rename the PCD to PcdAllowHttpConnections. * Rename the Nt32Pkg control flag. * Refine the PCD descriptions. If the value of PcdAllowHttpConnections 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. 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> Jiaxin Wu (2): NetworkPkg: Add PCD to enable the HTTP connections switch Nt32Pkg.dsc: Add flag to control HTTP connections NetworkPkg/HttpBootDxe/HttpBootClient.c | 20 +++++++- NetworkPkg/HttpBootDxe/HttpBootConfig.c | 81 ++++++++++++++++++++------------ NetworkPkg/HttpBootDxe/HttpBootDxe.inf | 5 +- NetworkPkg/HttpBootDxe/HttpBootSupport.c | 53 ++++++++++++++++++++- NetworkPkg/HttpBootDxe/HttpBootSupport.h | 17 ++++++- NetworkPkg/HttpDxe/HttpDxe.inf | 5 +- NetworkPkg/HttpDxe/HttpImpl.c | 12 ++++- NetworkPkg/NetworkPkg.dec | 8 +++- Nt32Pkg/Nt32Pkg.dsc | 18 ++++++- 9 files changed, 180 insertions(+), 39 deletions(-) -- 1.9.5.msysgit.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] NetworkPkg: Add PCD to enable the HTTP connections switch 2017-01-17 3:33 [PATCH v2 0/2] Enable the HTTP connections switch Jiaxin Wu @ 2017-01-17 3:33 ` Jiaxin Wu 2017-01-17 8:53 ` Laszlo Ersek 2017-01-17 3:33 ` [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections Jiaxin Wu 1 sibling, 1 reply; 10+ messages in thread From: Jiaxin Wu @ 2017-01-17 3:33 UTC (permalink / raw) To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Laszlo Ersek, Kinney Michael D, Wu Jiaxin v2: * Rename the PCD to PcdAllowHttpConnections. * Refine the PCD descriptions. If the value of PcdAllowHttpConnections 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. Cc: Ye Ting <ting.ye@intel.com> Cc: Fu Siyuan <siyuan.fu@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> --- NetworkPkg/HttpBootDxe/HttpBootClient.c | 20 +++++++- NetworkPkg/HttpBootDxe/HttpBootConfig.c | 81 ++++++++++++++++++++------------ NetworkPkg/HttpBootDxe/HttpBootDxe.inf | 5 +- NetworkPkg/HttpBootDxe/HttpBootSupport.c | 53 ++++++++++++++++++++- NetworkPkg/HttpBootDxe/HttpBootSupport.h | 17 ++++++- NetworkPkg/HttpDxe/HttpDxe.inf | 5 +- NetworkPkg/HttpDxe/HttpImpl.c | 12 ++++- NetworkPkg/NetworkPkg.dec | 8 +++- 8 files changed, 164 insertions(+), 37 deletions(-) diff --git a/NetworkPkg/HttpBootDxe/HttpBootClient.c b/NetworkPkg/HttpBootDxe/HttpBootClient.c index 916f237..99db3d5 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootClient.c +++ b/NetworkPkg/HttpBootDxe/HttpBootClient.c @@ -1,9 +1,9 @@ /** @file Implementation of the boot file download function. -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<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 that accompanies this distribution. The full text of the license may be found at http://opensource.org/licenses/bsd-license.php. @@ -190,10 +190,19 @@ HttpBootDhcp4ExtractUriInfo ( Private->BootFileUriParser = Private->FilePathUriParser; Private->BootFileUri = Private->FilePathUri; } // + // Check the URI scheme. + // + Status = HttpBootCheckUriScheme (Private->BootFileUri); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_ERROR, "HttpBootDhcp4ExtractUriInfo: %r.\n", Status)); + return Status; + } + + // // Configure the default DNS server if server assigned. // if ((SelectOffer->OfferType == HttpOfferTypeDhcpNameUriDns) || (SelectOffer->OfferType == HttpOfferTypeDhcpDns) || (SelectOffer->OfferType == HttpOfferTypeDhcpIpUriDns)) { @@ -293,10 +302,19 @@ HttpBootDhcp6ExtractUriInfo ( Private->BootFileUriParser = Private->FilePathUriParser; Private->BootFileUri = Private->FilePathUri; } // + // Check the URI scheme. + // + Status = HttpBootCheckUriScheme (Private->BootFileUri); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_ERROR, "HttpBootDhcp6ExtractUriInfo: %r.\n", Status)); + return Status; + } + + // // Set the Local station address to IP layer. // Status = HttpBootSetIp6Address (Private); if (EFI_ERROR (Status)) { return Status; diff --git a/NetworkPkg/HttpBootDxe/HttpBootConfig.c b/NetworkPkg/HttpBootDxe/HttpBootConfig.c index 7c883b8..f32bf18 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootConfig.c +++ b/NetworkPkg/HttpBootDxe/HttpBootConfig.c @@ -1,9 +1,9 @@ /** @file Helper functions for configuring or getting the parameters relating to HTTP Boot. -Copyright (c) 2016, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR> This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at http://opensource.org/licenses/bsd-license.php @@ -444,13 +444,20 @@ HttpBootFormCallback ( IN OUT EFI_IFR_TYPE_VALUE *Value, OUT EFI_BROWSER_ACTION_REQUEST *ActionRequest ) { EFI_INPUT_KEY Key; - UINTN Index; CHAR16 *Uri; + UINTN UriLen; + CHAR8 *AsciiUri; HTTP_BOOT_FORM_CALLBACK_INFO *CallbackInfo; + EFI_STATUS Status; + + Uri = NULL; + UriLen = 0; + AsciiUri = NULL; + Status = EFI_SUCCESS; if (This == NULL || Value == NULL) { return EFI_INVALID_PARAMETER; } @@ -464,53 +471,67 @@ HttpBootFormCallback ( case KEY_INITIATOR_URI: // // Get user input URI string // Uri = HiiGetString (CallbackInfo->RegisteredHandle, Value->string, NULL); - if (Uri == NULL) { - return EFI_UNSUPPORTED; - } // - // Convert the scheme to all lower case. + // The URI should be either an empty string (for corporate environment) ,or http(s) for home environment. + // Pop up a message box for the unsupported URI. // - for (Index = 0; Index < StrLen (Uri); Index++) { - if (Uri[Index] == L':') { - break; + if (StrLen (Uri) != 0) { + UriLen = StrLen (Uri) + 1; + AsciiUri = AllocateZeroPool (UriLen); + if (AsciiUri == NULL) { + FreePool (Uri); + return EFI_OUT_OF_RESOURCES; } - if (Uri[Index] >= L'A' && Uri[Index] <= L'Z') { - Uri[Index] -= (CHAR16)(L'A' - L'a'); + + UnicodeStrToAsciiStrS (Uri, AsciiUri, UriLen); + + Status = HttpBootCheckUriScheme (AsciiUri); + + if (Status == EFI_INVALID_PARAMETER) { + + DEBUG ((EFI_D_ERROR, "HttpBootFormCallback: %r.\n", Status)); + + CreatePopUp ( + EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, + &Key, + L"ERROR: Unsupported URI!", + L"Only supports HTTP and HTTPS", + NULL + ); + } else if (Status == EFI_ACCESS_DENIED) { + + DEBUG ((EFI_D_ERROR, "HttpBootFormCallback: %r.\n", Status)); + + CreatePopUp ( + EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, + &Key, + L"ERROR: Unsupported URI!", + L"HTTP is disabled", + NULL + ); } } - // - // Set the converted URI string back - // - HiiSetString (CallbackInfo->RegisteredHandle, Value->string, Uri, NULL); - - // - // The URI should be either an empty string (for corporate environment) ,or http(s) for home environment. - // Pop up a message box for other unsupported URI. - // - if ((StrLen (Uri) != 0) && (StrnCmp (Uri, L"http://", 7) != 0) && (StrnCmp (Uri, L"https://", 8) != 0)) { - CreatePopUp ( - EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE, - &Key, - L"ERROR: Unsupported URI!", - L"Only supports HTTP and HTTPS", - NULL - ); + if (Uri != NULL) { + FreePool (Uri); } - FreePool (Uri); + if (AsciiUri != NULL) { + FreePool (AsciiUri); + } + break; default: break; } - return EFI_SUCCESS; + return Status; } /** Initialize the configuration form. diff --git a/NetworkPkg/HttpBootDxe/HttpBootDxe.inf b/NetworkPkg/HttpBootDxe/HttpBootDxe.inf index e6ce864..982e6b4 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootDxe.inf +++ b/NetworkPkg/HttpBootDxe/HttpBootDxe.inf @@ -1,9 +1,9 @@ ## @file # This modules produce the Load File Protocol for UEFI HTTP boot. # -# Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> +# Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR> # This program and the accompanying materials # are licensed and made available under the terms and conditions of the BSD License # which accompanies this distribution. The full text of the license may be found at # http://opensource.org/licenses/bsd-license.php # @@ -92,7 +92,10 @@ ## SOMETIMES_CONSUMES ## HII gHttpBootConfigGuid gEfiVirtualCdGuid ## SOMETIMES_CONSUMES ## GUID gEfiVirtualDiskGuid ## SOMETIMES_CONSUMES ## GUID +[Pcd] + gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections ## CONSUMES + [UserExtensions.TianoCore."ExtraFiles"] HttpBootDxeExtra.uni diff --git a/NetworkPkg/HttpBootDxe/HttpBootSupport.c b/NetworkPkg/HttpBootDxe/HttpBootSupport.c index bdb29ae..69b129f 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootSupport.c +++ b/NetworkPkg/HttpBootDxe/HttpBootSupport.c @@ -1,9 +1,9 @@ /** @file Support functions implementation for UEFI HTTP boot driver. -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<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 that accompanies this distribution. The full text of the license may be found at http://opensource.org/licenses/bsd-license.php. @@ -987,10 +987,61 @@ HttpIoRecvResponse ( return Status; } /** + This function checks the HTTP(S) URI scheme. + + @param[in] Uri The pointer to the URI string. + + @retval EFI_SUCCESS The URI scheme is valid. + @retval EFI_INVALID_PARAMETER The URI scheme is not HTTP or HTTPS. + @retval EFI_ACCESS_DENIED HTTP is disabled and the URI is HTTP. + +**/ +EFI_STATUS +HttpBootCheckUriScheme ( + IN CHAR8 *Uri + ) +{ + UINTN Index; + EFI_STATUS Status; + + Status = EFI_SUCCESS; + + // + // Convert the scheme to all lower case. + // + for (Index = 0; Index < AsciiStrLen (Uri); Index++) { + if (Uri[Index] == ':') { + break; + } + if (Uri[Index] >= 'A' && Uri[Index] <= 'Z') { + Uri[Index] -= (CHAR8)('A' - 'a'); + } + } + + // + // Return EFI_INVALID_PARAMETER if the URI is not HTTP or HTTPS. + // + if ((AsciiStrnCmp (Uri, "http://", 7) != 0) && (AsciiStrnCmp (Uri, "https://", 8) != 0)) { + DEBUG ((EFI_D_ERROR, "HttpBootCheckUriScheme: Invalid Uri.\n")); + return EFI_INVALID_PARAMETER; + } + + // + // HTTP is disabled, return EFI_ACCESS_DENIED if the URI is HTTP. + // + if (!PcdGetBool (PcdAllowHttpConnections) && (AsciiStrnCmp (Uri, "http://", 7) == 0)) { + DEBUG ((EFI_D_ERROR, "HttpBootCheckUriScheme: HTTP is disabled.\n")); + return EFI_ACCESS_DENIED; + } + + return Status; +} + +/** Get the URI address string from the input device path. Caller need to free the buffer in the UriAddress pointer. @param[in] FilePath Pointer to the device path which contains a URI device path node. diff --git a/NetworkPkg/HttpBootDxe/HttpBootSupport.h b/NetworkPkg/HttpBootDxe/HttpBootSupport.h index 4d02427..65302d2 100644 --- a/NetworkPkg/HttpBootDxe/HttpBootSupport.h +++ b/NetworkPkg/HttpBootDxe/HttpBootSupport.h @@ -1,9 +1,9 @@ /** @file Support functions declaration for UEFI HTTP boot driver. -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR> This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License that accompanies this distribution. The full text of the license may be found at http://opensource.org/licenses/bsd-license.php. @@ -330,10 +330,25 @@ HttpIoRecvResponse ( IN BOOLEAN RecvMsgHeader, OUT HTTP_IO_RESPONSE_DATA *ResponseData ); /** + This function checks the HTTP(S) URI scheme. + + @param[in] Uri The pointer to the URI string. + + @retval EFI_SUCCESS The URI scheme is valid. + @retval EFI_INVALID_PARAMETER The URI scheme is not HTTP or HTTPS. + @retval EFI_ACCESS_DENIED HTTP is disabled and the URI is HTTP. + +**/ +EFI_STATUS +HttpBootCheckUriScheme ( + IN CHAR8 *Uri + ); + +/** Get the URI address string from the input device path. Caller need to free the buffer in the UriAddress pointer. @param[in] FilePath Pointer to the device path which contains a URI device path node. diff --git a/NetworkPkg/HttpDxe/HttpDxe.inf b/NetworkPkg/HttpDxe/HttpDxe.inf index 1118181..df2efdc 100644 --- a/NetworkPkg/HttpDxe/HttpDxe.inf +++ b/NetworkPkg/HttpDxe/HttpDxe.inf @@ -1,9 +1,9 @@ ## @file # Implementation of EFI HTTP protocol interfaces. # -# Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> +# Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR> # # This program and the accompanying materials # are licensed and made available under the terms and conditions of the BSD License # which accompanies this distribution. The full text of the license may be found at # http://opensource.org/licenses/bsd-license.php. @@ -73,7 +73,10 @@ gEfiTlsConfigurationProtocolGuid ## SOMETIMES_CONSUMES [Guids] gEfiTlsCaCertificateGuid ## CONSUMES ## GUID +[Pcd] + gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections ## CONSUMES + [UserExtensions.TianoCore."ExtraFiles"] HttpDxeExtra.uni \ No newline at end of file diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c index d19f733..85b0083 100644 --- a/NetworkPkg/HttpDxe/HttpImpl.c +++ b/NetworkPkg/HttpDxe/HttpImpl.c @@ -1,9 +1,9 @@ /** @file Implementation of EFI_HTTP_PROTOCOL protocol interfaces. - Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> + Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR> (C) Copyright 2015-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 which accompanies this distribution. The full text of the license may be found at @@ -353,10 +353,20 @@ EfiHttpRequest ( // be able to determine whether to use http or https. // HttpInstance->UseHttps = IsHttpsUrl (Url); // + // HTTP is disabled, return directly if the URI is not HTTPS. + // + if (!PcdGetBool (PcdAllowHttpConnections) && !(HttpInstance->UseHttps)) { + + DEBUG ((EFI_D_ERROR, "EfiHttpRequest: HTTP is disabled.\n")); + + return EFI_ACCESS_DENIED; + } + + // // Check whether we need to create Tls child and open the TLS protocol. // if (HttpInstance->UseHttps && HttpInstance->TlsChildHandle == NULL) { // // Use TlsSb to create Tls child and open the TLS protocol. diff --git a/NetworkPkg/NetworkPkg.dec b/NetworkPkg/NetworkPkg.dec index 24d45f4..d51f816 100644 --- a/NetworkPkg/NetworkPkg.dec +++ b/NetworkPkg/NetworkPkg.dec @@ -2,11 +2,11 @@ # Network Package. # # This package provides network modules that conform to UEFI 2.4 specification. # # (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR> -# Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR> +# Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR> # # This program and the accompanying materials are licensed and made available under # the terms and conditions of the BSD License which accompanies this distribution. # The full text of the license may be found at # http://opensource.org/licenses/bsd-license.php @@ -77,10 +77,16 @@ ## Private Key's size. # @Prompt Private Key's size. gEfiNetworkPkgTokenSpaceGuid.PcdIpsecUefiCertificateKeySize|0x3d5|UINT32|0x00000006 + ## Indicates whether HTTP connections (i.e., unsecured) are permitted or not. + # TRUE - HTTP connections is allowed. Both the "https://" and "http://" URI schemes are permitted. + # FALSE - HTTP connections is denied. Only the "https://" URI scheme is permitted. + # @Prompt Indicates whether HTTP connections are permitted or not. + gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|FALSE|BOOLEAN|0x00000008 + [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] ## IPv6 DHCP Unique Identifier (DUID) Type configuration (From RFCs 3315 and 6355). # 01 = DUID Based on Link-layer Address Plus Time [DUID-LLT] # 04 = UUID-Based DHCPv6 Unique Identifier (DUID-UUID) # 02 = DUID Assigned by Vendor Based on Enterprise Number [DUID-EN] (not supported) -- 1.9.5.msysgit.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] NetworkPkg: Add PCD to enable the HTTP connections switch 2017-01-17 3:33 ` [PATCH v2 1/2] NetworkPkg: Add PCD to enable " Jiaxin Wu @ 2017-01-17 8:53 ` Laszlo Ersek 0 siblings, 0 replies; 10+ messages in thread From: Laszlo Ersek @ 2017-01-17 8:53 UTC (permalink / raw) To: Jiaxin Wu, edk2-devel; +Cc: Ye Ting, Fu Siyuan, Kinney Michael D On 01/17/17 04:33, Jiaxin Wu wrote: > v2: > * Rename the PCD to PcdAllowHttpConnections. > * Refine the PCD descriptions. > > If the value of PcdAllowHttpConnections 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. > > Cc: Ye Ting <ting.ye@intel.com> > Cc: Fu Siyuan <siyuan.fu@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> > --- > NetworkPkg/HttpBootDxe/HttpBootClient.c | 20 +++++++- > NetworkPkg/HttpBootDxe/HttpBootConfig.c | 81 ++++++++++++++++++++------------ > NetworkPkg/HttpBootDxe/HttpBootDxe.inf | 5 +- > NetworkPkg/HttpBootDxe/HttpBootSupport.c | 53 ++++++++++++++++++++- > NetworkPkg/HttpBootDxe/HttpBootSupport.h | 17 ++++++- > NetworkPkg/HttpDxe/HttpDxe.inf | 5 +- > NetworkPkg/HttpDxe/HttpImpl.c | 12 ++++- > NetworkPkg/NetworkPkg.dec | 8 +++- > 8 files changed, 164 insertions(+), 37 deletions(-) [snip] > diff --git a/NetworkPkg/NetworkPkg.dec b/NetworkPkg/NetworkPkg.dec > index 24d45f4..d51f816 100644 > --- a/NetworkPkg/NetworkPkg.dec > +++ b/NetworkPkg/NetworkPkg.dec > @@ -2,11 +2,11 @@ > # Network Package. > # > # This package provides network modules that conform to UEFI 2.4 specification. > # > # (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR> > -# Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR> > +# Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR> > # > # This program and the accompanying materials are licensed and made available under > # the terms and conditions of the BSD License which accompanies this distribution. > # The full text of the license may be found at > # http://opensource.org/licenses/bsd-license.php > @@ -77,10 +77,16 @@ > > ## Private Key's size. > # @Prompt Private Key's size. > gEfiNetworkPkgTokenSpaceGuid.PcdIpsecUefiCertificateKeySize|0x3d5|UINT32|0x00000006 > > + ## Indicates whether HTTP connections (i.e., unsecured) are permitted or not. > + # TRUE - HTTP connections is allowed. Both the "https://" and "http://" URI schemes are permitted. > + # FALSE - HTTP connections is denied. Only the "https://" URI scheme is permitted. > + # @Prompt Indicates whether HTTP connections are permitted or not. > + gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|FALSE|BOOLEAN|0x00000008 > + > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] > ## IPv6 DHCP Unique Identifier (DUID) Type configuration (From RFCs 3315 and 6355). > # 01 = DUID Based on Link-layer Address Plus Time [DUID-LLT] > # 04 = UUID-Based DHCPv6 Unique Identifier (DUID-UUID) > # 02 = DUID Assigned by Vendor Based on Enterprise Number [DUID-EN] (not supported) > Minor nit: I suggest replacing connections is with connections are in the commit message and in the code. (I count four instances in total.) It can be done when you commit / push the patch. Other than that, this looks good to me, from a client platform's perspective. Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections 2017-01-17 3:33 [PATCH v2 0/2] Enable the HTTP connections switch Jiaxin Wu 2017-01-17 3:33 ` [PATCH v2 1/2] NetworkPkg: Add PCD to enable " Jiaxin Wu @ 2017-01-17 3:33 ` Jiaxin Wu 2017-01-17 10:02 ` Laszlo Ersek 1 sibling, 1 reply; 10+ messages in thread From: Jiaxin Wu @ 2017-01-17 3:33 UTC (permalink / raw) To: edk2-devel Cc: Ye Ting, Fu Siyuan, Ruiyu Ni, Laszlo Ersek, Kinney Michael D, Wu Jiaxin 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.PcdResetOnMemoryTypeInformationChange|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 -- 1.9.5.msysgit.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections 2017-01-17 3:33 ` [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections Jiaxin Wu @ 2017-01-17 10:02 ` Laszlo Ersek 2017-01-17 10:29 ` Gary Lin 2017-01-18 2:16 ` Wu, Jiaxin 0 siblings, 2 replies; 10+ messages in thread From: Laszlo Ersek @ 2017-01-17 10:02 UTC (permalink / raw) To: Jiaxin Wu, edk2-devel Cc: Ruiyu Ni, Ye Ting, Kinney Michael D, Fu Siyuan, Gary Ching-Pang Lin, Jordan Justen (Intel address) 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.PcdResetOnMemoryTypeInformationChange|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? 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? 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? (Note that in "OvmfPkgIa32X64.dsc", the setting should likely go under [PcdsFixedAtBuild.X64].) Thanks! Laszlo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections 2017-01-17 10:02 ` Laszlo Ersek @ 2017-01-17 10:29 ` Gary Lin 2017-01-18 2:16 ` Wu, Jiaxin 1 sibling, 0 replies; 10+ messages in thread From: Gary Lin @ 2017-01-17 10:29 UTC (permalink / raw) To: Laszlo Ersek Cc: Jiaxin Wu, edk2-devel, Ruiyu Ni, Ye Ting, Kinney Michael D, Fu Siyuan, Jordan Justen (Intel address) On Tue, Jan 17, 2017 at 11:02:17AM +0100, Laszlo Ersek wrote: > 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.PcdResetOnMemoryTypeInformationChange|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? > > 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? > > 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? I'm good to set PcdAllowHttpConnections to TRUE. For the people who are paranoid, it's easy to set it to FALSE as long as it's documented. The firmware file has to be rebuilt anyway whether the extra flag exists or not. Thanks, Gary Lin > > (Note that in "OvmfPkgIa32X64.dsc", the setting should likely go under > [PcdsFixedAtBuild.X64].) > > Thanks! > Laszlo > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections 2017-01-17 10:02 ` Laszlo Ersek 2017-01-17 10:29 ` Gary Lin @ 2017-01-18 2:16 ` Wu, Jiaxin 2017-01-18 8:30 ` Laszlo Ersek 1 sibling, 1 reply; 10+ messages in thread From: Wu, Jiaxin @ 2017-01-18 2:16 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@ml01.01.org Cc: Ni, Ruiyu, Ye, Ting, Kinney, Michael D, Fu, Siyuan, Gary Ching-Pang Lin, Justen, Jordan L > 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? Thanks, Jiaxin > (Note that in "OvmfPkgIa32X64.dsc", the setting should likely go under > [PcdsFixedAtBuild.X64].) > > Thanks! > Laszlo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections 2017-01-18 2:16 ` Wu, Jiaxin @ 2017-01-18 8:30 ` Laszlo Ersek 2017-01-18 9:27 ` Gary Lin 0 siblings, 1 reply; 10+ messages in thread From: Laszlo Ersek @ 2017-01-18 8:30 UTC (permalink / raw) To: Wu, Jiaxin, edk2-devel@ml01.01.org Cc: Ni, Ruiyu, Ye, Ting, Kinney, Michael D, Fu, Siyuan, Gary Ching-Pang Lin, Justen, Jordan L 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections 2017-01-18 8:30 ` Laszlo Ersek @ 2017-01-18 9:27 ` Gary Lin 2017-01-19 3:19 ` Wu, Jiaxin 0 siblings, 1 reply; 10+ messages in thread From: Gary Lin @ 2017-01-18 9:27 UTC (permalink / raw) To: Laszlo Ersek Cc: Wu, Jiaxin, edk2-devel@ml01.01.org, Ni, Ruiyu, Ye, Ting, Justen, Jordan L, Kinney, Michael D, Fu, Siyuan 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 <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? > (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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections 2017-01-18 9:27 ` Gary Lin @ 2017-01-19 3:19 ` Wu, Jiaxin 0 siblings, 0 replies; 10+ messages in thread From: Wu, Jiaxin @ 2017-01-19 3:19 UTC (permalink / raw) To: Gary Lin, Laszlo Ersek Cc: Ni, Ruiyu, Ye, Ting, Justen, Jordan L, edk2-devel@ml01.01.org, Kinney, Michael D, Fu, Siyuan > > >> 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. > Ok, I will append a third patch for OVMF with solution (2) but keep the ALLOW_HTTP_CONNECTIONS only for Nt32Pkg. !if $( HTTP_BOOT_ENABLE) == TRUE gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE !endif Thanks all of your comments. Jiaxin > 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 > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-01-19 3:19 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-17 3:33 [PATCH v2 0/2] Enable the HTTP connections switch Jiaxin Wu 2017-01-17 3:33 ` [PATCH v2 1/2] NetworkPkg: Add PCD to enable " Jiaxin Wu 2017-01-17 8:53 ` Laszlo Ersek 2017-01-17 3:33 ` [PATCH v2 2/2] Nt32Pkg.dsc: Add flag to control HTTP connections Jiaxin Wu 2017-01-17 10:02 ` Laszlo Ersek 2017-01-17 10:29 ` Gary Lin 2017-01-18 2:16 ` Wu, Jiaxin 2017-01-18 8:30 ` Laszlo Ersek 2017-01-18 9:27 ` Gary Lin 2017-01-19 3:19 ` Wu, Jiaxin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox