Re: [PATCH] CryptoPkg: Add new API to retrieve commonName of X.509 certificate

[Laszlo Ersek <lersek@redhat.com>]

Hello Qin,

On 09/19/17 05:38, Long Qin wrote:
> Add one new API (X509GetCommonName()) to retrieve the subject commonName
> string from one X.509 certificate.
> 
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@intel.com>
> ---
>  CryptoPkg/Application/Cryptest/RsaVerify2.c        | 17 ++++
>  CryptoPkg/Include/Library/BaseCryptLib.h           | 32 ++++++++
>  CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c      | 93 ++++++++++++++++++++++
>  CryptoPkg/Library/BaseCryptLib/Pk/CryptX509Null.c  | 32 ++++++++
>  .../Pk/CryptX509Null.c                             | 34 +++++++-
>  5 files changed, 207 insertions(+), 1 deletion(-)

> diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
> index 9c5ffcd9cf..d861be6725 100644
> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> @@ -2171,6 +2171,38 @@ X509GetSubjectName (
>    IN OUT  UINTN        *SubjectSize
>    );
>  
> +/**
> +  Retrieve the common name (CN) string from one X.509 certificate.
> +
> +  If Cert or CommonNameSize is NULL, then return FALSE.
> +  If this interface is not supported, then return FALSE.
> +
> +  @param[in]      Cert            Pointer to the DER-encoded X509 certificate.
> +  @param[in]      CertSize        Size of the X509 certificate in bytes.
> +  @param[out]     CommonName      Buffer to contain the retrieved certificate common
> +                                  name string. At most CommonNameSize bytes will be
> +                                  written and the string will be null terminated. May be
> +                                  NULL in order to determine the size buffer needed.
> +  @param[in,out]  CommonNameSize  The size in bytes of the CommonName buffer on input,
> +                                  and the size of buffer returned CommonName on output.
> +                                  if CommonName is NULL then the amount of space needed
> +                                  in buffer (including the final null) is returned.
> +
> +  @retval  TRUE   The certificate CommonName retrieved successfully.
> +  @retval  FALSE  Invalid certificate, or CommonNameSize is NULL,
> +                  or no CommonName entry exists.
> +  @retval  FALSE  This interface is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +X509GetCommonName (
> +  IN      CONST UINT8  *Cert,
> +  IN      UINTN        CertSize,
> +  OUT     CHAR8        *CommonName,
> +  IN OUT  UINTN        *CommonNameSize
> +  );
> +
>  /**
>    Verify one X509 certificate was issued by the trusted CA.
>  

I hope my questions / suggestions aren't unwelcome (or misguided) --
have you considered returning RETURN_STATUS from this function?

Currently FALSE is returned for several error cases, but we have good
RETURN_xxx macros for telling them apart:

- RETURN_BUFFER_TOO_SMALL: "The buffer was not large enough to hold the
requested data. The required buffer size is returned in the appropriate
parameter when this error occurs."

- RETURN_UNSUPPORTED: "The operation is not supported."

- RETURN_NOT_FOUND: "The item was not found." -- this can be used for
"no CommonName entry exists".

- RETURN_INVALID_PARAMETER: "The parameter was incorrect." -- this can
be used for "CommonNameSize is NULL", and likely for "Invalid
certificate" as well.

If you don't want to update the interface, I'm OK with that of course; I
just figured I'd raise the question.

Thanks!
Laszlo