public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael Brown" <mcb30@ipxe.org>
To: devel@edk2.groups.io, 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
Date: Tue, 2 Jan 2024 12:42:16 +0000	[thread overview]
Message-ID: <0102018cca323415-72e68e82-2db0-4821-897e-8eff33fbe586-000000@eu-west-1.amazonses.com> (raw)
In-Reply-To: <MN2PR12MB39664F99340EF3AA29886FD7EA61A@MN2PR12MB3966.namprd12.prod.outlook.com>

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.

- 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.

- 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.

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
   );

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 (#113033): https://edk2.groups.io/g/devel/message/113033
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]
-=-=-=-=-=-=-=-=-=-=-=-



  reply	other threads:[~2024-01-02 12:42 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-26 11:28 [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform TLS policy Chang, Abner via groups.io
2023-12-26 11:28 ` [edk2-devel] [RFC][PATCH 1/2] NetworkPkg: EDKII HTTPS platform " Chang, Abner via groups.io
2023-12-26 11:28 ` [edk2-devel] [RFC][PATCH 2/2] NetworkPkg: Check " Chang, Abner via groups.io
2023-12-27 15:55 ` [edk2-devel] [RFC][PATCH 0/2] Introduce HTTPS Platform " Michael Brown
2023-12-28  2:47   ` Chang, Abner via groups.io
2023-12-28 14:16     ` Michael Brown
2023-12-28 15:04       ` Chang, Abner via groups.io
2023-12-28 15:31         ` Michael Brown
2023-12-28 23:37           ` Chang, Abner via groups.io
2023-12-29  0:01             ` Michael Brown
2023-12-29 15:07               ` Chang, Abner via groups.io
2023-12-30 11:31                 ` Chang, Abner via groups.io
2024-01-01 23:07                 ` Michael Brown
2024-01-02  6:06                   ` Chang, Abner via groups.io
2024-01-02 12:42                     ` Michael Brown [this message]
2024-01-02 16:31                       ` Chang, Abner via groups.io
2024-01-02 17:46                         ` Michael Brown
2024-01-04  3:13                           ` Chang, Abner via groups.io
2024-01-05  8:41                         ` Chang, Abner via groups.io
2024-01-05 17:16                           ` Michael Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0102018cca323415-72e68e82-2db0-4821-897e-8eff33fbe586-000000@eu-west-1.amazonses.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox