From 5e128ed1209335fb72fe50a68640331e354cbea6 Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Wed, 20 Jan 2021 20:41:15 +0100 Subject: [PATCH] CMP: Fix total_timeout behavior; small doc and diagnostic improvements Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/14019) --- apps/cmp.c | 31 +++++++++------- crypto/cmp/cmp_client.c | 71 ++++++++++++++++++------------------- crypto/cmp/cmp_msg.c | 4 +++ doc/man1/openssl-cmp.pod.in | 16 ++++----- 4 files changed, 64 insertions(+), 58 deletions(-) diff --git a/apps/cmp.c b/apps/cmp.c index 887ec5d22e..5778fd95a7 100644 --- 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 985a474c48..00c5256013 100644 --- 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 36256b3d1d..8514336801 100644 --- 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 9800de6465..dcb3ceedac 100644 --- a/doc/man1/openssl-cmp.pod.in +++ b/doc/man1/openssl-cmp.pod.in @@ -218,9 +218,9 @@ initialized to the PKI hierarchy. B requests issuing an additional certificate similarly to B but using PKCS#10 CSR format. -B requests a (key) update for an existing, given certificate. +B requests a (key) update for an existing certificate. -B requests revocation of an existing, given certificate. +B requests revocation of an existing certificate. B requests information using a General Message, where optionally included Bs 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 PKCS#10 CSR in PEM or DER format containing a certificate request. -When used with a with B<-cmd> I used directly in a legacy P10CR message. -When used with B<-cmd> I, I, or I, it is tranformed into the +With B<-cmd> I it is used directly in a legacy P10CR message. +When used with B<-cmd> I, I, or I, it is transformed into the respective regular CMP request. -It may also be used with B<-cmd> I to specifiy the certificate to be revoked +It may also be used with B<-cmd> I to specify the certificate to be revoked via the included subject and public key. =item B<-out_trusted> I|I @@ -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. +For KUR the certificate to be updated defaults to B<-cert>, +and the resulting certificate is called I. 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.