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-boun...@lists.01.org] On Behalf Of Laszlo Ersek Sent: Wednesday, September 20, 2017 8:09 PM To: Long, Qin <qin.l...@intel.com>; Ye, Ting <ting...@intel.com>; Zhang, Chao B <chao.b.zh...@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...@intel.com<mailto:ting...@intel.com>> > Cc: Chao Zhang <chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Qin Long <qin.l...@intel.com<mailto:qin.l...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
