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

["Chang, Abner via groups.io" <abner.chang=amd.com@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]
-=-=-=-=-=-=-=-=-=-=-=-