Re: [Patch 2/2] NetworkPkg: Read TlsCipherList variable and configure it for HTTPS session.

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Thanks Laszlo, I will integrate your comments into the new patch.

Best Regards!
Jiaxin 

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, February 9, 2018 6:16 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Zimmer, Vincent
> <vincent.zimmer@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ye,
> Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: Re: [Patch 2/2] NetworkPkg: Read TlsCipherList variable and
> configure it for HTTPS session.
> 
> On 02/09/18 04:59, Jiaxin Wu wrote:
> > This patch is to read the TlsCipherList variable and configure it for the
> > later HTTPS session.
> >
> > If the variable is not set by any platform, EFI_NOT_FOUND will be returned
> > from GetVariable service. In such a case, the default CipherList created in
> > TlsDxe driver will be used.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Kinney Michael D <michael.d.kinney@intel.com>
> > Cc: Zimmer Vincent <vincent.zimmer@intel.com>
> > Cc: Yao Jiewen <jiewen.yao@intel.com>
> > Cc: Ye Ting <ting.ye@intel.com>
> > Cc: Fu Siyuan <siyuan.fu@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> > ---
> >  NetworkPkg/HttpDxe/HttpDriver.h   |  3 +-
> >  NetworkPkg/HttpDxe/HttpDxe.inf    |  3 +-
> >  NetworkPkg/HttpDxe/HttpsSupport.c | 92
> ++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 95 insertions(+), 3 deletions(-)
> >
> > diff --git a/NetworkPkg/HttpDxe/HttpDriver.h
> b/NetworkPkg/HttpDxe/HttpDriver.h
> > index 93a412a..eba7d32 100644
> > --- a/NetworkPkg/HttpDxe/HttpDriver.h
> > +++ b/NetworkPkg/HttpDxe/HttpDriver.h
> > @@ -1,9 +1,9 @@
> >  /** @file
> >    The header files of the driver binding and service binding protocol for
> HttpDxe driver.
> >
> > -  Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> > +  Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
> >    (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> >
> >    This program and the accompanying materials
> >    are licensed and made available under the terms and conditions of the
> BSD License
> >    which accompanies this distribution.  The full text of the license may be
> found at
> > @@ -59,10 +59,11 @@
> >  // Produced Protocols
> >  //
> >  #include <Protocol/Http.h>
> >
> >  #include <Guid/TlsAuthentication.h>
> > +#include <Guid/TlsCipherList.h>
> >
> >  #include <IndustryStandard/Tls1.h>
> >
> >  //
> >  // Driver Version
> > diff --git a/NetworkPkg/HttpDxe/HttpDxe.inf
> b/NetworkPkg/HttpDxe/HttpDxe.inf
> > index 20075f5..b1d7bd2 100644
> > --- a/NetworkPkg/HttpDxe/HttpDxe.inf
> > +++ b/NetworkPkg/HttpDxe/HttpDxe.inf
> > @@ -1,9 +1,9 @@
> >  ## @file
> >  #  Implementation of EFI HTTP protocol interfaces.
> >  #
> > -#  Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR>
> >  #
> >  #  This program and the accompanying materials
> >  #  are licensed and made available under the terms and conditions of the
> BSD License
> >  #  which accompanies this distribution. The full text of the license may be
> found at
> >  #  http://opensource.org/licenses/bsd-license.php.
> > @@ -72,10 +72,11 @@
> >    gEfiTlsProtocolGuid                              ## SOMETIMES_CONSUMES
> >    gEfiTlsConfigurationProtocolGuid                 ## SOMETIMES_CONSUMES
> >
> >  [Guids]
> >    gEfiTlsCaCertificateGuid                         ## SOMETIMES_CONSUMES  ##
> Variable:L"TlsCaCertificate"
> > +  gTlsCipherListGuid                               ## SOMETIMES_CONSUMES  ##
> Variable:L"TlsCipherList"
> >
> >  [Pcd]
> >    gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections       ##
> CONSUMES
> >    gEfiNetworkPkgTokenSpaceGuid.PcdHttpsAuthenticationMode    ##
> SOMETIMES_CONSUMES
> >    gEfiNetworkPkgTokenSpaceGuid.PcdHttpsHostPublicCert        ##
> SOMETIMES_CONSUMES
> > diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c
> b/NetworkPkg/HttpDxe/HttpsSupport.c
> > index 288082a..62cb867 100644
> > --- a/NetworkPkg/HttpDxe/HttpsSupport.c
> > +++ b/NetworkPkg/HttpDxe/HttpsSupport.c
> > @@ -1,9 +1,9 @@
> >  /** @file
> >    Miscellaneous routines specific to Https for HttpDxe driver.
> >
> > -Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
> >  (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> >  This program and the accompanying materials
> >  are licensed and made available under the terms and conditions of the BSD
> License
> >  which accompanies this distribution.  The full text of the license may be
> found at
> >  http://opensource.org/licenses/bsd-license.php
> > @@ -492,10 +492,91 @@ TlsConfigCertificate (
> >
> >    return Status;
> >  }
> >
> >  /**
> > +  Read the TlsCipherList variable and configure it for HTTPS session.
> > +
> > +  @param[in, out]  HttpInstance       The HTTP instance private data.
> > +
> > +  @retval EFI_SUCCESS            The prefered TLS CipherList is configured.
> > +  @retval EFI_NOT_FOUND          Fail to get 'TlsCipherList' variable.
> > +  @retval EFI_INVALID_PARAMETER  The contents of variable are invalid.
> > +  @retval EFI_OUT_OF_RESOURCES   Can't allocate memory resources.
> > +
> > +  @retval Others                 Other error as indicated.
> > +
> > +**/
> > +EFI_STATUS
> > +TlsConfigCipherList (
> > +  IN OUT HTTP_PROTOCOL      *HttpInstance
> > +  )
> > +{
> > +  EFI_STATUS          Status;
> > +  UINT8               *CipherList;
> > +  UINTN               CipherListSize;
> > +
> > +  CipherList     = NULL;
> > +  CipherListSize = 0;
> > +
> > +  //
> > +  // Try to read the TlsCipherList variable.
> > +  //
> > +  Status  = gRT->GetVariable (
> > +                   EDKII_TLS_CIPHER_LIST_VARIABLE,
> > +                   &gTlsCipherListGuid,
> > +                   NULL,
> > +                   &CipherListSize,
> > +                   NULL
> > +                   );
> > +
> > +  if (EFI_ERROR (Status) && Status != EFI_BUFFER_TOO_SMALL) {
> > +    return Status;
> > +  }
> 
> I think the above GetVariable service call can never succeed. What about:
> 
>   ASSERT (EFI_ERROR (Status));
>   if (Status != EFI_BUFFER_TOO_SMALL) {
>     return Status;
>   }
> 
> Not very important (the behavior won't change), but I think this is
> better for documentation purposes.
> 
> The patch looks good to me otherwise.
> 
> Thank you!
> Laszlo
> 
> 
> > +
> > +  if (CipherListSize % sizeof (EFI_TLS_CIPHER) != 0) {
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  //
> > +  // Allocate buffer and read the config variable.
> > +  //
> > +  CipherList = AllocatePool (CipherListSize);
> > +  if (CipherList == NULL) {
> > +    return EFI_OUT_OF_RESOURCES;
> > +  }
> > +
> > +  Status = gRT->GetVariable (
> > +                  EDKII_TLS_CIPHER_LIST_VARIABLE,
> > +                  &gTlsCipherListGuid,
> > +                  NULL,
> > +                  &CipherListSize,
> > +                  CipherList
> > +                  );
> > +  if (EFI_ERROR (Status)) {
> > +    //
> > +    // GetVariable still error or the variable is corrupted.
> > +    //
> > +    goto ON_EXIT;
> > +  }
> > +
> > +  ASSERT (CipherList != NULL);
> > +
> > +  Status = HttpInstance->Tls->SetSessionData (
> > +                                HttpInstance->Tls,
> > +                                EfiTlsCipherList,
> > +                                CipherList,
> > +                                CipherListSize
> > +                                );
> > +
> > +ON_EXIT:
> > +  FreePool (CipherList);
> > +
> > +  return Status;
> > +}
> > +
> > +/**
> >    Configure TLS session data.
> >
> >    @param[in, out]  HttpInstance       The HTTP instance private data.
> >
> >    @retval EFI_SUCCESS            TLS session data is configured.
> > @@ -551,10 +632,19 @@ TlsConfigureSession (
> >    if (EFI_ERROR (Status)) {
> >      return Status;
> >    }
> >
> >    //
> > +  // Tls Cipher List
> > +  //
> > +  Status = TlsConfigCipherList (HttpInstance);
> > +  if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND) {
> > +    DEBUG ((EFI_D_ERROR, "TlsConfigCipherList: return %r error.\n",
> Status));
> > +    return Status;
> > +  }
> > +
> > +  //
> >    // Tls Config Certificate
> >    //
> >    Status = TlsConfigCertificate (HttpInstance);
> >    if (EFI_ERROR (Status)) {
> >      DEBUG ((EFI_D_ERROR, "TlsConfigCertificate: return %r error.\n",
> Status));
> >