[AMD Official Use Only - General] > -----Original Message----- > From: Michael Brown <mc...@ipxe.org> > Sent: Tuesday, January 2, 2024 8:42 PM > To: devel@edk2.groups.io; Chang, Abner <abner.ch...@amd.com> > Cc: Saloni Kasbekar <saloni.kasbe...@intel.com>; Zachary Clark-williams > <zachary.clark-willi...@intel.com>; Nickle Wang <nick...@nvidia.com>; Igor > Kulchytskyy <ig...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
