Thanks for your review comments, Abner! I will update new version patch later. The CI build error will be handled together.
> please add Igor as reviewer too Sure! > + *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. Yes, the additional one byte is for NULL terminator. USERNAME_MAX_LENGTH is defined as 16 and follow host interface specification. Regards, Nickle -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chang, Abner via groups.io Sent: Saturday, October 22, 2022 3:01 PM To: Nickle Wang <nick...@nvidia.com>; devel@edk2.groups.io Cc: Nick Ramirez <nrami...@nvidia.com>; Igor Kulchytskyy <ig...@ami.com> Subject: Re: [edk2-devel] [PATCH] RedfishPkg/RedfishPlatformCredentialLib: IPMI implementation External email: Use caution opening links or attachments [AMD Official Use Only - General] Hi Nickle, please add Igor as reviewer too. My comments is in below, > -----Original Message----- > From: Nickle Wang <nick...@nvidia.com> > Sent: Thursday, October 20, 2022 10:55 AM > To: devel@edk2.groups.io > Cc: Chang, Abner <abner.ch...@amd.com>; Nick Ramirez > <nrami...@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.ch...@amd.com> > Cc: Nick Ramirez <nrami...@nvidia.com> > Signed-off-by: Nickle Wang <nick...@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/RedfishPlatformCredent > ialLib.i > nf > > diff --git > a/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCrede > ntialLi > b.c > b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCrede > ntialLi > b.c > new file mode 100644 > index 0000000000..23a15ab1fa > --- /dev/null > +++ b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformC > +++ re > +++ 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/RedfishPlatformCrede > ntialLi > b.h > b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCrede > ntialLi > b.h > new file mode 100644 > index 0000000000..5b448e01be > --- /dev/null > +++ b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformC > +++ re > +++ 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/RedfishPlatformCrede > ntialLi > b.inf > b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformCrede > ntialLi > b.inf > new file mode 100644 > index 0000000000..a990d28363 > --- /dev/null > +++ b/RedfishPkg/Library/RedfishPlatformCredentialLib/RedfishPlatformC > +++ re > +++ 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#95552): https://edk2.groups.io/g/devel/message/95552 Mute This Topic: https://groups.io/mt/94446537/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-