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