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]

> -----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]
-=-=-=-=-=-=-=-=-=-=-=-