CMP: Fix total_timeout behavior; small doc and diagnostic improvements

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14019)

diff --git a/apps/cmp.c b/apps/cmp.c
index 887ec5d22ef7766c10a59f68322e9b6a0ba6e61e..5778fd95a77293f8905f959ab2d6047f4ce702b2 100644 (file)
--- a/apps/cmp.c
+++ b/apps/cmp.c
@@ -697,12 +697,13 @@ static void warn_cert_msg(const char *uri, X509 *cert, const char *msg)
 
 static void warn_cert(const char *uri, X509 *cert, int warn_EE)
 {
+    uint32_t ex_flags = X509_get_extension_flags(cert);
     int res = X509_cmp_timeframe(vpm, X509_get0_notBefore(cert),
                                  X509_get0_notAfter(cert));
 
     if (res != 0)
         warn_cert_msg(uri, cert, res > 0 ? "has expired" : "not yet valid");
-    if (warn_EE && (X509_get_extension_flags(cert) & EXFLAG_CA) == 0)
+    if (warn_EE && (ex_flags & EXFLAG_V1) == 0 && (ex_flags & EXFLAG_CA) == 0)
         warn_cert_msg(uri, cert, "is not a CA cert");
 }
 
@@ -788,14 +789,14 @@ static int write_PKIMESSAGE(const OSSL_CMP_MSG *msg, char **filenames)
         return 0;
     }
     if (*filenames == NULL) {
-        CMP_err("Not enough file names provided for writing PKIMessage");
+        CMP_err("not enough file names provided for writing PKIMessage");
         return 0;
     }
 
     file = *filenames;
     *filenames = next_item(file);
     if (OSSL_CMP_MSG_write(file, msg) < 0) {
-        CMP_err1("Cannot write PKIMessage to file '%s'", file);
+        CMP_err1("cannot write PKIMessage to file '%s'", file);
         return 0;
     }
     return 1;
@@ -812,7 +813,7 @@ static OSSL_CMP_MSG *read_PKIMESSAGE(char **filenames)
         return NULL;
     }
     if (*filenames == NULL) {
-        CMP_err("Not enough file names provided for reading PKIMessage");
+        CMP_err("not enough file names provided for reading PKIMessage");
         return NULL;
     }
 
@@ -821,7 +822,7 @@ static OSSL_CMP_MSG *read_PKIMESSAGE(char **filenames)
 
     ret = OSSL_CMP_MSG_read(file);
     if (ret == NULL)
-        CMP_err1("Cannot read PKIMessage from file '%s'", file);
+        CMP_err1("cannot read PKIMessage from file '%s'", file);
     return ret;
 }
 
@@ -1654,9 +1655,9 @@ static int setup_request_ctx(OSSL_CMP_CTX *ctx, ENGINE *engine)
 
     if (opt_csr != NULL) {
         if (opt_cmd == CMP_GENM) {
-            CMP_warn("-csr option is ignored for genm command");
+            CMP_warn("-csr option is ignored for command 'genm'");
         } else {
-            csr = load_csr_autofmt(opt_csr, "PKCS#10 CSR for p10cr");
+            csr = load_csr_autofmt(opt_csr, "PKCS#10 CSR");
             if (csr == NULL)
                 return 0;
             if (!OSSL_CMP_CTX_set1_p10CSR(ctx, csr)) {
@@ -1737,10 +1738,14 @@ static int setup_request_ctx(OSSL_CMP_CTX *ctx, ENGINE *engine)
 
     if (opt_oldcert != NULL) {
         if (opt_cmd == CMP_GENM) {
-            CMP_warn("-oldcert option is ignored for genm command");
+            CMP_warn("-oldcert option is ignored for command 'genm'");
         } else {
             X509 *oldcert = load_cert_pwd(opt_oldcert, opt_keypass,
-                                          "certificate to be updated/revoked");
+                                          opt_cmd == CMP_KUR ?
+                                          "certificate to be updated" :
+                                          opt_cmd == CMP_RR ?
+                                          "certificate to be revoked" :
+                                          "reference certificate (oldcert)");
             /* opt_keypass needed if opt_oldcert is an encrypted PKCS#12 file */
 
             if (oldcert == NULL)
@@ -1892,7 +1897,7 @@ static int setup_client_ctx(OSSL_CMP_CTX *ctx, ENGINE *engine)
         char *ref_cert = opt_oldcert != NULL ? opt_oldcert : opt_cert;
 
         if (ref_cert == NULL && opt_csr == NULL) {
-            CMP_err("missing -oldcert or -csr option for certificate to be updated");
+            CMP_err("missing -oldcert for certificate to be updated and no fallback -csr given");
             goto err;
         }
         if (opt_subject != NULL)
@@ -1901,11 +1906,11 @@ static int setup_client_ctx(OSSL_CMP_CTX *ctx, ENGINE *engine)
     }
     if (opt_cmd == CMP_RR) {
         if (opt_oldcert == NULL && opt_csr == NULL) {
-            CMP_err("missing certificate to be revoked and no fallback -csr given");
+            CMP_err("missing -oldcert for certificate to be revoked and no fallback -csr given");
             goto err;
         }
         if (opt_oldcert != NULL && opt_csr != NULL)
-            CMP_warn("Ignoring -csr since certificate to be revoked is given");
+            CMP_warn("ignoring -csr since certificate to be revoked is given");
     }
     if (opt_cmd == CMP_P10CR && opt_csr == NULL) {
         CMP_err("missing PKCS#10 CSR for p10cr");
@@ -2787,7 +2792,7 @@ int cmp_main(int argc, char **argv)
             if (req != NULL) {
                 if (strcmp(path, "") != 0 && strcmp(path, "pkix/") != 0) {
                     (void)http_server_send_status(cbio, 404, "Not Found");
-                    CMP_err1("Expecting empty path or 'pkix/' but got '%s'",
+                    CMP_err1("expecting empty path or 'pkix/' but got '%s'",
                              path);
                     OPENSSL_free(path);
                     OSSL_CMP_MSG_free(req);

diff --git a/crypto/cmp/cmp_client.c b/crypto/cmp/cmp_client.c
index 985a474c48290682c3b6ea1680a42c915b250733..00c5256013cef5d30477433686cc26909bb672e8 100644 (file)
--- a/crypto/cmp/cmp_client.c
+++ b/crypto/cmp/cmp_client.c
@@ -129,6 +129,9 @@ static int save_statusInfo(OSSL_CMP_CTX *ctx, OSSL_CMP_PKISI *si)
 static int send_receive_check(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *req,
                               OSSL_CMP_MSG **rep, int expected_type)
 {
+    int is_enrollment = IS_CREP(expected_type)
+        || expected_type == OSSL_CMP_PKIBODY_POLLREP
+        || expected_type == OSSL_CMP_PKIBODY_PKICONF;
     const char *req_type_str =
         ossl_cmp_bodytype_to_string(ossl_cmp_msg_get_bodytype(req));
     const char *expected_type_str = ossl_cmp_bodytype_to_string(expected_type);
@@ -143,14 +146,13 @@ static int send_receive_check(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *req,
 
     *rep = NULL;
     msg_timeout = ctx->msg_timeout; /* backup original value */
-    if ((IS_CREP(expected_type) || expected_type == OSSL_CMP_PKIBODY_POLLREP)
-            && ctx->total_timeout > 0 /* timeout is not infinite */) {
+    if (is_enrollment && ctx->total_timeout > 0 /* timeout is not infinite */) {
         if (now >= ctx->end_time) {
             ERR_raise(ERR_LIB_CMP, CMP_R_TOTAL_TIMEOUT);
             return 0;
         }
         if (!ossl_assert(ctx->end_time - time(NULL) < INT_MAX)) {
-            /* cannot really happen due to the assignment in do_certreq_seq() */
+            /* actually cannot happen due to assignment in initial_certreq() */
             ERR_raise(ERR_LIB_CMP, CMP_R_INVALID_ARGS);
             return 0;
         }
@@ -168,7 +170,9 @@ static int send_receive_check(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *req,
     ctx->msg_timeout = msg_timeout; /* restore original value */
 
     if (*rep == NULL) {
-        ERR_raise_data(ERR_LIB_CMP, CMP_R_TRANSFER_ERROR, /* or receiving response */
+        ERR_raise_data(ERR_LIB_CMP,
+                       ctx->total_timeout > 0 && time(NULL) >= ctx->end_time ?
+                       CMP_R_TOTAL_TIMEOUT : CMP_R_TRANSFER_ERROR,
                        "request sent: %s, expected response: %s",
                        req_type_str, expected_type_str);
         return 0;
@@ -641,10 +645,32 @@ static int cert_response(OSSL_CMP_CTX *ctx, int sleep, int rid,
     return ret;
 }
 
+static int initial_certreq(OSSL_CMP_CTX *ctx,
+                           int req_type, const OSSL_CRMF_MSG *crm,
+                           OSSL_CMP_MSG **p_rep, int rep_type)
+{
+    OSSL_CMP_MSG *req;
+    int res;
+
+    ctx->status = -1;
+    if (!ossl_cmp_ctx_set0_newCert(ctx, NULL))
+        return 0;
+
+    if (ctx->total_timeout > 0) /* else ctx->end_time is not used */
+        ctx->end_time = time(NULL) + ctx->total_timeout;
+
+    /* also checks if all necessary options are set */
+    if ((req = ossl_cmp_certreq_new(ctx, req_type, crm)) == NULL)
+        return 0;
+
+    res = send_receive_check(ctx, req, p_rep, rep_type);
+    OSSL_CMP_MSG_free(req);
+    return res;
+}
+
 int OSSL_CMP_try_certreq(OSSL_CMP_CTX *ctx, int req_type,
                          const OSSL_CRMF_MSG *crm, int *checkAfter)
 {
-    OSSL_CMP_MSG *req = NULL;
     OSSL_CMP_MSG *rep = NULL;
     int is_p10 = req_type == OSSL_CMP_PKIBODY_P10CR;
     int rid = is_p10 ? -1 : OSSL_CMP_CERTREQID;
@@ -657,18 +683,7 @@ int OSSL_CMP_try_certreq(OSSL_CMP_CTX *ctx, int req_type,
     }
 
     if (ctx->status != OSSL_CMP_PKISTATUS_waiting) { /* not polling already */
-        ctx->status = -1;
-        if (!ossl_cmp_ctx_set0_newCert(ctx, NULL))
-            return 0;
-
-        if (ctx->total_timeout > 0) /* else ctx->end_time is not used */
-            ctx->end_time = time(NULL) + ctx->total_timeout;
-
-        req = ossl_cmp_certreq_new(ctx, req_type, crm);
-        if (req == NULL) /* also checks if all necessary options are set */
-            return 0;
-
-        if (!send_receive_check(ctx, req, &rep, rep_type))
+        if (!initial_certreq(ctx, req_type, crm, &rep, rep_type))
             goto err;
     } else {
         if (req_type < 0)
@@ -684,7 +699,6 @@ int OSSL_CMP_try_certreq(OSSL_CMP_CTX *ctx, int req_type,
                         req_type, rep_type);
 
  err:
-    OSSL_CMP_MSG_free(req);
     OSSL_CMP_MSG_free(rep);
     return res;
 }
@@ -701,7 +715,6 @@ X509 *OSSL_CMP_exec_certreq(OSSL_CMP_CTX *ctx, int req_type,
                             const OSSL_CRMF_MSG *crm)
 {
 
-    OSSL_CMP_MSG *req = NULL;
     OSSL_CMP_MSG *rep = NULL;
     int is_p10 = req_type == OSSL_CMP_PKIBODY_P10CR;
     int rid = is_p10 ? -1 : OSSL_CMP_CERTREQID;
@@ -712,23 +725,8 @@ X509 *OSSL_CMP_exec_certreq(OSSL_CMP_CTX *ctx, int req_type,
         ERR_raise(ERR_LIB_CMP, CMP_R_NULL_ARGUMENT);
         return NULL;
     }
-    if (is_p10 && crm != NULL) {
-        ERR_raise(ERR_LIB_CMP, CMP_R_INVALID_ARGS);
-        return NULL;
-    }
-
-    ctx->status = -1;
-    if (!ossl_cmp_ctx_set0_newCert(ctx, NULL))
-        return NULL;
-
-    if (ctx->total_timeout > 0) /* else ctx->end_time is not used */
-        ctx->end_time = time(NULL) + ctx->total_timeout;
 
-    /* OSSL_CMP_certreq_new() also checks if all necessary options are set */
-    if ((req = ossl_cmp_certreq_new(ctx, req_type, crm)) == NULL)
-        goto err;
-
-    if (!send_receive_check(ctx, req, &rep, rep_type))
+    if (!initial_certreq(ctx, req_type, crm, &rep, rep_type))
         goto err;
 
     if (cert_response(ctx, 1 /* sleep */, rid, &rep, NULL, req_type, rep_type)
@@ -737,7 +735,6 @@ X509 *OSSL_CMP_exec_certreq(OSSL_CMP_CTX *ctx, int req_type,
 
     result = ctx->newCert;
  err:
-    OSSL_CMP_MSG_free(req);
     OSSL_CMP_MSG_free(rep);
     return result;
 }
@@ -818,7 +815,7 @@ int OSSL_CMP_exec_RR_ses(OSSL_CMP_CTX *ctx)
         goto err;
     }
 
-    /* check any pretent CertId in optional revCerts field */
+    /* check any present CertId in optional revCerts field */
     if (sk_OSSL_CRMF_CERTID_num(rrep->revCerts) >= 1) {
         OSSL_CRMF_CERTID *cid;
         OSSL_CRMF_CERTTEMPLATE *tmpl =

diff --git a/crypto/cmp/cmp_msg.c b/crypto/cmp/cmp_msg.c
index 36256b3d1d9372b4654ab77826a5a9b59e0adb71..85143368012b4b69a699710fe697398939c41314 100644 (file)
--- a/crypto/cmp/cmp_msg.c
+++ b/crypto/cmp/cmp_msg.c
@@ -352,6 +352,10 @@ OSSL_CMP_MSG *ossl_cmp_certreq_new(OSSL_CMP_CTX *ctx, int type,
         ERR_raise(ERR_LIB_CMP, CMP_R_INVALID_ARGS);
         return NULL;
     }
+    if (type == OSSL_CMP_PKIBODY_P10CR && crm != NULL) {
+        ERR_raise(ERR_LIB_CMP, CMP_R_INVALID_ARGS);
+        return NULL;
+    }
 
     if ((msg = ossl_cmp_msg_create(ctx, type)) == NULL)
         goto err;

diff --git a/doc/man1/openssl-cmp.pod.in b/doc/man1/openssl-cmp.pod.in
index 9800de6465b8aaa17dfe2ae0993eabbbd4213234..dcb3ceedacaee24ba5230c39182f9bc06ebee5af 100644 (file)
--- a/doc/man1/openssl-cmp.pod.in
+++ b/doc/man1/openssl-cmp.pod.in
@@ -218,9 +218,9 @@ initialized to the PKI hierarchy.
 B<p10cr> requests issuing an additional certificate similarly to B<cr>
 but using PKCS#10 CSR format.
 
-B<kur> requests a (key) update for an existing, given certificate.
+B<kur> requests a (key) update for an existing certificate.
 
-B<rr> requests revocation of an existing, given certificate.
+B<rr> requests revocation of an existing certificate.
 
 B<genm> requests information using a General Message, where optionally
 included B<InfoTypeAndValue>s may be used to state which info is of interest.
@@ -344,10 +344,10 @@ is provided via the B<-newkey> or B<-key> options.
 =item B<-csr> I<filename>
 
 PKCS#10 CSR in PEM or DER format containing a certificate request.
-When used with a with B<-cmd> I<p10cr> used directly in a legacy P10CR message.
-When used with B<-cmd> I<ir>, I<cr>, or I<kur>, it is tranformed into the
+With B<-cmd> I<p10cr> it is used directly in a legacy P10CR message.
+When used with B<-cmd> I<ir>, I<cr>, or I<kur>, it is transformed into the
 respective regular CMP request.
-It may also be used with B<-cmd> I<rr> to specifiy the certificate to be revoked
+It may also be used with B<-cmd> I<rr> to specify the certificate to be revoked
 via the included subject and public key.
 
 =item B<-out_trusted> I<filenames>|I<uris>
@@ -392,12 +392,12 @@ The file where the chain of the newly enrolled certificate should be saved.
 The certificate to be updated (i.e., renewed or re-keyed) in Key Update Request
 (KUR) messages or to be revoked in Revocation Request (RR) messages.
 For RR the certificate to be revoked can also be specified using B<-csr>.
-For KUR certificate to be updated defaults to B<-cert>, and the resulting certificate is called
-I<reference certificate>.
+For KUR the certificate to be updated defaults to B<-cert>,
+and the resulting certificate is called I<reference certificate>.
 
 The reference certificate, if any, is also used for
 deriving default subject DN and Subject Alternative Names and the
-default issuer entry in the requested certificate template of a IR/CR/KUR.
+default issuer entry in the requested certificate template of an IR/CR/KUR.
 Its subject is used as sender of outgoing messages if B<-cert> is not given.
 Its issuer is used as default recipient in CMP message headers
 if neither B<-recipient>, B<-srvcert>, nor B<-issuer> is given.