On 02/01/2024 16:31, Chang, Abner via groups.io wrote:
From: Michael Brown <mc...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
