[edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

From: Abner Chang <abner.chang@amd.com>

For the HTTPS connetion that doesn't require TLS peer verification,
EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL is introduced to platform
developer to provide the TLS configure data that is different than
the default TLS configuration. The use case such as Redfish service
connction which doesn't require the TLS peer verification on the
cetificate, especially to the Redfish service connection through
the in-band network interface.

Platform developer can provide this protoocl to EFI HTTP driver to
configure TLS using TLS conifg data provided by
EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL for the specific HTTP
protocol handle. How to distinguish the correct HTTP protocol
handle for the platform TLS policy is outside the scope of this
change. For Redfish, we will provide this protocol in EFI Redfish
REST EX driver.

Question:
Do we need the version control of platform TLS configuration
data structure for the flexibility in future?

Signed-off-by: Abner Chang <abner.chang@amd.com>
Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
Cc: Michael Brown <mcb30@ipxe.org>
Cc: Nickle Wang <nicklew@nvidia.com>
Cc: Igor Kulchytskyy <igork@ami.com>

Abenr Chang (1):
  NetworkPkg: Check platform TLS policy

Abner Chang (1):
  NetworkPkg: EDKII HTTPS platform TLS policy

 NetworkPkg/NetworkPkg.dec                     |   3 +
 NetworkPkg/HttpDxe/HttpDxe.inf                |   1 +
 NetworkPkg/HttpDxe/HttpDriver.h               |   1 +
 .../Protocol/HttpsTlsPlatformPolicyProtocol.h |  72 +++++++++++
 NetworkPkg/HttpDxe/HttpsSupport.c             | 117 ++++++++++++++++--
 5 files changed, 182 insertions(+), 12 deletions(-)
 create mode 100644 NetworkPkg/Include/Protocol/HttpsTlsPlatformPolicyProtocol.h

-- 
2.37.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112912): https://edk2.groups.io/g/devel/message/112912
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [RFC][PATCH 1/2] NetworkPkg: EDKII HTTPS platform TLS policy

[Chang, Abner via groups.io]

From: Abner Chang <abner.chang@amd.com>

Definitions of EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL.

Signed-off-by: Abner Chang <abner.chang@amd.com>
Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
Cc: Michael Brown <mcb30@ipxe.org>
Cc: Nickle Wang <nicklew@nvidia.com>
Cc: Igor Kulchytskyy <igork@ami.com>
---
 NetworkPkg/NetworkPkg.dec                     |  3 +
 .../Protocol/HttpsTlsPlatformPolicyProtocol.h | 72 +++++++++++++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 NetworkPkg/Include/Protocol/HttpsTlsPlatformPolicyProtocol.h

diff --git a/NetworkPkg/NetworkPkg.dec b/NetworkPkg/NetworkPkg.dec
index e06f35e7747..88676c7eaf6 100644
--- a/NetworkPkg/NetworkPkg.dec
+++ b/NetworkPkg/NetworkPkg.dec
@@ -94,6 +94,9 @@
   ## Include/Protocol/WiFiProfileSyncProtocol.h
   gEdkiiWiFiProfileSyncProtocolGuid = {0x399a2b8a, 0xc267, 0x44aa, {0x9a, 0xb4, 0x30, 0x58, 0x8c, 0xd2, 0x2d, 0xcc}}
 
+  ## Include/Protocol/HttpsTlsPlatformPolicyProtocol.h
+  gEdkiiHttpsTlsPlatformPolicyProtocolGuid = {0xbfe8e3e3, 0xb884, 0x4a6f, {0xae, 0xd3, 0xb8, 0xdb, 0xeb, 0xc5, 0x58, 0xc0}}
+
 [PcdsFixedAtBuild]
   ## The max attempt number will be created by iSCSI driver.
   # @Prompt Max attempt number.
diff --git a/NetworkPkg/Include/Protocol/HttpsTlsPlatformPolicyProtocol.h b/NetworkPkg/Include/Protocol/HttpsTlsPlatformPolicyProtocol.h
new file mode 100644
index 00000000000..5f82ceba924
--- /dev/null
+++ b/NetworkPkg/Include/Protocol/HttpsTlsPlatformPolicyProtocol.h
@@ -0,0 +1,72 @@
+/** @file
+  This file defines the EDKII HTTPS TLS Platform Protocol interface.
+
+  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL_H_
+#define EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL_H_
+
+#include <Protocol/Http.h>
+#include <Protocol/Tls.h>
+
+#define EEDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL_GUID \
+  { \
+    0xbfe8e3e3, 0xb884, 0x4a6f, {0xae, 0xd3, 0xb8, 0xdb, 0xeb, 0xc5, 0x58, 0xc0} \
+  }
+
+typedef struct _EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL;
+
+///
+/// EDKII_PLATFORM_HTTPS_TLS_CONFIG_DATA_VERSION
+///
+typedef struct {
+  UINT8    Major;
+  UINT8    Minor;
+} EDKII_PLATFORM_HTTPS_TLS_CONFIG_DATA_VERSION;
+
+typedef struct {
+  EDKII_PLATFORM_HTTPS_TLS_CONFIG_DATA_VERSION    Version;
+  ///
+  /// EDKII_PLATFORM_HTTPS_TLS_CONFIG_DATA_VERSION V1.0
+  ///
+  EFI_TLS_CONNECTION_END                          ConnectionEnd;
+  EFI_TLS_VERIFY                                  VerifyMethod;
+  EFI_TLS_VERIFY_HOST                             VerifyHost;
+} EDKII_PLATFORM_HTTPS_TLS_CONFIG_DATA;
+
+/**
+  Function to get platform HTTPS TLS Policy.
+
+  @param[in]   This                   Pointer to the EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL
+                                      instance.
+  @param[in]   HttpHandle             EFI_HTTP_PROTOCOL handle used to transfer HTTP payload.
+  @param[out]  PlatformPolicy         Pointer to retrieve EDKII_PLATFORM_HTTPS_TLS_CONFIG_DATA.
+
+  @retval EFI_SUCCESS                 Platform HTTPS TLS config data is returned in
+                                      PlatformPolicy.
+  @retval EFI_INVALID_PARAMETER       Either HttpHandle or PlatformPolicy is NULL, or both are NULL.
+  @retval EFI_NOT_FOUND               No HTTP protocol insterface is found on HttpHandle.
+  @retval EFI_UNSUPPORTED             HttpProtocolInstance is not the HTTP instance platform
+                                      would like to config.
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_HTTPS_TLS_GET_PLATFORM_POLICY)(
+  IN   EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL  *This,
+  IN   EFI_HANDLE                                HttpHandle,
+  OUT  EDKII_PLATFORM_HTTPS_TLS_CONFIG_DATA      *PlatformPolicy
+  );
+
+///
+/// Platform can install more than one EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL
+/// instances to return the platfrom HTTP TLS policy config data for the
+/// multiple HTTP instances.
+///
+struct _EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL {
+  EDKII_HTTPS_TLS_GET_PLATFORM_POLICY    PlatformGetPolicy;
+};
+
+extern EFI_GUID  gEdkiiHttpsTlsPlatformPolicyProtocolGuid;
+#endif // EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL_H_
-- 
2.37.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112914): https://edk2.groups.io/g/devel/message/112914
Mute This Topic: https://groups.io/mt/103368440/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [RFC][PATCH 2/2] NetworkPkg: Check platform TLS policy

[Chang, Abner via groups.io]

From: Abenr Chang <abner.chang@amd.com>

Go through each
EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL protocol
instance to check if platform HTTPS TLS policy is
provided.

Signed-off-by: Abner Chang <abner.chang@amd.com>
Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
Cc: Michael Brown <mcb30@ipxe.org>
Cc: Nickle Wang <nicklew@nvidia.com>
Cc: Igor Kulchytskyy <igork@ami.com>
---
 NetworkPkg/HttpDxe/HttpDxe.inf    |   1 +
 NetworkPkg/HttpDxe/HttpDriver.h   |   1 +
 NetworkPkg/HttpDxe/HttpsSupport.c | 117 +++++++++++++++++++++++++++---
 3 files changed, 107 insertions(+), 12 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpDxe.inf b/NetworkPkg/HttpDxe/HttpDxe.inf
index c9502d0bb6d..7699bd9cc17 100644
--- a/NetworkPkg/HttpDxe/HttpDxe.inf
+++ b/NetworkPkg/HttpDxe/HttpDxe.inf
@@ -66,6 +66,7 @@
   gEfiTlsProtocolGuid                              ## SOMETIMES_CONSUMES
   gEfiTlsConfigurationProtocolGuid                 ## SOMETIMES_CONSUMES
   gEdkiiHttpCallbackProtocolGuid                   ## SOMETIMES_CONSUMES
+  gEdkiiHttpsTlsPlatformPolicyProtocolGuid         ## SOMETIMES_CONSUMES
 
 [Guids]
   gEfiTlsCaCertificateGuid                         ## SOMETIMES_CONSUMES  ## Variable:L"TlsCaCertificate"
diff --git a/NetworkPkg/HttpDxe/HttpDriver.h b/NetworkPkg/HttpDxe/HttpDriver.h
index 01a6bb7f4b7..5554befad4d 100644
--- a/NetworkPkg/HttpDxe/HttpDriver.h
+++ b/NetworkPkg/HttpDxe/HttpDriver.h
@@ -48,6 +48,7 @@
 #include <Protocol/Tls.h>
 #include <Protocol/TlsConfig.h>
 #include <Protocol/HttpCallback.h>
+#include <Protocol/HttpsTlsPlatformPolicyProtocol.h>
 
 #include <Guid/ImageAuthentication.h>
 //
diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c
index 7330be42c00..354e5cfc79c 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -131,6 +131,93 @@ IsHttpsUrl (
   return FALSE;
 }
 
+/**
+  Locate all EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL instances and go through each
+  to check if platform HTTPS TLS policy is provided.
+
+  @param[in]       HttpHandle         The HTTP protocol handle.
+  @param[in, out]  TlsConfigData      Pointer to TLS_CONFIG_DATA of this HTTP instance.
+
+**/
+VOID
+HttpsPlatformTlsPolicy (
+  IN EFI_HANDLE           HttpHandle,
+  IN OUT TLS_CONFIG_DATA  *TlsConfigData
+  )
+{
+  EFI_STATUS                                Status;
+  UINTN                                     NumHandles;
+  EFI_HANDLE                                *HandleBuffer;
+  EFI_HANDLE                                *HandleBufferIndex;
+  EDKII_PLATFORM_HTTPS_TLS_CONFIG_DATA      PlatformHttpsTlsPolicy;
+  EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL  *ProtocolInterface;
+
+  if ((HttpHandle == NULL) || (TlsConfigData == NULL)) {
+    return;
+  }
+
+  Status = gBS->LocateHandleBuffer (
+                  ByProtocol,
+                  &gEdkiiHttpsTlsPlatformPolicyProtocolGuid,
+                  NULL,
+                  &NumHandles,
+                  &HandleBuffer
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_INFO,
+      "%a: There is no EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL instance is installed for HTTP this handle:0x%x.\n",
+      __func__,
+      HttpHandle
+      ));
+    return;
+  }
+
+  HandleBufferIndex = HandleBuffer;
+  while (NumHandles != 0) {
+    Status = gBS->HandleProtocol (
+                    *HandleBufferIndex,
+                    &gEdkiiHttpsTlsPlatformPolicyProtocolGuid,
+                    (VOID **)&ProtocolInterface
+                    );
+    if (!EFI_ERROR (Status)) {
+      Status = ProtocolInterface->PlatformGetPolicy (
+                                    *HandleBufferIndex,
+                                    HttpHandle,
+                                    &PlatformHttpsTlsPolicy
+                                    );
+      if (!EFI_ERROR (Status)) {
+        if ((PlatformHttpsTlsPolicy.Version.Major == 1) && (PlatformHttpsTlsPolicy.Version.Minor == 0)) {
+          //
+          // HTTPS platform TLS policy config data version 1.0.
+          //
+          TlsConfigData->ConnectionEnd = PlatformHttpsTlsPolicy.ConnectionEnd;
+          TlsConfigData->VerifyHost    = PlatformHttpsTlsPolicy.VerifyHost;
+          TlsConfigData->VerifyMethod  = PlatformHttpsTlsPolicy.VerifyMethod;
+          Status                       = EFI_SUCCESS;
+          break;
+        }
+      }
+    }
+
+    HandleBufferIndex++;
+    NumHandles--;
+    Status = EFI_NOT_FOUND;
+  }
+
+  FreePool ((VOID *)HandleBuffer);
+  if (!EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_INFO,
+      "%a: There is a EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL instance installed for this HTTP handle:0x%x.\n",
+      __func__,
+      HttpHandle
+      ));
+  }
+
+  return;
+}
+
 /**
   Creates a Tls child handle, open EFI_TLS_PROTOCOL and EFI_TLS_CONFIGURATION_PROTOCOL.
 
@@ -650,6 +737,8 @@ TlsConfigureSession (
   HttpInstance->TlsConfigData.VerifyHost.HostName = HttpInstance->RemoteHost;
   HttpInstance->TlsConfigData.SessionState        = EfiTlsSessionNotStarted;
 
+  HttpsPlatformTlsPolicy (HttpInstance->Handle, &HttpInstance->TlsConfigData);
+
   //
   // EfiTlsConnectionEnd,
   // EfiTlsVerifyMethod,
@@ -676,14 +765,16 @@ TlsConfigureSession (
     return Status;
   }
 
-  Status = HttpInstance->Tls->SetSessionData (
-                                HttpInstance->Tls,
-                                EfiTlsVerifyHost,
-                                &HttpInstance->TlsConfigData.VerifyHost,
-                                sizeof (EFI_TLS_VERIFY_HOST)
-                                );
-  if (EFI_ERROR (Status)) {
-    return Status;
+  if (HttpInstance->TlsConfigData.VerifyMethod != EFI_TLS_VERIFY_NONE) {
+    Status = HttpInstance->Tls->SetSessionData (
+                                  HttpInstance->Tls,
+                                  EfiTlsVerifyHost,
+                                  &HttpInstance->TlsConfigData.VerifyHost,
+                                  sizeof (EFI_TLS_VERIFY_HOST)
+                                  );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
   }
 
   Status = HttpInstance->Tls->SetSessionData (
@@ -708,10 +799,12 @@ TlsConfigureSession (
   //
   // Tls Config Certificate
   //
-  Status = TlsConfigCertificate (HttpInstance);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "TLS Certificate Config Error!\n"));
-    return Status;
+  if (HttpInstance->TlsConfigData.VerifyMethod != EFI_TLS_VERIFY_NONE) {
+    Status = TlsConfigCertificate (HttpInstance);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "TLS Certificate Config Error!\n"));
+      return Status;
+    }
   }
 
   //
-- 
2.37.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112913): https://edk2.groups.io/g/devel/message/112913
Mute This Topic: https://groups.io/mt/103368439/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 26/12/2023 11:28, Chang, Abner via groups.io wrote:
> For the HTTPS connetion that doesn't require TLS peer verification,
> EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL is introduced to platform
> developer to provide the TLS configure data that is different than
> the default TLS configuration. The use case such as Redfish service
> connction which doesn't require the TLS peer verification on the
> cetificate, especially to the Redfish service connection through
> the in-band network interface.
> 
> Platform developer can provide this protoocl to EFI HTTP driver to
> configure TLS using TLS conifg data provided by
> EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL for the specific HTTP
> protocol handle. How to distinguish the correct HTTP protocol
> handle for the platform TLS policy is outside the scope of this
> change. For Redfish, we will provide this protocol in EFI Redfish
> REST EX driver.

This looks messy to me.

Did you try my suggestion of using RegisterProtocolNotify() in order to 
register a callback that will be called for any new instances of 
EFI_TLS_PROTOCOL?

This would be functionally equivalent to your patch, but with zero lines 
of additional code required in HttpDxe.

(My apologies if you did try it and already found a reason why it would 
not work - I have not been able to keep up with all EDK2 list messages.)

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112932): https://edk2.groups.io/g/devel/message/112932
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: Wednesday, December 27, 2023 11:55 PM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 26/12/2023 11:28, Chang, Abner via groups.io wrote:
> > For the HTTPS connetion that doesn't require TLS peer verification,
> > EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL is introduced to platform
> > developer to provide the TLS configure data that is different than
> > the default TLS configuration. The use case such as Redfish service
> > connction which doesn't require the TLS peer verification on the
> > cetificate, especially to the Redfish service connection through
> > the in-band network interface.
> >
> > Platform developer can provide this protoocl to EFI HTTP driver to
> > configure TLS using TLS conifg data provided by
> > EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL for the specific HTTP
> > protocol handle. How to distinguish the correct HTTP protocol
> > handle for the platform TLS policy is outside the scope of this
> > change. For Redfish, we will provide this protocol in EFI Redfish
> > REST EX driver.
>
> This looks messy to me.
>
> Did you try my suggestion of using RegisterProtocolNotify() in order to
> register a callback that will be called for any new instances of
> EFI_TLS_PROTOCOL?
>
> This would be functionally equivalent to your patch, but with zero lines
> of additional code required in HttpDxe.

I think you suggest to hook/replace the EFI_TLS_PROTOCOL for the specific HTTP handle?
EFI_TLS_PROTOCOL is installed implicitly when the first time HTTPs request is performed. There is no connection between HTTP handle and EFI TLS protocol instance besides the HTTP driver internal structure.
Listen to the installation of EFI_TLS_PROTOCOL has no way to distinguish the dedicated HTTP handle, for example the HTTP handle created by Redfish REST EX driver.
I don’t see the chance to provide the flexibility to TLS config with using RegisterProtocolNotify for EFI_TLS_PROTOCO unless we add one line to install the same TLS_PTOTOCOL on the given HTTP instance.  Or something I missed?

Thanks
Abner

>
> (My apologies if you did try it and already found a reason why it would
> not work - I have not been able to keep up with all EDK2 list messages.)
>
> Thanks,
>
> Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112937): https://edk2.groups.io/g/devel/message/112937
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 28/12/2023 02:47, Chang, Abner via groups.io wrote:
>> On 26/12/2023 11:28, Chang, Abner via groups.io wrote:
>>> Platform developer can provide this protoocl to EFI HTTP driver to
>>> configure TLS using TLS conifg data provided by
>>> EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL for the specific HTTP
>>> protocol handle. How to distinguish the correct HTTP protocol
>>> handle for the platform TLS policy is outside the scope of this
>>> change. For Redfish, we will provide this protocol in EFI Redfish
>>> REST EX driver.
>>
>> This looks messy to me.
>>
>> Did you try my suggestion of using RegisterProtocolNotify() in order to
>> register a callback that will be called for any new instances of
>> EFI_TLS_PROTOCOL?
>>
>> This would be functionally equivalent to your patch, but with zero lines
>> of additional code required in HttpDxe.
> 
> I think you suggest to hook/replace the EFI_TLS_PROTOCOL for the specific HTTP handle?
> EFI_TLS_PROTOCOL is installed implicitly when the first time HTTPs request is performed. There is no connection between HTTP handle and EFI TLS protocol instance besides the HTTP driver internal structure.
> Listen to the installation of EFI_TLS_PROTOCOL has no way to distinguish the dedicated HTTP handle, for example the HTTP handle created by Redfish REST EX driver.
> I don’t see the chance to provide the flexibility to TLS config with using RegisterProtocolNotify for EFI_TLS_PROTOCO unless we add one line to install the same TLS_PTOTOCOL on the given HTTP instance.  Or something I missed?

Having looked through the code in more detail, you are correct that 
there is no connection between the TLS handle and the HTTP handle, since 
TlsCreateChild() in HttpsSupport.c currently chooses to call 
(*TlsSb)->CreateChild() with a NULL handle and does not ever call 
OpenProtocol() with BY_CHILD to set up a parent/child relationship.

I would therefore suggest a mild refactoring of TlsCreateChild() so that 
it installs the TLS protocols directly onto the HTTP handle.  This 
refactoring does not break any APIs, since TlsCreateChild() is purely 
internal to HttpDxe.

Since all other functions in HttpsSupport.c seem to take HttpInstance as 
the first parameter, I would probably change TlsCreateChild() to do the 
same:

EFI_STATUS
EFIAPI
TlsCreateChild (
   IN OUT HTTP_PROTOCOL  *HttpInstance
   )

There is only one call site of TlsCreateChild() (in EfiHttpRequest()) 
and so the most obvious approach would be to move the logic around the 
call site to be inside of TlsCreateChild():

   if (HttpInstance->LocalAddressIsIPv6) {
     ImageHandle = HttpInstance->Service->Ip6DriverBindingHandle;
   } else {
     ImageHandle = HttpInstance->Service->Ip4DriverBindingHandle;
   }

and then use HttpInstance->TlsSb in place of the current *TlsSb, etc, 
within TlsCreateChild().

The call within TlsCreateChild() to HttpInstance->TlsSb->CreateChild() 
can then pass &HttpInstance->Handle as the second parameter, so that the 
EFI_TLS_PROTOCOL is installed onto the HTTP handle.

This refactoring would have the side effect of cleaning up 
TlsCreateChild() to be consistent with other functions in the same file, 
and would allow it to return an EFI_STATUS for more meaningful error 
reporting.

With the TLS protocol installed onto the same handle, I don't think you 
then even need to use RegisterProtocolNotify().  On return from 
EFI_HTTP_PROTOCOL.Request() you can open the TLS protocol on the handle 
and immediately call SetSessionData() to override VerifyMethod etc.

What do you think?

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112980): https://edk2.groups.io/g/devel/message/112980
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: Thursday, December 28, 2023 10:16 PM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 28/12/2023 02:47, Chang, Abner via groups.io wrote:
> >> On 26/12/2023 11:28, Chang, Abner via groups.io wrote:
> >>> Platform developer can provide this protoocl to EFI HTTP driver to
> >>> configure TLS using TLS conifg data provided by
> >>> EDKII_HTTPS_TLS_PLATFORM_POLICY_PROTOCOL for the specific HTTP
> >>> protocol handle. How to distinguish the correct HTTP protocol
> >>> handle for the platform TLS policy is outside the scope of this
> >>> change. For Redfish, we will provide this protocol in EFI Redfish
> >>> REST EX driver.
> >>
> >> This looks messy to me.
> >>
> >> Did you try my suggestion of using RegisterProtocolNotify() in order to
> >> register a callback that will be called for any new instances of
> >> EFI_TLS_PROTOCOL?
> >>
> >> This would be functionally equivalent to your patch, but with zero lines
> >> of additional code required in HttpDxe.
> >
> > I think you suggest to hook/replace the EFI_TLS_PROTOCOL for the specific
> HTTP handle?
> > EFI_TLS_PROTOCOL is installed implicitly when the first time HTTPs request is
> performed. There is no connection between HTTP handle and EFI TLS protocol
> instance besides the HTTP driver internal structure.
> > Listen to the installation of EFI_TLS_PROTOCOL has no way to distinguish the
> dedicated HTTP handle, for example the HTTP handle created by Redfish REST
> EX driver.
> > I don’t see the chance to provide the flexibility to TLS config with using
> RegisterProtocolNotify for EFI_TLS_PROTOCO unless we add one line to install
> the same TLS_PTOTOCOL on the given HTTP instance.  Or something I missed?
>
> Having looked through the code in more detail, you are correct that
> there is no connection between the TLS handle and the HTTP handle, since
> TlsCreateChild() in HttpsSupport.c currently chooses to call
> (*TlsSb)->CreateChild() with a NULL handle and does not ever call
> OpenProtocol() with BY_CHILD to set up a parent/child relationship.
>
> I would therefore suggest a mild refactoring of TlsCreateChild() so that
> it installs the TLS protocols directly onto the HTTP handle.  This
> refactoring does not break any APIs, since TlsCreateChild() is purely
> internal to HttpDxe.
>
> Since all other functions in HttpsSupport.c seem to take HttpInstance as
> the first parameter, I would probably change TlsCreateChild() to do the
> same:
>
> EFI_STATUS
> EFIAPI
> TlsCreateChild (
>    IN OUT HTTP_PROTOCOL  *HttpInstance
>    )
>
> There is only one call site of TlsCreateChild() (in EfiHttpRequest())
> and so the most obvious approach would be to move the logic around the
> call site to be inside of TlsCreateChild():
>
>    if (HttpInstance->LocalAddressIsIPv6) {
>      ImageHandle = HttpInstance->Service->Ip6DriverBindingHandle;
>    } else {
>      ImageHandle = HttpInstance->Service->Ip4DriverBindingHandle;
>    }
>
> and then use HttpInstance->TlsSb in place of the current *TlsSb, etc,
> within TlsCreateChild().
>
> The call within TlsCreateChild() to HttpInstance->TlsSb->CreateChild()
> can then pass &HttpInstance->Handle as the second parameter, so that the
> EFI_TLS_PROTOCOL is installed onto the HTTP handle.
>
> This refactoring would have the side effect of cleaning up
> TlsCreateChild() to be consistent with other functions in the same file,
> and would allow it to return an EFI_STATUS for more meaningful error
> reporting.
This is definitely perfect if we can make some changes on HttpSupport. The reason we introduced a new protocol is to prevent from changing HttpSupport too much, however this is not ideal as you mentioned it looks messy.

>
> With the TLS protocol installed onto the same handle, I don't think you
> then even need to use RegisterProtocolNotify().  On return from
> EFI_HTTP_PROTOCOL.Request() you can open the TLS protocol on the handle
> and immediately call SetSessionData() to override VerifyMethod etc.
>
This part I am not sure, as TLS is initiated on the first HttpRequest. Reconfigure TLS session on return from HTTP Request function means we have to take one time error. I think I will still use RegisterProtocolNotify and LocateHandle with ByRegisterNotify to get the newly installed TLS handle, then check it with REST EX HTTP handle. Hook the TLS SetSessionData() function provided by REST EX, override the value then invoke the original SetSessionData(). Something like this.

Would you like to refactor HttpSupport.c or let me do that?
Thanks
Abner

> What do you think?
>
> Thanks,
>
> Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112981): https://edk2.groups.io/g/devel/message/112981
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 28/12/2023 15:04, Chang, Abner via groups.io wrote:
>> With the TLS protocol installed onto the same handle, I don't think you
>> then even need to use RegisterProtocolNotify().  On return from
>> EFI_HTTP_PROTOCOL.Request() you can open the TLS protocol on the handle
>> and immediately call SetSessionData() to override VerifyMethod etc.
>>
> This part I am not sure, as TLS is initiated on the first HttpRequest. Reconfigure TLS session on return from HTTP Request function means we have to take one time error. I think I will still use RegisterProtocolNotify and LocateHandle with ByRegisterNotify to get the newly installed TLS handle, then check it with REST EX HTTP handle. Hook the TLS SetSessionData() function provided by REST EX, override the value then invoke the original SetSessionData(). Something like this.

As far as I am aware, EfiHttpRequest sets up all of the relevant data 
structures but functions as a non-blocking open.  If you reconfigure the 
TLS session immediately after return from EfiHttpRequest() then this 
reconfiguration should take effect before any network packets have been 
transmitted or received.  I have not tested this, though.

If the immediate reconfiguration does not work, then your suggestion of 
hooking SetSessionData() sounds like the easiest approach.

> Would you like to refactor HttpSupport.c or let me do that?

Nobody is paying me to work on EDK2, so I'll leave it to you.  :)

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112982): https://edk2.groups.io/g/devel/message/112982
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: Thursday, December 28, 2023 11:32 PM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 28/12/2023 15:04, Chang, Abner via groups.io wrote:
> >> With the TLS protocol installed onto the same handle, I don't think you
> >> then even need to use RegisterProtocolNotify().  On return from
> >> EFI_HTTP_PROTOCOL.Request() you can open the TLS protocol on the
> handle
> >> and immediately call SetSessionData() to override VerifyMethod etc.
> >>
> > This part I am not sure, as TLS is initiated on the first HttpRequest.
> Reconfigure TLS session on return from HTTP Request function means we
> have to take one time error. I think I will still use RegisterProtocolNotify and
> LocateHandle with ByRegisterNotify to get the newly installed TLS handle, then
> check it with REST EX HTTP handle. Hook the TLS SetSessionData() function
> provided by REST EX, override the value then invoke the original
> SetSessionData(). Something like this.
>
> As far as I am aware, EfiHttpRequest sets up all of the relevant data
> structures but functions as a non-blocking open.  If you reconfigure the
> TLS session immediately after return from EfiHttpRequest() then this
> reconfiguration should take effect before any network packets have been
> transmitted or received.  I have not tested this, though.
>
> If the immediate reconfiguration does not work, then your suggestion of
> hooking SetSessionData() sounds like the easiest approach.
I think the non-blocking transfer still sends out the request but just not waiting the response there, have to check the implementation.

>
> > Would you like to refactor HttpSupport.c or let me do that?
>
> Nobody is paying me to work on EDK2, so I'll leave it to you.  :)
Sounds good. 😊

Thanks
Abner
>
> Thanks,
>
> Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112994): https://edk2.groups.io/g/devel/message/112994
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 28/12/2023 23:37, Chang, Abner via groups.io wrote:
>> As far as I am aware, EfiHttpRequest sets up all of the relevant data
>> structures but functions as a non-blocking open.  If you reconfigure the
>> TLS session immediately after return from EfiHttpRequest() then this
>> reconfiguration should take effect before any network packets have been
>> transmitted or received.  I have not tested this, though.
>>
>> If the immediate reconfiguration does not work, then your suggestion of
>> hooking SetSessionData() sounds like the easiest approach.
> I think the non-blocking transfer still sends out the request but just not waiting the response there, have to check the implementation.

The code seems to construct the HTTP request and enqueue it, but unless 
it blocks polling on the network somewhere then the most it can do in 
terms of network I/O is to send out the initial TCP SYN.  (Not even 
that, if a DNS lookup is required.)

The implementation could plausibly construct and enqueue the 
ClientHello, in which case it would be too late to modify the cipher 
suite list, but any attempt to verify the hostname definitely can't 
happen until a lot of network I/O has taken place.

Good luck! :)

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112995): https://edk2.groups.io/g/devel/message/112995
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: Friday, December 29, 2023 8:01 AM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 28/12/2023 23:37, Chang, Abner via groups.io wrote:
> >> As far as I am aware, EfiHttpRequest sets up all of the relevant data
> >> structures but functions as a non-blocking open.  If you reconfigure the
> >> TLS session immediately after return from EfiHttpRequest() then this
> >> reconfiguration should take effect before any network packets have been
> >> transmitted or received.  I have not tested this, though.
> >>
> >> If the immediate reconfiguration does not work, then your suggestion of
> >> hooking SetSessionData() sounds like the easiest approach.
> > I think the non-blocking transfer still sends out the request but just not
> waiting the response there, have to check the implementation.
>
> The code seems to construct the HTTP request and enqueue it, but unless
> it blocks polling on the network somewhere then the most it can do in
> terms of network I/O is to send out the initial TCP SYN.  (Not even
> that, if a DNS lookup is required.)

Hi Michael,
To locate TLS protocol from the HTTP handle and configure TLS configuration data at the return from EfiHttpRequest during that short window of non-blocking request is not reliable. It also doesn't make sense to ask upper layer application to do this when it first time invokes EfiHttpRequest.
I already refactored TlsCreateChild to install TLS protocol on HTTP handle. I also implemented the corresponding code in Redfish REST EX to listen the installation of TLS protocol and hook the SetSessionData. It works fine on the system, however I really don’t like having the upper layer application to do this much just for overriding TLS configuration data. The code looked a specific implementation to hack the TLS protocol interface. Plus I still have to add few code in TlsConfigCertificate to skip configure certificate with checking TlsVerifyMethod.
We should sit back to consider introducing a new protocol for upper layer application to provide their own TLS configuration data, as the root cause is that hard coded TLS configuration data in HttpSupport.c. We shouldn't have the code like that and add the burdens to application.

What my thought is as below and maybe more elegant than the patch a sent,
- Still install TLS on HTTP handle, then upper layer application can listen to the installation of EFI TLS protocol to find the correct HTTP handle.
- Move TLS_CONFIG_DATA in a public header file.
- Introduce a new protocol called EDKII_HTTP_TLS_CONFIGURATION_DATA
- Upper layer application installs this protocol with their own TLS_CONFIG_DATA.
- TlsConfigureSession locates EDKII_HTTP_TLS_CONFIGURATION_DATA to replace the default TLS_CONFIG_DATA.

This way we can remove that hardcoded code and fix the root cause, also the upper layer application do not have to take the burden.
What do you think?
Thanks
Abner


>
> The implementation could plausibly construct and enqueue the
> ClientHello, in which case it would be too late to modify the cipher
> suite list, but any attempt to verify the hostname definitely can't
> happen until a lot of network I/O has taken place.


>
> Good luck! :)
>
> Thanks,
>
> Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113000): https://edk2.groups.io/g/devel/message/113000
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

I am going to withdraw this patch set and replaced by https://edk2.groups.io/g/devel/message/113004.

Thanks
Abner

> -----Original Message-----
> From: Chang, Abner
> Sent: Friday, December 29, 2023 11:08 PM
> To: Michael Brown <mcb30@ipxe.org>; devel@edk2.groups.io
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: RE: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
>
>
> > -----Original Message-----
> > From: Michael Brown <mcb30@ipxe.org>
> > Sent: Friday, December 29, 2023 8:01 AM
> > To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> > Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> > <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>;
> Igor
> > Kulchytskyy <igork@ami.com>
> > Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> > policy
> >
> > Caution: This message originated from an External Source. Use proper
> caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > On 28/12/2023 23:37, Chang, Abner via groups.io wrote:
> > >> As far as I am aware, EfiHttpRequest sets up all of the relevant data
> > >> structures but functions as a non-blocking open.  If you reconfigure the
> > >> TLS session immediately after return from EfiHttpRequest() then this
> > >> reconfiguration should take effect before any network packets have been
> > >> transmitted or received.  I have not tested this, though.
> > >>
> > >> If the immediate reconfiguration does not work, then your suggestion of
> > >> hooking SetSessionData() sounds like the easiest approach.
> > > I think the non-blocking transfer still sends out the request but just not
> > waiting the response there, have to check the implementation.
> >
> > The code seems to construct the HTTP request and enqueue it, but unless
> > it blocks polling on the network somewhere then the most it can do in
> > terms of network I/O is to send out the initial TCP SYN.  (Not even
> > that, if a DNS lookup is required.)
>
> Hi Michael,
> To locate TLS protocol from the HTTP handle and configure TLS configuration
> data at the return from EfiHttpRequest during that short window of non-
> blocking request is not reliable. It also doesn't make sense to ask upper layer
> application to do this when it first time invokes EfiHttpRequest.
> I already refactored TlsCreateChild to install TLS protocol on HTTP handle. I
> also implemented the corresponding code in Redfish REST EX to listen the
> installation of TLS protocol and hook the SetSessionData. It works fine on the
> system, however I really don’t like having the upper layer application to do this
> much just for overriding TLS configuration data. The code looked a specific
> implementation to hack the TLS protocol interface. Plus I still have to add few
> code in TlsConfigCertificate to skip configure certificate with checking
> TlsVerifyMethod.
> We should sit back to consider introducing a new protocol for upper layer
> application to provide their own TLS configuration data, as the root cause is
> that hard coded TLS configuration data in HttpSupport.c. We shouldn't have
> the code like that and add the burdens to application.
>
> What my thought is as below and maybe more elegant than the patch a sent,
> - Still install TLS on HTTP handle, then upper layer application can listen to the
> installation of EFI TLS protocol to find the correct HTTP handle.
> - Move TLS_CONFIG_DATA in a public header file.
> - Introduce a new protocol called EDKII_HTTP_TLS_CONFIGURATION_DATA
> - Upper layer application installs this protocol with their own
> TLS_CONFIG_DATA.
> - TlsConfigureSession locates EDKII_HTTP_TLS_CONFIGURATION_DATA to
> replace the default TLS_CONFIG_DATA.
>
> This way we can remove that hardcoded code and fix the root cause, also the
> upper layer application do not have to take the burden.
> What do you think?
> Thanks
> Abner
>
>
> >
> > The implementation could plausibly construct and enqueue the
> > ClientHello, in which case it would be too late to modify the cipher
> > suite list, but any attempt to verify the hostname definitely can't
> > happen until a lot of network I/O has taken place.
>
>
> >
> > Good luck! :)
> >
> > Thanks,
> >
> > Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113010): https://edk2.groups.io/g/devel/message/113010
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 29/12/2023 15:07, Chang, Abner via groups.io wrote:
> To locate TLS protocol from the HTTP handle and configure TLS configuration data at the return from EfiHttpRequest during that short window of non-blocking request is not reliable. It also doesn't make sense to ask upper layer application to do this when it first time invokes EfiHttpRequest.
> I already refactored TlsCreateChild to install TLS protocol on HTTP handle. I also implemented the corresponding code in Redfish REST EX to listen the installation of TLS protocol and hook the SetSessionData. It works fine on the system, however I really don’t like having the upper layer application to do this much just for overriding TLS configuration data. The code looked a specific implementation to hack the TLS protocol interface. Plus I still have to add few code in TlsConfigCertificate to skip configure certificate with checking TlsVerifyMethod.
> We should sit back to consider introducing a new protocol for upper layer application to provide their own TLS configuration data, as the root cause is that hard coded TLS configuration data in HttpSupport.c. We shouldn't have the code like that and add the burdens to application.
> 
> What my thought is as below and maybe more elegant than the patch a sent,
> - Still install TLS on HTTP handle, then upper layer application can listen to the installation of EFI TLS protocol to find the correct HTTP handle.
> - Move TLS_CONFIG_DATA in a public header file.
> - Introduce a new protocol called EDKII_HTTP_TLS_CONFIGURATION_DATA
> - Upper layer application installs this protocol with their own TLS_CONFIG_DATA.
> - TlsConfigureSession locates EDKII_HTTP_TLS_CONFIGURATION_DATA to replace the default TLS_CONFIG_DATA.
> 
> This way we can remove that hardcoded code and fix the root cause, also the upper layer application do not have to take the burden.
> What do you think?

Firstly, thank you very much for taking the time to dig through this and 
work towards a cleaner design - I, for one, really appreciate it.

I think we're completely agreed that installing the TLS protocols on the 
HTTP handle is the right thing to do - that seems to be a clear 
improvement over the status quo where there's no introspectable 
relationship between the two handles.

I'm torn on the use of TLS_CONFIG_DATA.  For better or worse, the 
existing and standardised EFI_TLS_CONFIGURATION_PROTOCOL is verb-based, 
using SetData() and GetData() methods.  Adding a noun-based protocol for 
TLS configuration seems to cut across this, with the potential to look 
confusing: a new reader of the code could legitimately wonder why the 
codebase contains two competing solutions to what is essentially the 
same problem.

Given that the verb-based approach of EFI_TLS_CONFIGURATION_PROTOCOL has 
made it as far as being standardised and included in the UEFI 
specification, I think we probably need to accept that this is the 
"correct" way to perform TLS configuration within UEFI code.  The 
problem with HttpsSupport.c then becomes that there is no good 
opportunity for a consumer to call SetData(), since (a) 
EFI_TLS_CONFIGURATION_PROTOCOL comes into existence only halfway through 
the call to EFI_HTTP_PROTOCOL.Request() and (b) the call to 
TlsConfigureSession() will overwrite the configuration anyway.

Is there a way that TlsConfigureSession() could sensibly provide an 
opportunity for the consumer to make calls to SetData(), so that the 
consumer could cleanly override any default configuration?

Looking through the code, TlsConfigureSession() is called only from 
HttpInitSession(), which in turn is called only from EfiHttpRequest(). 
This call is followed immediately by the line:

   HttpNotify (HttpEventInitSession, Status);

which seems to already use an existing EDKII_HTTP_CALLBACK_PROTOCOL to 
notify an arbitrary list of interested consumers that an event has taken 
place (in this case, that a session has just been initialised).

What do you think about:

- installing TLS on HTTP handle (as you have already implemented)

- using EDKII_HTTP_CALLBACK_PROTOCOL to catch the HttpEventInitSession 
and perform whatever calls are needed to SetData() to modify the TLS 
configuration?

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113015): https://edk2.groups.io/g/devel/message/113015
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Brown via groups.io
> Sent: Tuesday, January 2, 2024 7:07 AM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 29/12/2023 15:07, Chang, Abner via groups.io wrote:
> > To locate TLS protocol from the HTTP handle and configure TLS configuration
> data at the return from EfiHttpRequest during that short window of non-
> blocking request is not reliable. It also doesn't make sense to ask upper layer
> application to do this when it first time invokes EfiHttpRequest.
> > I already refactored TlsCreateChild to install TLS protocol on HTTP handle. I
> also implemented the corresponding code in Redfish REST EX to listen the
> installation of TLS protocol and hook the SetSessionData. It works fine on the
> system, however I really don’t like having the upper layer application to do this
> much just for overriding TLS configuration data. The code looked a specific
> implementation to hack the TLS protocol interface. Plus I still have to add few
> code in TlsConfigCertificate to skip configure certificate with checking
> TlsVerifyMethod.
> > We should sit back to consider introducing a new protocol for upper layer
> application to provide their own TLS configuration data, as the root cause is
> that hard coded TLS configuration data in HttpSupport.c. We shouldn't have
> the code like that and add the burdens to application.
> >
> > What my thought is as below and maybe more elegant than the patch a
> sent,
> > - Still install TLS on HTTP handle, then upper layer application can listen to
> the installation of EFI TLS protocol to find the correct HTTP handle.
> > - Move TLS_CONFIG_DATA in a public header file.
> > - Introduce a new protocol called EDKII_HTTP_TLS_CONFIGURATION_DATA
> > - Upper layer application installs this protocol with their own
> TLS_CONFIG_DATA.
> > - TlsConfigureSession locates EDKII_HTTP_TLS_CONFIGURATION_DATA to
> replace the default TLS_CONFIG_DATA.
> >
> > This way we can remove that hardcoded code and fix the root cause, also the
> upper layer application do not have to take the burden.
> > What do you think?
>
> Firstly, thank you very much for taking the time to dig through this and
> work towards a cleaner design - I, for one, really appreciate it.
Thank you.

>
> I think we're completely agreed that installing the TLS protocols on the
> HTTP handle is the right thing to do - that seems to be a clear
> improvement over the status quo where there's no introspectable
> relationship between the two handles.
>
> I'm torn on the use of TLS_CONFIG_DATA.  For better or worse, the
> existing and standardised EFI_TLS_CONFIGURATION_PROTOCOL is verb-
> based,
> using SetData() and GetData() methods.  Adding a noun-based protocol for
> TLS configuration seems to cut across this, with the potential to look
> confusing: a new reader of the code could legitimately wonder why the
> codebase contains two competing solutions to what is essentially the
> same problem.
>
> Given that the verb-based approach of EFI_TLS_CONFIGURATION_PROTOCOL
> has
> made it as far as being standardised and included in the UEFI
> specification, I think we probably need to accept that this is the
> "correct" way to perform TLS configuration within UEFI code.  The
> problem with HttpsSupport.c then becomes that there is no good
> opportunity for a consumer to call SetData(), since (a)
> EFI_TLS_CONFIGURATION_PROTOCOL comes into existence only halfway
> through
> the call to EFI_HTTP_PROTOCOL.Request() and (b) the call to
> TlsConfigureSession() will overwrite the configuration anyway.
>
> Is there a way that TlsConfigureSession() could sensibly provide an
> opportunity for the consumer to make calls to SetData(), so that the
> consumer could cleanly override any default configuration?
>
> Looking through the code, TlsConfigureSession() is called only from
> HttpInitSession(), which in turn is called only from EfiHttpRequest().
> This call is followed immediately by the line:
>
>    HttpNotify (HttpEventInitSession, Status);
>
> which seems to already use an existing EDKII_HTTP_CALLBACK_PROTOCOL to
> notify an arbitrary list of interested consumers that an event has taken
> place (in this case, that a session has just been initialised).
>
> What do you think about:
>
> - installing TLS on HTTP handle (as you have already implemented)
>
> - using EDKII_HTTP_CALLBACK_PROTOCOL to catch the HttpEventInitSession
> and perform whatever calls are needed to SetData() to modify the TLS
> configuration?

Leverage HttpNotify is good but still has some problems, as HttpNotify is designed to notify callback owner about a specific task was done. In order to keep this HttpNotify nature, we can create a callback point at the end of TlsCreateChild() with a newly introduced event type says HttpEventTlsChildCreated. The reason we have to create this notification before TlsConfigureSession() is because this function uses the default configuration data to configure TLS. However, it doesn't have to do EfiTlsVerifyHost and TlsConfigCertificate if there is nothing to verify.
The problem in configuring  EfiTlsVerifyHost is It always checks verification method with EFI_TLS_VERIFY_PEER, while the problem of TlsConfigCertificate is it considers platform always can provide the certificate.  Anyway to configure TLS after TlsConfigCertificate is to late as the error status already returned earlier. Furthermore the design of HttpNotify doesn't provide the output information for caller to determine the different code paths.  So with above, how can we skip configuring TLS again with the default values in HttpSupport.c even platform code already configured it before TlsConfigureSession()?

Another approach is to introduced HttpsTlsConfigureSession notification, callback routine may receive the error in EventStatus when the first time TlsConfigureSession is invoked. In the failure case, the callback function calls Tls->SetSessionData to configure its own TLS configuration data. I do a quick try to set the different configuration after the first time failure. It works fine and seems the TLS library internal state machine is not messed up.  However, it looks not quite intuitive to the upper layer HTTP application. Why upper layer HTTP application has to reconfigure TLS when HttpsTlsConfigureSession is notified with an EFI error status? This approach looks okey to me, acceptable but not perfect. Says this approach is fine to me with a detail comment above the new event type in C header file.
Any better suggestion?

Thanks
Abner

>
> Thanks,
>
> Michael
>
>
>
> 
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113028): https://edk2.groups.io/g/devel/message/113028
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 02/01/2024 06:06, Chang, Abner via groups.io wrote:
>> What do you think about:
>>
>> - installing TLS on HTTP handle (as you have already implemented)
>>
>> - using EDKII_HTTP_CALLBACK_PROTOCOL to catch the HttpEventInitSession
>> and perform whatever calls are needed to SetData() to modify the TLS
>> configuration?
> 
> Leverage HttpNotify is good but still has some problems, as HttpNotify is designed to notify callback owner about a specific task was done. In order to keep this HttpNotify nature, we can create a callback point at the end of TlsCreateChild() with a newly introduced event type says HttpEventTlsChildCreated. The reason we have to create this notification before TlsConfigureSession() is because this function uses the default configuration data to configure TLS. However, it doesn't have to do EfiTlsVerifyHost and TlsConfigCertificate if there is nothing to verify.
> The problem in configuring  EfiTlsVerifyHost is It always checks verification method with EFI_TLS_VERIFY_PEER, while the problem of TlsConfigCertificate is it considers platform always can provide the certificate.  Anyway to configure TLS after TlsConfigCertificate is to late as the error status already returned earlier. Furthermore the design of HttpNotify doesn't provide the output information for caller to determine the different code paths.  So with above, how can we skip configuring TLS again with the default values in HttpSupport.c even platform code already configured it before TlsConfigureSession()?

I may not have been clear enough: I'm suggesting that we allow 
TlsConfigureSession() to perform its normal configuration, and then use 
the HttpEventInitSession callback to modify this configuration (e.g. 
setting EFI_TLS_VERIFY_NONE).

Yes, this would mean that a tiny amount of unnecessary work is done 
(e.g. first setting EFI_TLS_VERIFY_PEER, then later changing it to 
EFI_TLS_VERIFY_NONE) but this is a negligible amount of execution time 
and allows us to keep the code simple.

The HttpEventInitSession callback is guaranteed to be called before the 
calls to HttpGenRequestMessage() and HttpTransmitTcp() (even if running 
at TPL_APPLICATION with network polling taking place) and so is 
guaranteed to be a safe point at which to perform additional TLS 
configuration via SetData().

So, to expand on what I wrote before, I'm suggesting:

- refactor TlsCreateChild() to install the TLS protocols on the HTTP 
handle (as you have already implemented).

- (optional) Remove TlsChildHandle and replace with a boolean flag.

- No further changes required to HttpDxe, as far as I can tell.

- In RedfishRestExDxe, install an EDKII_HTTP_CALLBACK_PROTOCOL before 
calling EFI_HTTP_PROTOCOL.Request().

- Allow the call to Request() to perform its normal TLS configuration 
via TlsConfigureSession(), as though the connection were going to 
perform host verification etc as per the platform default policy.  This 
configuration should succeed, with no error returned.

- In the RedfishRestExDxe callback, check for HttpEventInitSession and 
use calls to EFI_TLS_CONFIGURATION_PROTOCOL.SetData() to modify the TLS 
configuration to e.g. set EFI_TLS_VERIFY_NONE.

To make the callback implementation easier, you may want to extend 
HttpNotify() to take HTTP_PROTOCOL *HttpInstance as its first parameter, 
and then pass HttpInstance->Handle as an additional parameter to the 
callback method, i.e.:

typedef
VOID
(EFIAPI *EDKII_HTTP_CALLBACK)(
   IN EDKII_HTTP_CALLBACK_PROTOCOL     *This,
   IN EFI_HANDLE                       HttpHandle,
   IN EDKII_HTTP_CALLBACK_EVENT        Event,
   IN EFI_STATUS                       EventStatus
   );

VOID
HttpNotify (
   IN  HTTP_PROTOCOL              *HttpInstance,
   IN  EDKII_HTTP_CALLBACK_EVENT  Event,
   IN  EFI_STATUS                 EventStatus
   )
{
   ...
   HttpCallback->Callback (
                         HttpCallback,
                         HttpInstance->Handle,
                         Event,
                         EventStatus
                         );
   ...
}

Since EDKII_HTTP_CALLBACK_PROTOCOL is internal to EDK2 (and not covered 
by the UEFI specification), this change should be possible.  I've 
checked all of the HttpNotify() call sites in the EDK2 repo, and they do 
all have an HttpInstance available to use.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113033): https://edk2.groups.io/g/devel/message/113033
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Michael Brown <mcb30@ipxe.org>
> Sent: Tuesday, January 2, 2024 8:42 PM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 02/01/2024 06:06, Chang, Abner via groups.io wrote:
> >> What do you think about:
> >>
> >> - installing TLS on HTTP handle (as you have already implemented)
> >>
> >> - using EDKII_HTTP_CALLBACK_PROTOCOL to catch the
> HttpEventInitSession
> >> and perform whatever calls are needed to SetData() to modify the TLS
> >> configuration?
> >
> > Leverage HttpNotify is good but still has some problems, as HttpNotify is
> designed to notify callback owner about a specific task was done. In order to
> keep this HttpNotify nature, we can create a callback point at the end of
> TlsCreateChild() with a newly introduced event type says
> HttpEventTlsChildCreated. The reason we have to create this notification
> before TlsConfigureSession() is because this function uses the default
> configuration data to configure TLS. However, it doesn't have to do
> EfiTlsVerifyHost and TlsConfigCertificate if there is nothing to verify.
> > The problem in configuring  EfiTlsVerifyHost is It always checks verification
> method with EFI_TLS_VERIFY_PEER, while the problem of TlsConfigCertificate
> is it considers platform always can provide the certificate.  Anyway to
> configure TLS after TlsConfigCertificate is to late as the error status already
> returned earlier. Furthermore the design of HttpNotify doesn't provide the
> output information for caller to determine the different code paths.  So with
> above, how can we skip configuring TLS again with the default values in
> HttpSupport.c even platform code already configured it before
> TlsConfigureSession()?
>
> I may not have been clear enough: I'm suggesting that we allow
> TlsConfigureSession() to perform its normal configuration, and then use
> the HttpEventInitSession callback to modify this configuration (e.g.
> setting EFI_TLS_VERIFY_NONE).
>
> Yes, this would mean that a tiny amount of unnecessary work is done
> (e.g. first setting EFI_TLS_VERIFY_PEER, then later changing it to
> EFI_TLS_VERIFY_NONE) but this is a negligible amount of execution time
> and allows us to keep the code simple.
>
> The HttpEventInitSession callback is guaranteed to be called before the
> calls to HttpGenRequestMessage() and HttpTransmitTcp() (even if running
> at TPL_APPLICATION with network polling taking place) and so is
> guaranteed to be a safe point at which to perform additional TLS
> configuration via SetData().
>
> So, to expand on what I wrote before, I'm suggesting:
>
> - refactor TlsCreateChild() to install the TLS protocols on the HTTP
> handle (as you have already implemented).
>
> - (optional) Remove TlsChildHandle and replace with a boolean flag.
>
Hi Michael,
Above has no problems.


> - No further changes required to HttpDxe, as far as I can tell.
>
> - In RedfishRestExDxe, install an EDKII_HTTP_CALLBACK_PROTOCOL before
> calling EFI_HTTP_PROTOCOL.Request().
>
> - Allow the call to Request() to perform its normal TLS configuration
> via TlsConfigureSession(), as though the connection were going to
> perform host verification etc as per the platform default policy.  This
> configuration should succeed, with no error returned.

This is not correct. The first Request would be failed at TlsConfigureCertificate here https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpsSupport.c#L711 and  SetSessionData to EfiTlsVerifyHost here: https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpsSupport.c#L679.
The default values is the key issue we are trying to fix in the past week, the root cause is because the hardcoded default value.

>
> - In the RedfishRestExDxe callback, check for HttpEventInitSession and
> use calls to EFI_TLS_CONFIGURATION_PROTOCOL.SetData() to modify the TLS
> configuration to e.g. set EFI_TLS_VERIFY_NONE.
Here is the thing. Even we reconfigure TLS configuration data at HttpEventInitSession in RestEx, the EfiHttpRequest still returns fail to the caller here: https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpImpl.c#L599. Not to mention the reason of failures may not be caused by TlsConfigureSession. There are failures for some other reasons in HttpInitSession. Also, what the caller suppose to do when it gets error returned? How does caller knows the error is just because the TLS configuration failure and it has to reconfigure TLS and retry HttpRequest? The logic doesn’t make sense if the caller assumes the failure is caused by TLS configure at HttpEventInitSession callback. Actually, having a high layer application to reconfigure TLS configuration data because the failure caused by not well-considered default TLS config values also doesn’t make sense, right?
But the compromise solution is to introduce HttpTlsConfigured callback event right after TlsConfigureSession here https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpProto.c#L1424. When upper layer HTTP application gets the error at this callback, reconfigure TLS configuration data and retry httpRequest, not perfect but kind of acceptable in logic point.

>
> To make the callback implementation easier, you may want to extend
> HttpNotify() to take HTTP_PROTOCOL *HttpInstance as its first parameter,
> and then pass HttpInstance->Handle as an additional parameter to the
> callback method, i.e.:
>
> typedef
> VOID
> (EFIAPI *EDKII_HTTP_CALLBACK)(
>    IN EDKII_HTTP_CALLBACK_PROTOCOL     *This,
>    IN EFI_HANDLE                       HttpHandle,
>    IN EDKII_HTTP_CALLBACK_EVENT        Event,
>    IN EFI_STATUS                       EventStatus
>    );
We shouldn’t change the prototype as the callback mechanism may used by OEM/ODM platform code which is not part of tianocore. This change breaks backward compatible. Honestly, leverage HTTP callback function doesn’t really serve the purpose well. This is the HttpDxe design defect as we don’t the used case like in-band Redfish communication.

Thanks
Abner

>
> VOID
> HttpNotify (
>    IN  HTTP_PROTOCOL              *HttpInstance,
>    IN  EDKII_HTTP_CALLBACK_EVENT  Event,
>    IN  EFI_STATUS                 EventStatus
>    )
> {
>    ...
>    HttpCallback->Callback (
>                          HttpCallback,
>                          HttpInstance->Handle,
>                          Event,
>                          EventStatus
>                          );
>    ...
> }
>
> Since EDKII_HTTP_CALLBACK_PROTOCOL is internal to EDK2 (and not covered
> by the UEFI specification), this change should be possible.  I've
> checked all of the HttpNotify() call sites in the EDK2 repo, and they do
> all have an HttpInstance available to use.
>
> Thanks,
>
> Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113038): https://edk2.groups.io/g/devel/message/113038
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 02/01/2024 16:31, Chang, Abner via groups.io wrote:
>> From: Michael Brown <mcb30@ipxe.org>
>> - Allow the call to Request() to perform its normal TLS configuration
>> via TlsConfigureSession(), as though the connection were going to
>> perform host verification etc as per the platform default policy.  This
>> configuration should succeed, with no error returned.
> 
> This is not correct. The first Request would be failed at TlsConfigureCertificate here https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpsSupport.c#L711

I assume this is because we expect that the TlsCaCertificate variable 
may be empty or non-existent?

Would it make sense to have an EFI_NOT_FOUND from TlsConfigCertificate() 
be ignored, as is already done for TlsConfigCipherList() just above? 
Something like:

   Status = TlsConfigCertificate (HttpInstance);
   if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
     DEBUG ((DEBUG_ERROR, "TLS Certificate Config Error!\n"));
     return Status;
   }

i.e. treat an absent TlsCaCertificate variable as meaning "there are no 
explicit CA certificates", allowing TlsDxe to do whatever it does in 
that situation (which is presumably to fail any attempted certificate 
verifications).

That would eliminate this problem in the RedfishRestExDxe case.

> and  SetSessionData to EfiTlsVerifyHost here: https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpsSupport.c#L679.

I don't understand why this call would be expected to fail.  It's not 
performing any verification at this stage, just recording the hostname 
from the URI for subsequent use in certificate verification.

I would expect this call to succeed and record whatever hostname is 
present in the request from RedfishRestExDxe.  This hostname will 
subsequently be ignored for verification, since the HttpEventInitSession 
callback will set EFI_TLS_VERIFY_NONE.

>> - In the RedfishRestExDxe callback, check for HttpEventInitSession and
>> use calls to EFI_TLS_CONFIGURATION_PROTOCOL.SetData() to modify the TLS
>> configuration to e.g. set EFI_TLS_VERIFY_NONE.
> Here is the thing. Even we reconfigure TLS configuration data at HttpEventInitSession in RestEx, the EfiHttpRequest still returns fail to the caller here: https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/HttpImpl.c#L599. Not to mention the reason of failures may not be caused by TlsConfigureSession. There are failures for some other reasons in HttpInitSession. Also, what the caller suppose to do when it gets error returned? How does caller knows the error is just because the TLS configuration failure and it has to reconfigure TLS and retry HttpRequest? The logic doesn’t make sense if the caller assumes the failure is caused by TLS configure at HttpEventInitSession callback. Actually, having a high layer application to reconfigure TLS configuration data because the failure caused by not well-considered default TLS config values also doesn’t make sense, right?

I would expect this to be resolved by the above suggestions. 
HttpInitSession() should succeed.  The HttpEventInitSession callback 
should be called with an EFI_SUCCESS status code, and there is no need 
for the caller to retry anything.

>> To make the callback implementation easier, you may want to extend
>> HttpNotify() to take HTTP_PROTOCOL *HttpInstance as its first parameter,
>> and then pass HttpInstance->Handle as an additional parameter to the
>> callback method, i.e.:
>>
>> typedef
>> VOID
>> (EFIAPI *EDKII_HTTP_CALLBACK)(
>>     IN EDKII_HTTP_CALLBACK_PROTOCOL     *This,
>>     IN EFI_HANDLE                       HttpHandle,
>>     IN EDKII_HTTP_CALLBACK_EVENT        Event,
>>     IN EFI_STATUS                       EventStatus
>>     );
> We shouldn’t change the prototype as the callback mechanism may used by OEM/ODM platform code which is not part of tianocore. This change breaks backward compatible. Honestly, leverage HTTP callback function doesn’t really serve the purpose well. This is the HttpDxe design defect as we don’t the used case like in-band Redfish communication.

OEM/ODM code should restrict itself to using APIs covered by the UEFI 
specification.  If OEM/ODMs choose to rely on EDK2 private 
implementation details (such as EDKII_HTTP_CALLBACK) then they must be 
prepared to update their code when the private implementation detail 
changes.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113040): https://edk2.groups.io/g/devel/message/113040
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Brown via groups.io
> Sent: Wednesday, January 3, 2024 1:47 AM
> To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On 02/01/2024 16:31, Chang, Abner via groups.io wrote:
> >> From: Michael Brown <mcb30@ipxe.org>
> >> - Allow the call to Request() to perform its normal TLS configuration
> >> via TlsConfigureSession(), as though the connection were going to
> >> perform host verification etc as per the platform default policy.  This
> >> configuration should succeed, with no error returned.
> >
> > This is not correct. The first Request would be failed at
> TlsConfigureCertificate here
> https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/Http
> sSupport.c#L711
>
> I assume this is because we expect that the TlsCaCertificate variable
> may be empty or non-existent?
Yes.

>
> Would it make sense to have an EFI_NOT_FOUND from TlsConfigCertificate()
> be ignored,
Why not 😊,  if some code needs to be changed to fix the hardcoded values issues.  I don’t see this change harms anything.

> as is already done for TlsConfigCipherList() just above?
Not sure what is about "this is already done for TlsConfigCipherList". But anyway, we can treat EFI_NOT_FOUND as success and push back the error to TLS.

> Something like:
>
>    Status = TlsConfigCertificate (HttpInstance);
>    if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
>      DEBUG ((DEBUG_ERROR, "TLS Certificate Config Error!\n"));
>      return Status;
>    }
>
> i.e. treat an absent TlsCaCertificate variable as meaning "there are no
> explicit CA certificates", allowing TlsDxe to do whatever it does in
> that situation (which is presumably to fail any attempted certificate
> verifications).
>
> That would eliminate this problem in the RedfishRestExDxe case.
>
> > and  SetSessionData to EfiTlsVerifyHost here:
> https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/Http
> sSupport.c#L679.
>
> I don't understand why this call would be expected to fail.  It's not
> performing any verification at this stage, just recording the hostname
> from the URI for subsequent use in certificate verification.
This is my mistake as I changed the code flow of default TLS config data initialization, please just ignore this one.

Thanks
Abner

>
> I would expect this call to succeed and record whatever hostname is
> present in the request from RedfishRestExDxe.  This hostname will
> subsequently be ignored for verification, since the HttpEventInitSession
> callback will set EFI_TLS_VERIFY_NONE.
>
> >> - In the RedfishRestExDxe callback, check for HttpEventInitSession and
> >> use calls to EFI_TLS_CONFIGURATION_PROTOCOL.SetData() to modify the
> TLS
> >> configuration to e.g. set EFI_TLS_VERIFY_NONE.
> > Here is the thing. Even we reconfigure TLS configuration data at
> HttpEventInitSession in RestEx, the EfiHttpRequest still returns fail to the caller
> here:
> https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/Http
> Impl.c#L599. Not to mention the reason of failures may not be caused by
> TlsConfigureSession. There are failures for some other reasons in
> HttpInitSession. Also, what the caller suppose to do when it gets error
> returned? How does caller knows the error is just because the TLS
> configuration failure and it has to reconfigure TLS and retry HttpRequest? The
> logic doesn’t make sense if the caller assumes the failure is caused by TLS
> configure at HttpEventInitSession callback. Actually, having a high layer
> application to reconfigure TLS configuration data because the failure caused by
> not well-considered default TLS config values also doesn’t make sense, right?
>
> I would expect this to be resolved by the above suggestions.
> HttpInitSession() should succeed.  The HttpEventInitSession callback
> should be called with an EFI_SUCCESS status code, and there is no need
> for the caller to retry anything.
>
> >> To make the callback implementation easier, you may want to extend
> >> HttpNotify() to take HTTP_PROTOCOL *HttpInstance as its first parameter,
> >> and then pass HttpInstance->Handle as an additional parameter to the
> >> callback method, i.e.:
> >>
> >> typedef
> >> VOID
> >> (EFIAPI *EDKII_HTTP_CALLBACK)(
> >>     IN EDKII_HTTP_CALLBACK_PROTOCOL     *This,
> >>     IN EFI_HANDLE                       HttpHandle,
> >>     IN EDKII_HTTP_CALLBACK_EVENT        Event,
> >>     IN EFI_STATUS                       EventStatus
> >>     );
> > We shouldn’t change the prototype as the callback mechanism may used by
> OEM/ODM platform code which is not part of tianocore. This change breaks
> backward compatible. Honestly, leverage HTTP callback function doesn’t really
> serve the purpose well. This is the HttpDxe design defect as we don’t the used
> case like in-band Redfish communication.
>
> OEM/ODM code should restrict itself to using APIs covered by the UEFI
> specification.  If OEM/ODMs choose to rely on EDK2 private
> implementation details (such as EDKII_HTTP_CALLBACK) then they must be
> prepared to update their code when the private implementation detail
> changes.
> Thanks,
>
> Michael
>
>
>
> 
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113129): https://edk2.groups.io/g/devel/message/113129
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Chang, Abner via groups.io]

[AMD Official Use Only - General]

Hi Michael,
We are not aware there is a TlsConnectSession() for TLS handshake using the default TLS configuration data and it returns a failure as expected because the default TLS configuration is TLS_VERIFY_HOST without certificate installed on system.
This happens in HttpInitSession before notifying HttpEventInitSession event,  so we have to reconfigure TLS config data before TlsConnectSession() function.
As there is an existing HttpEventTlsConnectSession event notified after TlsConnectSession(), that makes sense to me to introduce a new HTTP event HttpEventTlsConfigured as I mentioned in previous conversation and notify callback functions after TlsConfigureSession().
Upper layer HTTP application then listen to HttpEventTlsConfigured event and reconfigure TLS configuration data in the callback function.

Please check it here: https://edk2.groups.io/g/devel/message/113224

Thanks
Abner
> -----Original Message-----
> From: Chang, Abner
> Sent: Wednesday, January 3, 2024 12:32 AM
> To: Michael Brown <mcb30@ipxe.org>; devel@edk2.groups.io
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>; Igor
> Kulchytskyy <igork@ami.com>
> Subject: RE: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> policy
>
>
>
> > -----Original Message-----
> > From: Michael Brown <mcb30@ipxe.org>
> > Sent: Tuesday, January 2, 2024 8:42 PM
> > To: devel@edk2.groups.io; Chang, Abner <Abner.Chang@amd.com>
> > Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams
> > <zachary.clark-williams@intel.com>; Nickle Wang <nicklew@nvidia.com>;
> Igor
> > Kulchytskyy <igork@ami.com>
> > Subject: Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS
> > policy
> >
> > Caution: This message originated from an External Source. Use proper
> caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > On 02/01/2024 06:06, Chang, Abner via groups.io wrote:
> > >> What do you think about:
> > >>
> > >> - installing TLS on HTTP handle (as you have already implemented)
> > >>
> > >> - using EDKII_HTTP_CALLBACK_PROTOCOL to catch the
> > HttpEventInitSession
> > >> and perform whatever calls are needed to SetData() to modify the TLS
> > >> configuration?
> > >
> > > Leverage HttpNotify is good but still has some problems, as HttpNotify is
> > designed to notify callback owner about a specific task was done. In order to
> > keep this HttpNotify nature, we can create a callback point at the end of
> > TlsCreateChild() with a newly introduced event type says
> > HttpEventTlsChildCreated. The reason we have to create this notification
> > before TlsConfigureSession() is because this function uses the default
> > configuration data to configure TLS. However, it doesn't have to do
> > EfiTlsVerifyHost and TlsConfigCertificate if there is nothing to verify.
> > > The problem in configuring  EfiTlsVerifyHost is It always checks verification
> > method with EFI_TLS_VERIFY_PEER, while the problem of
> TlsConfigCertificate
> > is it considers platform always can provide the certificate.  Anyway to
> > configure TLS after TlsConfigCertificate is to late as the error status already
> > returned earlier. Furthermore the design of HttpNotify doesn't provide the
> > output information for caller to determine the different code paths.  So with
> > above, how can we skip configuring TLS again with the default values in
> > HttpSupport.c even platform code already configured it before
> > TlsConfigureSession()?
> >
> > I may not have been clear enough: I'm suggesting that we allow
> > TlsConfigureSession() to perform its normal configuration, and then use
> > the HttpEventInitSession callback to modify this configuration (e.g.
> > setting EFI_TLS_VERIFY_NONE).
> >
> > Yes, this would mean that a tiny amount of unnecessary work is done
> > (e.g. first setting EFI_TLS_VERIFY_PEER, then later changing it to
> > EFI_TLS_VERIFY_NONE) but this is a negligible amount of execution time
> > and allows us to keep the code simple.
> >
> > The HttpEventInitSession callback is guaranteed to be called before the
> > calls to HttpGenRequestMessage() and HttpTransmitTcp() (even if running
> > at TPL_APPLICATION with network polling taking place) and so is
> > guaranteed to be a safe point at which to perform additional TLS
> > configuration via SetData().
> >
> > So, to expand on what I wrote before, I'm suggesting:
> >
> > - refactor TlsCreateChild() to install the TLS protocols on the HTTP
> > handle (as you have already implemented).
> >
> > - (optional) Remove TlsChildHandle and replace with a boolean flag.
> >
> Hi Michael,
> Above has no problems.
>
>
> > - No further changes required to HttpDxe, as far as I can tell.
> >
> > - In RedfishRestExDxe, install an EDKII_HTTP_CALLBACK_PROTOCOL before
> > calling EFI_HTTP_PROTOCOL.Request().
> >
> > - Allow the call to Request() to perform its normal TLS configuration
> > via TlsConfigureSession(), as though the connection were going to
> > perform host verification etc as per the platform default policy.  This
> > configuration should succeed, with no error returned.
>
> This is not correct. The first Request would be failed at TlsConfigureCertificate
> here
> https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/Http
> sSupport.c#L711 and  SetSessionData to EfiTlsVerifyHost here:
> https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/Http
> sSupport.c#L679.
> The default values is the key issue we are trying to fix in the past week, the root
> cause is because the hardcoded default value.
>
> >
> > - In the RedfishRestExDxe callback, check for HttpEventInitSession and
> > use calls to EFI_TLS_CONFIGURATION_PROTOCOL.SetData() to modify the
> TLS
> > configuration to e.g. set EFI_TLS_VERIFY_NONE.
> Here is the thing. Even we reconfigure TLS configuration data at
> HttpEventInitSession in RestEx, the EfiHttpRequest still returns fail to the caller
> here:
> https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/Http
> Impl.c#L599. Not to mention the reason of failures may not be caused by
> TlsConfigureSession. There are failures for some other reasons in
> HttpInitSession. Also, what the caller suppose to do when it gets error
> returned? How does caller knows the error is just because the TLS
> configuration failure and it has to reconfigure TLS and retry HttpRequest? The
> logic doesn’t make sense if the caller assumes the failure is caused by TLS
> configure at HttpEventInitSession callback. Actually, having a high layer
> application to reconfigure TLS configuration data because the failure caused by
> not well-considered default TLS config values also doesn’t make sense, right?
> But the compromise solution is to introduce HttpTlsConfigured callback event
> right after TlsConfigureSession here
> https://github.com/tianocore/edk2/blob/master/NetworkPkg/HttpDxe/Http
> Proto.c#L1424. When upper layer HTTP application gets the error at this
> callback, reconfigure TLS configuration data and retry httpRequest, not perfect
> but kind of acceptable in logic point.
>
> >
> > To make the callback implementation easier, you may want to extend
> > HttpNotify() to take HTTP_PROTOCOL *HttpInstance as its first parameter,
> > and then pass HttpInstance->Handle as an additional parameter to the
> > callback method, i.e.:
> >
> > typedef
> > VOID
> > (EFIAPI *EDKII_HTTP_CALLBACK)(
> >    IN EDKII_HTTP_CALLBACK_PROTOCOL     *This,
> >    IN EFI_HANDLE                       HttpHandle,
> >    IN EDKII_HTTP_CALLBACK_EVENT        Event,
> >    IN EFI_STATUS                       EventStatus
> >    );
> We shouldn’t change the prototype as the callback mechanism may used by
> OEM/ODM platform code which is not part of tianocore. This change breaks
> backward compatible. Honestly, leverage HTTP callback function doesn’t really
> serve the purpose well. This is the HttpDxe design defect as we don’t the used
> case like in-band Redfish communication.
>
> Thanks
> Abner
>
> >
> > VOID
> > HttpNotify (
> >    IN  HTTP_PROTOCOL              *HttpInstance,
> >    IN  EDKII_HTTP_CALLBACK_EVENT  Event,
> >    IN  EFI_STATUS                 EventStatus
> >    )
> > {
> >    ...
> >    HttpCallback->Callback (
> >                          HttpCallback,
> >                          HttpInstance->Handle,
> >                          Event,
> >                          EventStatus
> >                          );
> >    ...
> > }
> >
> > Since EDKII_HTTP_CALLBACK_PROTOCOL is internal to EDK2 (and not
> covered
> > by the UEFI specification), this change should be possible.  I've
> > checked all of the HttpNotify() call sites in the EDK2 repo, and they do
> > all have an HttpInstance available to use.
> >
> > Thanks,
> >
> > Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113230): https://edk2.groups.io/g/devel/message/113230
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy

[Michael Brown]

On 05/01/2024 08:41, Chang, Abner wrote:
> We are not aware there is a TlsConnectSession() for TLS handshake using the default TLS configuration data and it returns a failure as expected because the default TLS configuration is TLS_VERIFY_HOST without certificate installed on system.
> This happens in HttpInitSession before notifying HttpEventInitSession event,  so we have to reconfigure TLS config data before TlsConnectSession() function.
> As there is an existing HttpEventTlsConnectSession event notified after TlsConnectSession(), that makes sense to me to introduce a new HTTP event HttpEventTlsConfigured as I mentioned in previous conversation and notify callback functions after TlsConfigureSession().
> Upper layer HTTP application then listen to HttpEventTlsConfigured event and reconfigure TLS configuration data in the callback function.

Sounds good to me.  Thank you for the improvements.  I think this design 
is now ready.

Thanks,

Michael



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113312): https://edk2.groups.io/g/devel/message/113312
Mute This Topic: https://groups.io/mt/103368438/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-