Re: [PATCH] RedfishPkg/RedfishPlatformCredentialLib: IPMI implementation

["Chang, Abner" <abner.chang@amd.com>]

[AMD Official Use Only - General]

Hi Nickle, please add Igor as reviewer too. My comments is in below,

> -----Original Message-----
> From: Nickle Wang <nicklew@nvidia.com>
> Sent: Thursday, October 20, 2022 10:55 AM
> To: devel@edk2.groups.io
> Cc: Chang, Abner <Abner.Chang@amd.com>; Nick Ramirez
> <nramirez@nvidia.com>
> Subject: [PATCH] RedfishPkg/RedfishPlatformCredentialLib: IPMI
> implementation
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> This library follows Redfish Host Interface specification and use IPMI command
> to get bootstrap account credential(NetFn 2Ch, Command 02h) from BMC.
> RedfishHostInterfaceDxe will use this credential for the following
> communication between BIOS and BMC.
> 
> Cc: Abner Chang <abner.chang@amd.com>
> Cc: Nick Ramirez <nramirez@nvidia.com>
> Signed-off-by: Nickle Wang <nicklew@nvidia.com>
> ---
>  .../RedfishPlatformCredentialLib.c            | 273 ++++++++++++++++++
>  .../RedfishPlatformCredentialLib.h            |  75 +++++
>  .../RedfishPlatformCredentialLib.inf          |  37 +++
[Chang, Abner] 
Could we name this library RedfishPlatformCredentialIpmi so the naming style is consistent with RedfishPlatformCredentialNull?

>  3 files changed, 385 insertions(+)
>  create mode 100644
> RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLib.
> c
>  create mode 100644
> RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLib.
> h
>  create mode 100644
> RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLib.i
> nf
> 
> diff --git
> a/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLi
> b.c
> b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLi
> b.c
> new file mode 100644
> index 0000000000..23a15ab1fa
> --- /dev/null
> +++ b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCre
> +++ dentialLib.c
> @@ -0,0 +1,273 @@
> +/** @file
> +*
> +*  Copyright (c) 2022 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
[Chang, Abner] 
We can have "@par Revision Reference:"  in the file header to point out the spec.
https://www.dmtf.org/sites/default/files/standards/documents/DSP0270_1.3.0.pdf

> +*
> +**/
> +
> +#include "RedfishPlatformCredentialLib.h"
> +
> +//
> +// Global flag of controlling credential service // BOOLEAN
> +mRedfishServiceStopped = FALSE;
> +
> +/**
> +  Notify the Redfish service provide to stop provide configuration service to this
> platform.
> +
> +  This function should be called when the platfrom is about to leave the safe
> environment.
> +  It will notify the Redfish service provider to abort all logined
> + session, and prohibit  further login with original auth info.
> + GetAuthInfo() will return EFI_UNSUPPORTED once this  function is returned.
> +
> +  @param[in]   This                Pointer to
> EDKII_REDFISH_CREDENTIAL_PROTOCOL instance.
> +  @param[in]   ServiceStopType     Reason of stopping Redfish service.
> +
> +  @retval EFI_SUCCESS              Service has been stoped successfully.
> +  @retval EFI_INVALID_PARAMETER    This is NULL.
> +  @retval Others                   Some error happened.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibStopRedfishService (
> +  IN     EDKII_REDFISH_CREDENTIAL_PROTOCOL           *This,
> +  IN     EDKII_REDFISH_CREDENTIAL_STOP_SERVICE_TYPE  ServiceStopType
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  if ((ServiceStopType <= ServiceStopTypeNone) || (ServiceStopType >=
> ServiceStopTypeMax)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  //
> +  // Raise flag first
> +  //
> +  mRedfishServiceStopped = TRUE;
> +
> +  //
> +  // Notify BMC to disable credential bootstrapping support.
> +  //
> +  Status = GetBootstrapAccountCredentials (TRUE, NULL, NULL);  if
> + (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: fail to disable bootstrap credential: %r\n",
> __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  Notification of Exit Boot Service.
> +
> +  @param[in]  This    Pointer to EDKII_REDFISH_CREDENTIAL_PROTOCOL.
> +**/
> +VOID
> +EFIAPI
> +LibCredentialExitBootServicesNotify (
> +  IN  EDKII_REDFISH_CREDENTIAL_PROTOCOL  *This
> +  )
> +{
> +  //
> +  // Stop the credential support when system is about to enter OS.
> +  //
> +  LibStopRedfishService (This, ServiceStopTypeExitBootService); }
> +
> +/**
> +  Notification of End of DXe.
> +
> +  @param[in]  This    Pointer to EDKII_REDFISH_CREDENTIAL_PROTOCOL.
> +**/
> +VOID
> +EFIAPI
> +LibCredentialEndOfDxeNotify (
> +  IN  EDKII_REDFISH_CREDENTIAL_PROTOCOL  *This
> +  )
> +{
> +  //
> +  // Do nothing now.
> +  // We can stop credential support when system reach end-of-dxe for security
> reason.
> +  //
> +}
> +
> +/**
> +  Function to retrieve temporary use credentials for the UEFI redfish
> +client
[Chang, Abner] 
We miss the functionality to disable bootstrap credential service in the function description.

> +
> +  @param[in]  DisableBootstrapControl
> +                                      TRUE - Tell the BMC to disable the bootstrap credential
> +                                             service to ensure no one else gains credentials
> +                                      FALSE  Allow the bootstrap
> + credential service to continue  @param[out] BootstrapUsername
> +                                      A pointer to a UTF-8 encoded string for the credential
> username
> +                                      When DisableBootstrapControl is
> + TRUE, this pointer can be NULL
> +
> +  @param[out] BootstrapPassword
> +                                      A pointer to a UTF-8 encoded string for the credential
> password
> +                                      When DisableBootstrapControl is
> + TRUE, this pointer can be NULL
> +
> +  @retval  EFI_SUCCESS                Credentials were successfully fetched and
> returned
> +  @retval  EFI_INVALID_PARAMETER      BootstrapUsername or
> BootstrapPassword is NULL when DisableBootstrapControl
> +                                      is set to FALSE
> +  @retval  EFI_DEVICE_ERROR           An IPMI failure occurred
[Chang, Abner]
The return status should also include the status of disabling bootstrap credential.


> +**/
> +EFI_STATUS
> +GetBootstrapAccountCredentials (
> +  IN BOOLEAN DisableBootstrapControl,
> +  IN OUT CHAR8 *BootstrapUsername, OPTIONAL
> +  IN OUT CHAR8  *BootstrapPassword    OPTIONAL
> +  )
> +{
> +  EFI_STATUS                                  Status;
> +  IPMI_BOOTSTRAP_CREDENTIALS_COMMAND_DATA     CommandData;
> +  IPMI_BOOTSTRAP_CREDENTIALS_RESULT_RESPONSE  ResponseData;
> +  UINT32                                      ResponseSize;
> +
> +  if (!PcdGetBool (PcdIpmiFeatureEnable)) {
> +    DEBUG ((DEBUG_ERROR, "%a: IPMI is not enabled! Unable to fetch Redfish
> credentials\n", __FUNCTION__));
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  //
> +  // NULL buffer check
> +  //
> +  if (!DisableBootstrapControl && ((BootstrapUsername == NULL) ||
> (BootstrapPassword == NULL))) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a: Disable bootstrap control: 0x%x\n",
> + __FUNCTION__, DisableBootstrapControl));
> +
> +  //
> +  // IPMI callout to NetFn 2C, command 02
> +  //    Request data:
> +  //      Byte 1: REDFISH_IPMI_GROUP_EXTENSION
> +  //      Byte 2: DisableBootstrapControl
> +  //
> +  CommandData.GroupExtensionId        = REDFISH_IPMI_GROUP_EXTENSION;
> +  CommandData.DisableBootstrapControl = (DisableBootstrapControl ?
> + REDFISH_IPMI_BOOTSTRAP_CREDENTIAL_DISABLE :
> + REDFISH_IPMI_BOOTSTRAP_CREDENTIAL_ENABLE);
> +
> +  ResponseSize = sizeof (ResponseData);
> +
> +  //
> +  //    Response data:
> +  //      Byte 1    : Completion code
> +  //      Byte 2    : REDFISH_IPMI_GROUP_EXTENSION
> +  //      Byte 3-18 : Username
> +  //      Byte 19-34: Password
> +  //
> +  Status = IpmiSubmitCommand (
> +             IPMI_NETFN_GROUP_EXT,
> +             REDFISH_IPMI_GET_BOOTSTRAP_CREDENTIALS_CMD,
> +             (UINT8 *)&CommandData,
> +             sizeof (CommandData),
> +             (UINT8 *)&ResponseData,
> +             &ResponseSize
> +             );
> +
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: IPMI transaction failure. Returning\n",
> __FUNCTION__));
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
> +  } else {
> +    if (ResponseData.CompletionCode != IPMI_COMP_CODE_NORMAL) {
> +      if (ResponseData.CompletionCode ==
> REDFISH_IPMI_COMP_CODE_BOOTSTRAP_CREDENTIAL_DISABLED) {
> +        DEBUG ((DEBUG_ERROR, "%a: bootstrap credential support was
> disabled\n", __FUNCTION__));
> +        return EFI_ACCESS_DENIED;
> +      }
> +
> +      DEBUG ((DEBUG_ERROR, "%a: Completion code = 0x%x. Returning\n",
> __FUNCTION__, ResponseData.CompletionCode));
> +      return EFI_PROTOCOL_ERROR;
> +    } else if (ResponseData.GroupExtensionId !=
> REDFISH_IPMI_GROUP_EXTENSION) {
> +      DEBUG ((DEBUG_ERROR, "%a: Group Extension Response = 0x%x.
> Returning\n", __FUNCTION__, ResponseData.GroupExtensionId));
> +      return EFI_DEVICE_ERROR;
> +    } else {
> +      if (BootstrapUsername != NULL) {
> +        CopyMem (BootstrapUsername, ResponseData.Username,
> USERNAME_MAX_LENGTH);
> +        //
> +        // Manually append null-terminator in case 16 characters username
> returned.
> +        //
> +        BootstrapUsername[USERNAME_MAX_LENGTH] = '\0';
> +      }
> +
> +      if (BootstrapPassword != NULL) {
> +        CopyMem (BootstrapPassword, ResponseData.Password,
> PASSWORD_MAX_LENGTH);
> +        //
> +        // Manually append null-terminator in case 16 characters password
> returned.
> +        //
> +        BootstrapPassword[PASSWORD_MAX_LENGTH] = '\0';
> +      }
> +    }
> +  }
> +
> +  return Status;
> +}
> +
> +/**
> +  Retrieve platform's Redfish authentication information.
> +
> +  This functions returns the Redfish authentication method together
> + with the user Id and  password.
> +  - For AuthMethodNone, the UserId and Password could be used for HTTP
> header authentication
> +    as defined by RFC7235.
> +  - For AuthMethodRedfishSession, the UserId and Password could be used for
> Redfish
> +    session login as defined by  Redfish API specification (DSP0266).
> +
> +  Callers are responsible for and freeing the returned string storage.
> +
> +  @param[in]   This                Pointer to
> EDKII_REDFISH_CREDENTIAL_PROTOCOL instance.
> +  @param[out]  AuthMethod          Type of Redfish authentication method.
> +  @param[out]  UserId              The pointer to store the returned UserId string.
> +  @param[out]  Password            The pointer to store the returned Password
> string.
> +
> +  @retval EFI_SUCCESS              Get the authentication information successfully.
> +  @retval EFI_ACCESS_DENIED        SecureBoot is disabled after EndOfDxe.
> +  @retval EFI_INVALID_PARAMETER    This or AuthMethod or UserId or
> Password is NULL.
> +  @retval EFI_OUT_OF_RESOURCES     There are not enough memory resources.
> +  @retval EFI_UNSUPPORTED          Unsupported authentication method is
> found.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +LibCredentialGetAuthInfo (
> +  IN  EDKII_REDFISH_CREDENTIAL_PROTOCOL  *This,
> +  OUT EDKII_REDFISH_AUTH_METHOD          *AuthMethod,
> +  OUT CHAR8                              **UserId,
> +  OUT CHAR8                              **Password
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  if ((AuthMethod == NULL) || (UserId == NULL) || (Password == NULL)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *UserId   = NULL;
> +  *Password = NULL;
> +
> +  if (mRedfishServiceStopped) {
> +    DEBUG ((DEBUG_ERROR, "%a: credential service is stopped due to security
> reason\n", __FUNCTION__));
> +    return EFI_ACCESS_DENIED;
> +  }
> +
> +  *AuthMethod = AuthMethodHttpBasic;
> +
> +  *UserId = AllocateZeroPool (sizeof (CHAR8) * USERNAME_MAX_SIZE);  if
[Chang, Abner] 
Allocation memory with the size (USERNAME_MAX_LENGTH + 1)  for both BootUsername and BootstrapPassword?   Because the maximum number of characters defined in the spec is USERNAME_MAX_LENGTH for the user/password.


> + (*UserId == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  *Password = AllocateZeroPool (sizeof (CHAR8) * PASSWORD_MAX_SIZE);
> + if (*Password == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  Status = GetBootstrapAccountCredentials (FALSE, *UserId, *Password);
> + if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR, "%a: fail to get bootstrap credential: %r\n",
> __FUNCTION__, Status));
> +    return Status;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> diff --git
> a/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLi
> b.h
> b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLi
> b.h
> new file mode 100644
> index 0000000000..5b448e01be
> --- /dev/null
> +++ b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCre
> +++ dentialLib.h
> @@ -0,0 +1,75 @@
> +/** @file
> +*
> +*  Copyright (c) 2022 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +#include <Uefi.h>
> +#include <IndustryStandard/Ipmi.h>
> +#include <Protocol/EdkIIRedfishCredential.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IpmiBaseLib.h>
> +#include <Library/MemoryAllocationLib.h> #include
> +<Library/RedfishCredentialLib.h>
> +
> +#define REDFISH_IPMI_GROUP_EXTENSION                          0x52
> +#define REDFISH_IPMI_GET_BOOTSTRAP_CREDENTIALS_CMD            0x02
> +#define REDFISH_IPMI_BOOTSTRAP_CREDENTIAL_ENABLE              0xA5
> +#define REDFISH_IPMI_BOOTSTRAP_CREDENTIAL_DISABLE             0x00
> +#define REDFISH_IPMI_COMP_CODE_BOOTSTRAP_CREDENTIAL_DISABLED
> 0x80
> +
> +//
> +// Per Redfish Host Interface Specification 1.3, The maximum lenght of
> +// username and password is 16 characters long.
> +//
> +#define USERNAME_MAX_LENGTH  16
> +#define PASSWORD_MAX_LENGTH  16
> +#define USERNAME_MAX_SIZE    (USERNAME_MAX_LENGTH + 1)  // NULL
> terminator
> +#define PASSWORD_MAX_SIZE    (PASSWORD_MAX_LENGTH + 1)  // NULL
> terminator
> +
> +#pragma pack(1)
> +///
> +/// The definition of IPMI command to get bootstrap account credentials
> +/// typedef struct {
> +  UINT8    GroupExtensionId;
> +  UINT8    DisableBootstrapControl;
> +} IPMI_BOOTSTRAP_CREDENTIALS_COMMAND_DATA;
> +
> +///
> +/// The response data of getting bootstrap credential /// typedef
> +struct {
> +  UINT8    CompletionCode;
> +  UINT8    GroupExtensionId;
> +  CHAR8    Username[USERNAME_MAX_LENGTH];
> +  CHAR8    Password[PASSWORD_MAX_LENGTH];
> +} IPMI_BOOTSTRAP_CREDENTIALS_RESULT_RESPONSE;
> +
> +#pragma pack()
> +
> +/**
> +  Function to retrieve temporary use credentials for the UEFI redfish
> +client
[Chang, Abner] 
We miss the functionality to disable bootstrap credential service in the function description.

> +
> +  @param[in]  DisableBootstrapControl
> +                                      TRUE - Tell the BMC to disable the bootstrap credential
> +                                             service to ensure no one else gains credentials
> +                                      FALSE  Allow the bootstrap
> + credential service to continue  @param[out] BootstrapUsername
> +                                      A pointer to a UTF-8 encoded
> + string for the credential username
> +
> +  @param[out] BootstrapPassword
> +                                      A pointer to a UTF-8 encoded
> + string for the credential password
> +
> +  @retval  EFI_SUCCESS                Credentials were successfully fetched and
> returned
[Chang, Abner]
Or the bootstrap credential service is disabled successfully, right?

> +  @retval  EFI_DEVICE_ERROR           An IPMI failure occurred
> +**/
> +EFI_STATUS
> +GetBootstrapAccountCredentials (
> +  IN BOOLEAN    DisableBootstrapControl,
> +  IN OUT CHAR8  *BootstrapUsername,
> +  IN OUT CHAR8  *BootstrapPassword
> +  );
> diff --git
> a/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLi
> b.inf
> b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCredentialLi
> b.inf
> new file mode 100644
> index 0000000000..a990d28363
> --- /dev/null
> +++ b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCre
> +++ dentialLib.inf
> @@ -0,0 +1,37 @@
> +## @file
> +#
> +# Copyright (c) 2022 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent # ##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001000b
> +  BASE_NAME                      = RedfishPlatformCredentialLib
> +  FILE_GUID                      = 9C45D622-4C66-417F-814C-F76246D97233
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = RedfishPlatformCredentialLib
> +
> +[Sources]
> +  RedfishPlatformCredentialLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  RedfishPkg/RedfishPkg.dec
> +  IpmiFeaturePkg/IpmiFeaturePkg.dec
[Chang, Abner] 
Could you please add a comment to the reference  of IpmiFeaturePkg? We have to give customers a notice that the dependence of "edk2-platforms/Features/Intel/OutOfBandManagement/". They have to add the path to PACKAGES_PATH. You also have to skip this dependence in the RedfishPkg.yaml to avoid the CI error.

Another thing is I propose to move out IpmiFeaturePkg from edk2-platforms/Features/Intel/OutOfBandManagement to edk2-platforms/Features/ManageabilityPkg  that also provides the implementation of PLDM/MCTP/IPMI/KCS.  I had an initial talk with IpmiFeaturePkg owner and get the positive response on this proposal. I will kick off the discussion on the dev mailing list. That is to say this module may need a little bit change later, however that is good to me having this implementation now.
Thanks
Abner
> +
> +[LibraryClasses]
> +  UefiLib
> +  DebugLib
> +  IpmiBaseLib
> +  MemoryAllocationLib
> +  BaseMemoryLib
> +
> +[Pcd]
> +  gIpmiFeaturePkgTokenSpaceGuid.PcdIpmiFeatureEnable
> +
> +[Depex]
> +  TRUE
> --
> 2.17.1