Re: [edk2-devel] [edk2][PATCH 1/1] NetworkPkg/HttpBootDxe: handle servers which may FIN after file sizing in HttpBootLoadFile

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 13687 bytes --]

I wanted to know whether the manual change to Python's http.server I 
have proposed worked for you. Did it?

Patch you have proposed, while being functional, is still a work-around. 
HttpBootDxe supports HTTP/1.1 with an assumption of TCP connections 
being persistent. Python HTTP server implementation does not support 
HTTP/1.1 by default but does contain support for it. Plus, changing it 
to operate on HTTP/1.1 is trivial and from what code suggests - it 
should work just fine.

If we were to discuss how to tackle the issue on HTTP client side, we 
have two potential options:
1. Detect HTTP version in HTTP response from server and add alternate 
path to the whole HTTP I/O if the version reported by the server is 
lower than 1.1. Large change to HttpBootDxe.

2. Add 'keep-alive' header to HTTP requests. I see two problems here:
a. Python server support does seem to contain proper support for that 
header when self.protocol_version == "HTTP/1.0". That will not solve the 
problem in your use case.
b. I have no idea whether other HTTP server implementations will reject 
the request altogether based on that 'keep-alive' header (HTTP/2.x case).

I will not opt for any of them. Adding support for pre-1997 features is 
something I would like to avoid.

Nevertheless, subject of informing the user about lack of support for 
HTTP/1.1 on server-side still stands. We should definitely handle and 
report EFI_CONNECTION_FIN (server terminated the connection). 
Additionally, HTTP client should terminate the connection when it 
detects that the server is responding to it with HTTP version lower than 
1.1 (no TCP connection persistence).

Summary & my suggestions:
1. I would like to drop the patch and close this issue as a problem on 
server-side (lack of support for HTTP/1.1, which is supported by 
HttpBootDxe).

2. I would like to open a new Bugzilla to directly state that 
HttpBootDxe does not correctly report lack of support for at least 
HTTP/1.1 on server-side and also does not report termination of the TCP 
connection.
Based on this Bugzilla, we could properly submit reporting of lack of 
support for servers not supporting at least HTTP/1.1.

Looking forward to your response.

On 26-May-20 08:13, Andrei Warkentin wrote:
> Hi Maciej,
>
> Thanks for your analysis.
>
> The way I see it, there are many people out there expecting HttpBoot 
> to work with the Python server - which without manual hacking is 
> HTTP/1.0 and not HTTP/1.1. I think it's fine to reject 1.0 but then 
> HttpBoot should explicitly reject 1.0 with an appropriate message and 
> *refuse* to boot. Today it's in the area where it may work and may not.
>
> 1.1 vs 1.0 aside, don't you think there should be a bit of resiliency 
> to the client? As it stands right now, an arbitrary FIN  between the 
> requests gets the user an "unexpected network error" (or similar), 
> with no clear indication it's a server misconfiguration, a network 
> error (which one?) or some such. With that in mind I see the proposed 
> fix as being a lesser evil (friendly to end user) than creating a 
> pedantic HttpBoot client which will fail because it doesn't operate in 
> an ideal environment.
>
> What do you think?
>
> A
> ------------------------------------------------------------------------
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of 
> Maciej Rabeda via groups.io <maciej.rabeda=linux.intel.com@groups.io>
> *Sent:* Monday, May 25, 2020 7:49 AM
> *To:* devel@edk2.groups.io <devel@edk2.groups.io>; 
> andrey.warkentin@gmail.com <andrey.warkentin@gmail.com>
> *Cc:* jiaxin.wu@intel.com <jiaxin.wu@intel.com>; siyuan.fu@intel.com 
> <siyuan.fu@intel.com>
> *Subject:* Re: [edk2-devel] [edk2][PATCH 1/1] NetworkPkg/HttpBootDxe: 
> handle servers which may FIN after file sizing in HttpBootLoadFile
> Copying my comments from Bugzilla:
> I have gone through the Wireshark trace that you have provided. It 
> seems to be all clear now and the approach to the issue must change. 
> HttpBootDxe supports HTTP 1.1 and assumes that the HTTP session stays 
> persistent (single TCP connection for multiple requests instead of one 
> for each HTTP request/response pair). 
> https://en.wikipedia.org/wiki/HTTP_persistent_connection 
> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FHTTP_persistent_connection&data=02%7C01%7Cawarkentin%40vmware.com%7Cec2598bdff38420cccac08d800aa02f4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637260077485261398&sdata=TpPhcFlc9GKHIx1YRm62hRnsxNqZqyAoqmSCViuYTK8%3D&reserved=0> 
> I am hesitant to introduce HTTP/1.0 keep-alive additions to the 
> header. Kindly request more debugging on Python server side whether to 
> confirm that implementation you are using supports HTTP/1.1. From what 
> I see in here: 
> https://github.com/python/cpython/blob/master/Lib/http/server.py 
> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpython%2Fcpython%2Fblob%2Fmaster%2FLib%2Fhttp%2Fserver.py&data=02%7C01%7Cawarkentin%40vmware.com%7Cec2598bdff38420cccac08d800aa02f4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637260077485261398&sdata=xRP7GQ%2FaNvtQHPNfgVUkKlUiF%2F5cb13PVYrr%2BislEZQ%3D&reserved=0> 
> http.server has support for HTTP/1.1 and assumption on not closing the 
> connection. At the time of writing this comment, it is line 311: if 
> version_number >= (1, 1) and self.protocol_version >= "HTTP/1.1": 
> self.close_connection = False Putting the patch on hold (with trend 
> for rejecting with no support for HTTP/1.0 keep-alive). Additional 
> comment: 
> https://github.com/python/cpython/blob/master/Lib/http/server.py 
> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fpython%2Fcpython%2Fblob%2Fmaster%2FLib%2Fhttp%2Fserver.py&data=02%7C01%7Cawarkentin%40vmware.com%7Cec2598bdff38420cccac08d800aa02f4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637260077485271388&sdata=MjOWGYwvUxDseQBxn27n7HaqcX2X0g2s7bxeacEXQW0%3D&reserved=0> 
> Line 616: # The version of the HTTP protocol we support. # Set this to 
> HTTP/1.1 to enable automatic keepalive protocol_version = "HTTP/1.0" 
> Please modify it that variable to "HTTP/1.1". Your scenario should 
> start working. Please come back to me with confirmation.
>
> Thanks,
> Maciej
>
> On 22-May-20 21:46, Andrei Warkentin wrote:
>> This is v2 with Maciej Rabeda's feedback.
>>
>> Python http.server seems to FIN after the first HEAD request to size
>> the loaded file is completed and ACKed. What happens next is interesting.
>> On low latency connections, the GET request to download may get sent
>> after the server sends the FIN but before the client has a chance to
>> process it. The net result is:
>> - Server ignores GET
>> - HttpBootLoadFile returns EFI_CONNECTION_FIN. Boot fails.
>>
>> In the other case, client handles the FIN before attempting the GET,
>> so there's a proper three-way handshake as part of GET.
>>
>> The solution is to retry HttpBootLoadFile 2 times if it returns
>> EFI_CONNECTION_FIN. This is because HttpBootLoadFile may issue up to
>> 3 requests: HEAD/GET to get size and final GET to load. Some servers
>> may send a FIN after each request. The first request (HEAD) is not
>> supposed to fail this way, so only the two subsequent GET request
>> may result in a FIN.
>>
>> Fixeshttps://bugzilla.tianocore.org/show_bug.cgi?id=2720  <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2720&data=02%7C01%7Cawarkentin%40vmware.com%7Cec2598bdff38420cccac08d800aa02f4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637260077485271388&sdata=QdLUU9IWG0NlMg0p93NWpWdoYrP9akaPdH%2F1WENUWk4%3D&reserved=0>
>>
>> This what the boot looks like now (when using that Python server):
>>>> Start HTTP Boot over IPv4....
>>    Station IP address is 10.0.1.186
>>
>>    URI:http://10.0.1.57/bootaa64.efi  <https://nam04.safelinks.protection.outlook.com/?url=http%3A%2F%2F10.0.1.57%2Fbootaa64.efi&data=02%7C01%7Cawarkentin%40vmware.com%7Cec2598bdff38420cccac08d800aa02f4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637260077485281382&sdata=%2FtRT7u%2Fnho6nIUrAKpUT1t6YEmeWGgVBmR7vH%2Fhs9OU%3D&reserved=0>
>>
>>   Error: Server has terminated the connection.
>>
>>    URI:http://10.0.1.57/bootaa64.efi  <https://nam04.safelinks.protection.outlook.com/?url=http%3A%2F%2F10.0.1.57%2Fbootaa64.efi&data=02%7C01%7Cawarkentin%40vmware.com%7Cec2598bdff38420cccac08d800aa02f4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637260077485281382&sdata=%2FtRT7u%2Fnho6nIUrAKpUT1t6YEmeWGgVBmR7vH%2Fhs9OU%3D&reserved=0>
>>    File Size: 179160 Bytes
>>    Downloading...100%
>>
>> Cc: Maciej Rabeda<maciej.rabeda@linux.intel.com>  <mailto:maciej.rabeda@linux.intel.com>
>> Cc: Jiaxin Wu<jiaxin.wu@intel.com>  <mailto:jiaxin.wu@intel.com>
>> Cc: Siyuan Fu<siyuan.fu@intel.com>  <mailto:siyuan.fu@intel.com>
>> Signed-off-by: Andrei Warkentin<andrey.warkentin@gmail.com>  <mailto:andrey.warkentin@gmail.com>
>> ---
>>   NetworkPkg/HttpBootDxe/HttpBootImpl.c | 35 +++++++++++++++++++-
>>   NetworkPkg/HttpDxe/HttpImpl.c         |  5 +++
>>   2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/NetworkPkg/HttpBootDxe/HttpBootImpl.c b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
>> index 4a51f35cdd..1a251b4347 100644
>> --- a/NetworkPkg/HttpBootDxe/HttpBootImpl.c
>> +++ b/NetworkPkg/HttpBootDxe/HttpBootImpl.c
>> @@ -288,6 +288,7 @@ HttpBootDhcp (
>>     @retval EFI_NOT_STARTED             The driver is in stopped state.
>>     @retval EFI_BUFFER_TOO_SMALL        The BufferSize is too small to read the boot file. BufferSize has
>>                                         been updated with the size needed to complete the request.
>> +  @retval EFI_CONNECTION_FIN          Server had closed the connection while we were waiting downloading.
>>     @retval EFI_DEVICE_ERROR            An unexpected network error occurred.
>>     @retval Others                      Other errors as indicated.
>>   
>> @@ -408,6 +409,8 @@ ON_EXIT:
>>         AsciiPrint ("\n  Error: Server response timeout.\n");
>>       } else if (Status == EFI_ABORTED) {
>>         AsciiPrint ("\n  Error: Remote boot cancelled.\n");
>> +    } else if (Status == EFI_CONNECTION_FIN) {
>> +      AsciiPrint ("\n Error: Server has terminated the connection.\n");
>>       } else if (Status != EFI_BUFFER_TOO_SMALL) {
>>         AsciiPrint ("\n  Error: Unexpected network error.\n");
>>       }
>> @@ -553,6 +556,7 @@ HttpBootDxeLoadFile (
>>     BOOLEAN                       UsingIpv6;
>>     EFI_STATUS                    Status;
>>     HTTP_BOOT_IMAGE_TYPE          ImageType;
>> +  UINTN                         MaxTries;
>>   
>>     if (This == NULL || BufferSize == NULL || FilePath == NULL) {
>>       return EFI_INVALID_PARAMETER;
>> @@ -598,13 +602,42 @@ HttpBootDxeLoadFile (
>>     // Load the boot file.
>>     //
>>     ImageType = ImageTypeMax;
>> -  Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
>> +  //
>> +  // HttpBootLoadFile may issue up to 2 requests: HEAD/GET to get
>> +  // size and final GET to load. Some servers may send a FIN after
>> +  // each request. The first request (HEAD) is not supposed to
>> +  // fail this way, so only the two possible GETs need the special
>> +  // handling.
>> +  //
>> +  MaxTries = 2;
>> +  do {
>> +    Status = HttpBootLoadFile (Private, BufferSize, Buffer, &ImageType);
>> +    if (Status == EFI_CONNECTION_FIN) {
>> +      if (Private->HttpCreated) {
>> +        //
>> +        // Tear down HTTP/TCP state entirely. Http->Configure (NULL) is not
>> +        // sufficient (EFI_ACCESS_DENIED from TCP stack on subsequent
>> +        // HttpBootLoadFile.
>> +        //
>> +        HttpIoDestroyIo (&Private->HttpIo);
>> +        Private->HttpCreated = FALSE;
>> +      }
>> +    }
>> +  } while (MaxTries-- && Status == EFI_CONNECTION_FIN);
>> +
>>     if (EFI_ERROR (Status)) {
>>       if (Status == EFI_BUFFER_TOO_SMALL && (ImageType == ImageTypeVirtualCd || ImageType == ImageTypeVirtualDisk)) {
>>         Status = EFI_WARN_FILE_SYSTEM;
>>       } else if (Status != EFI_BUFFER_TOO_SMALL) {
>>         HttpBootStop (Private);
>>       }
>> +    if (Status == EFI_CONNECTION_FIN) {
>> +      //
>> +      // EFI_CONNECTION_FIN is not an expected error for EFI_LOAD_FILE_PROTOCOL.LoadFile(), so
>> +      // map it to closest matching error. Note: already logged the error in HttpBootLoadFile.
>> +      //
>> +      Status = EFI_ABORTED;
>> +    }
>>       return Status;
>>     }
>>   
>> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
>> index 5a6ecbc9d9..34a33b09f7 100644
>> --- a/NetworkPkg/HttpDxe/HttpImpl.c
>> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
>> @@ -959,6 +959,8 @@ HttpBodyParserCallback (
>>     @retval EFI_OUT_OF_RESOURCES    Failed to complete the operation due to lack of resources.
>>     @retval EFI_NOT_READY           Can't find a corresponding Tx4Token/Tx6Token or
>>                                     the EFI_HTTP_UTILITIES_PROTOCOL is not available.
>> +  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
>> +                                  a response.
>>   
>>   **/
>>   EFI_STATUS
>> @@ -1528,6 +1530,9 @@ Error:
>>     @retval EFI_OUT_OF_RESOURCES    Could not allocate enough system resources.
>>     @retval EFI_ACCESS_DENIED       An open TCP connection is not present with the host
>>                                     specified by response URL.
>> +
>> +  @retval EFI_CONNECTION_FIN      Server had closed the connection while we were waiting for
>> +                                  a response.
>>   **/
>>   EFI_STATUS
>>   EFIAPI
>
> 


[-- Attachment #2: Type: text/html, Size: 19804 bytes --]