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

["Michael Brown" <mcb30@ipxe.org>]

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