Define X509V*_VERSION constants.

The X509 version APIs confuse everyone, including our own tests
(v3name_test.cc was using the wrong value). Introduce constants. Also
document which version to use for each type. It is extra confusing that,
although certificates and CRLs use the same Version enum, RFC5280
defines X.509 v3 certificates with X.509 v2 CRLs. (There are no v3
CRLs.)

I'll send a similar patch to OpenSSL to keep upstream aligned.

Change-Id: If096138eb004156d43df87a6d74f695b04f67480
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/45944
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/x509/x509_cmp.c

@@ -383,7 +383,7 @@ int X509_chain_check_suiteb(int *perror_depth, X509 *x, STACK_OF(X509) *chain,
     } else
         i = 0;
 
-    if (X509_get_version(x) != 2) {
+    if (X509_get_version(x) != X509V3_VERSION) {
         rv = X509_V_ERR_SUITE_B_INVALID_VERSION;
         /* Correct error depth */
         i = 0;
@@ -401,7 +401,7 @@ int X509_chain_check_suiteb(int *perror_depth, X509 *x, STACK_OF(X509) *chain,
     for (; i < sk_X509_num(chain); i++) {
         sign_nid = X509_get_signature_nid(x);
         x = sk_X509_value(chain, i);
-        if (X509_get_version(x) != 2) {
+        if (X509_get_version(x) != X509V3_VERSION) {
             rv = X509_V_ERR_SUITE_B_INVALID_VERSION;
             goto end;
         }

crypto/x509/x509_set.c

@@ -64,7 +64,7 @@ long X509_get_version(const X509 *x509)
 {
     // The default version is v1(0).
     if (x509->cert_info->version == NULL) {
-        return 0;
+        return X509V1_VERSION;
     }
     return ASN1_INTEGER_get(x509->cert_info->version);
 }
@@ -76,6 +76,7 @@ X509_CINF *X509_get_cert_info(const X509 *x509)
 
 int X509_set_version(X509 *x, long version)
 {
+    // TODO(davidben): Reject invalid version numbers.
     if (x == NULL)
         return (0);
     if (version == 0) {
@@ -90,7 +91,7 @@ int X509_set_version(X509 *x, long version)
     return (ASN1_INTEGER_set(x->cert_info->version, version));
 }
 
-int X509_set_serialNumber(X509 *x, ASN1_INTEGER *serial)
+int X509_set_serialNumber(X509 *x, const ASN1_INTEGER *serial)
 {
     ASN1_INTEGER *in;
 

crypto/x509/x509_test.cc

@@ -1599,7 +1599,7 @@ TEST(X509Test, RSASignManual) {
     if (new_cert) {
       cert.reset(X509_new());
       // Fill in some fields for the certificate arbitrarily.
-      EXPECT_TRUE(X509_set_version(cert.get(), 2 /* X.509v3 */));
+      EXPECT_TRUE(X509_set_version(cert.get(), X509V3_VERSION));
       EXPECT_TRUE(ASN1_INTEGER_set(X509_get_serialNumber(cert.get()), 1));
       EXPECT_TRUE(X509_gmtime_adj(X509_getm_notBefore(cert.get()), 0));
       EXPECT_TRUE(

crypto/x509/x509_vfy.c

@@ -2052,7 +2052,7 @@ X509_CRL *X509_CRL_diff(X509_CRL *base, X509_CRL *newer,
     }
     /* Create new CRL */
     crl = X509_CRL_new();
-    if (!crl || !X509_CRL_set_version(crl, 1))
+    if (!crl || !X509_CRL_set_version(crl, X509V2_VERSION))
         goto memerr;
     /* Set issuer name */
     if (!X509_CRL_set_issuer_name(crl, X509_CRL_get_issuer(newer)))

crypto/x509v3/v3_purp.c

@@ -440,7 +440,7 @@ int x509v3_cache_extensions(X509 *x)
     if (!X509_digest(x, EVP_sha1(), x->sha1_hash, NULL))
         x->ex_flags |= EXFLAG_INVALID;
     /* V1 should mean no extensions ... */
-    if (!X509_get_version(x))
+    if (X509_get_version(x) == X509V1_VERSION)
         x->ex_flags |= EXFLAG_V1;
     /* Handle basic constraints */
     if ((bs = X509_get_ext_d2i(x, NID_basic_constraints, &j, NULL))) {

crypto/x509v3/v3name_test.cc

@@ -305,7 +305,7 @@ static X509 *make_cert(void)
     crt = X509_new();
     if (crt == NULL)
         goto out;
-    if (!X509_set_version(crt, 3))
+    if (!X509_set_version(crt, X509V3_VERSION))
         goto out;
     ret = crt;
     crt = NULL;

include/openssl/x509.h

@@ -475,16 +475,34 @@ extern "C" {
 // it is safe to call mutating functions is a little tricky due to various
 // internal caches.
 
-// X509_get_version returns the numerical value of |x509|'s version. That is,
+// The following constants are version numbers of X.509-related structures. Note
-// it returns zero for X.509v1, one for X.509v2, and two for X.509v3. Unknown
+// APIs typically return the numerical value of X.509 versions, which are one
-// versions are rejected by the parser, but a manually-created |X509| object may
+// less than the named version.
-// encode invalid versions. In that case, the function will return the invalid
+#define X509V1_VERSION 0
-// version, or -1 on overflow.
+#define X509V2_VERSION 1
+#define X509V3_VERSION 2
+
+// X509_get_version returns the numerical value of |x509|'s version. Callers may
+// compare the result to the |X509V*_VERSION| constants. Unknown versions are
+// rejected by the parser, but a manually-created |X509| object may encode
+// invalid versions. In that case, the function will return the invalid version,
+// or -1 on overflow.
 OPENSSL_EXPORT long X509_get_version(const X509 *x509);
 
+// X509_set_version sets |x509|'s version to |version|, which should be one of
+// the |X509V*_VERSION| constants. It returns one on success and zero on error.
+//
+// If unsure, use |X509V3_VERSION|.
+OPENSSL_EXPORT int X509_set_version(X509 *x509, long version);
+
 // X509_get0_serialNumber returns |x509|'s serial number.
 OPENSSL_EXPORT const ASN1_INTEGER *X509_get0_serialNumber(const X509 *x509);
 
+// X509_set_serialNumber sets |x509|'s serial number to |serial|. It returns one
+// on success and zero on error.
+OPENSSL_EXPORT int X509_set_serialNumber(X509 *x509,
+                                         const ASN1_INTEGER *serial);
+
 // X509_get0_notBefore returns |x509|'s notBefore time.
 OPENSSL_EXPORT const ASN1_TIME *X509_get0_notBefore(const X509 *x509);
 
@@ -550,9 +568,9 @@ OPENSSL_EXPORT X509_CINF *X509_get_cert_info(const X509 *x509);
 // |EXFLAG_INVALID| bit.
 OPENSSL_EXPORT long X509_get_pathlen(X509 *x509);
 
-// X509_REQ_get_version returns the numerical value of |req|'s version. That is,
+// X509_REQ_get_version returns the numerical value of |req|'s version. Callers
-// it returns zero for a v1 request. If |req| is invalid, it may return another
+// may compare the result to |X509V*_VERSION| constants. If |req| is invalid, it
-// value, or -1 on overflow.
+// may return another value, or -1 on overflow.
 OPENSSL_EXPORT long X509_REQ_get_version(const X509_REQ *req);
 
 // X509_REQ_get_subject_name returns |req|'s subject name. Note this function is
@@ -565,9 +583,9 @@ OPENSSL_EXPORT X509_NAME *X509_REQ_get_subject_name(const X509_REQ *req);
 // X509_name_cmp is a legacy alias for |X509_NAME_cmp|.
 #define X509_name_cmp(a, b) X509_NAME_cmp((a), (b))
 
-// X509_REQ_get_version returns the numerical value of |crl|'s version. That is,
+// X509_CRL_get_version returns the numerical value of |crl|'s version. Callers
-// it returns zero for a v1 CRL and one for a v2 CRL. If |crl| is invalid, it
+// may compare the result to |X509V*_VERSION| constants. If |crl| is invalid,
-// may return another value, or -1 on overflow.
+// it may return another value, or -1 on overflow.
 OPENSSL_EXPORT long X509_CRL_get_version(const X509_CRL *crl);
 
 // X509_CRL_get0_lastUpdate returns |crl|'s lastUpdate time.
@@ -1071,8 +1089,6 @@ OPENSSL_EXPORT int ASN1_item_sign_ctx(const ASN1_ITEM *it, X509_ALGOR *algor1,
                                       ASN1_BIT_STRING *signature, void *asn,
                                       EVP_MD_CTX *ctx);
 
-OPENSSL_EXPORT int X509_set_version(X509 *x, long version);
-OPENSSL_EXPORT int X509_set_serialNumber(X509 *x, ASN1_INTEGER *serial);
 OPENSSL_EXPORT ASN1_INTEGER *X509_get_serialNumber(X509 *x);
 OPENSSL_EXPORT int X509_set_issuer_name(X509 *x, X509_NAME *name);
 OPENSSL_EXPORT X509_NAME *X509_get_issuer_name(const X509 *a);
@@ -1085,7 +1101,11 @@ OPENSSL_EXPORT const STACK_OF(X509_EXTENSION) *X509_get0_extensions(
     const X509 *x);
 OPENSSL_EXPORT const X509_ALGOR *X509_get0_tbs_sigalg(const X509 *x);
 
-OPENSSL_EXPORT int X509_REQ_set_version(X509_REQ *x, long version);
+// X509_REQ_set_version sets |req|'s version to |version|, which should be
+// |X509V1_VERSION|. It returns one on success and zero on error.
+//
+// Note no versions other than |X509V1_VERSION| are defined for CSRs.
+OPENSSL_EXPORT int X509_REQ_set_version(X509_REQ *req, long version);
 OPENSSL_EXPORT int X509_REQ_set_subject_name(X509_REQ *req, X509_NAME *name);
 OPENSSL_EXPORT void X509_REQ_get0_signature(const X509_REQ *req,
                                             const ASN1_BIT_STRING **psig,
@@ -1123,7 +1143,13 @@ OPENSSL_EXPORT int X509_REQ_add1_attr_by_txt(X509_REQ *req,
                                              const unsigned char *bytes,
                                              int len);
 
-OPENSSL_EXPORT int X509_CRL_set_version(X509_CRL *x, long version);
+// X509_CRL_set_version sets |crl|'s version to |version|, which should be one
+// of the |X509V*_VERSION| constants. It returns one on success and zero on
+// error.
+//
+// If unsure, use |X509V2_VERSION|. Note |X509V3_VERSION| is not defined for
+// CRLs.
+OPENSSL_EXPORT int X509_CRL_set_version(X509_CRL *crl, long version);
 OPENSSL_EXPORT int X509_CRL_set_issuer_name(X509_CRL *x, X509_NAME *name);
 OPENSSL_EXPORT int X509_CRL_sort(X509_CRL *crl);
 OPENSSL_EXPORT int X509_CRL_up_ref(X509_CRL *crl);