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

["Long, Qin" <qin.long@intel.com>]

Laszlo.

It's one good feedback.

This is one historical design issue. We choose to use simple BOOLEAN as the return value, because OpenSSL has complicated return data (reason) with extra api (e.g. ERR_get_error()...). It's hard to map these error messages directly, then we just used one simplest way before, and always kept this kind of API style in late updates.

I also think the return value (true/false) in current BaseCryptLib is really ambiguous to tell any more useful information. RETURN_xxx is more valuable in this new-added case. I would like to update the patch per your suggestion.

Thanks for raising this.

Best Regards & Thanks,
LONG, Qin

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Wednesday, September 20, 2017 8:09 PM
To: Long, Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] CryptoPkg: Add new API to retrieve commonName of X.509 certificate

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<mailto:ting.ye@intel.com>>
> Cc: Chao Zhang <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.long@intel.com<mailto: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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel