Compensate for CMP-related TODOs removed by PR #15539

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from https://github.com/openssl/openssl/pull/16006)

9 files changed, 30 insertions, 6 deletions

diff --git a/apps/lib/cmp_mock_srv.c b/apps/lib/cmp_mock_srv.c
index d890f7fde0..7e6f99fd52 100644
--- a/apps/lib/cmp_mock_srv.c
+++ b/apps/lib/cmp_mock_srv.c
@@ -264,6 +264,7 @@ static OSSL_CMP_PKISI *process_cert_request(OSSL_CMP_SRV_CTX *srv_ctx,
 
     if (ctx->certOut != NULL
             && (*certOut = X509_dup(ctx->certOut)) == NULL)
+        /* Should better return a cert produced from data in request template */
         goto err;
     if (ctx->chainOut != NULL
             && (*chainOut = X509_chain_up_ref(ctx->chainOut)) == NULL)
@@ -371,10 +372,9 @@ static void process_error(OSSL_CMP_SRV_CTX *srv_ctx, const OSSL_CMP_MSG *error,
         for (i = 0; i < sk_ASN1_UTF8STRING_num(errorDetails); i++) {
             if (i > 0)
                 BIO_printf(bio_err, ", ");
-            BIO_printf(bio_err, "\"");
-            ASN1_STRING_print(bio_err,
-                              sk_ASN1_UTF8STRING_value(errorDetails, i));
-            BIO_printf(bio_err, "\"");
+            ASN1_STRING_print_ex(bio_err,
+                                 sk_ASN1_UTF8STRING_value(errorDetails, i),
+                                 ASN1_STRFLGS_ESC_QUOTE);
         }
         BIO_printf(bio_err, "\n");
     }
diff --git a/crypto/cmp/cmp_local.h b/crypto/cmp/cmp_local.h
index a20eeac9dc..c227d7f152 100644
--- a/crypto/cmp/cmp_local.h
+++ b/crypto/cmp/cmp_local.h
@@ -118,7 +118,7 @@ struct ossl_cmp_ctx_st {
     int revocationReason; /* revocation reason code to be included in RR */
     STACK_OF(OSSL_CMP_ITAV) *genm_ITAVs; /* content of general message */
 
-    /* result returned in responses */
+    /* result returned in responses, so far supporting only one certResponse */
     int status; /* PKIStatus of last received IP/CP/KUP/RP/error or -1 */
     OSSL_CMP_PKIFREETEXT *statusString; /* of last IP/CP/KUP/RP/error */
     int failInfoCode; /* failInfoCode of last received IP/CP/KUP/error, or -1 */
@@ -710,6 +710,7 @@ DECLARE_ASN1_FUNCTIONS(OSSL_CMP_PROTECTEDPART)
  *   }       -- or HMAC [RFC2104, RFC2202])
  */
 /*-
+ *   Not supported:
  *   id-DHBasedMac OBJECT IDENTIFIER ::= {1 2 840 113533 7 66 30}
  *   DHBMParameter ::= SEQUENCE {
  *           owf                 AlgorithmIdentifier,
diff --git a/crypto/cmp/cmp_protect.c b/crypto/cmp/cmp_protect.c
index 7ff46a6dc1..76b9e55d3d 100644
--- a/crypto/cmp/cmp_protect.c
+++ b/crypto/cmp/cmp_protect.c
@@ -242,6 +242,7 @@ int ossl_cmp_msg_protect(OSSL_CMP_CTX *ctx, OSSL_CMP_MSG *msg)
 
     /*
      * For the case of re-protection remove pre-existing protection.
+     * Does not remove any pre-existing extraCerts.
      */
     X509_ALGOR_free(msg->header->protectionAlg);
     msg->header->protectionAlg = NULL;
diff --git a/crypto/cmp/cmp_server.c b/crypto/cmp/cmp_server.c
index 83bf4ac157..a48631d267 100644
--- a/crypto/cmp/cmp_server.c
+++ b/crypto/cmp/cmp_server.c
@@ -228,6 +228,7 @@ static OSSL_CMP_MSG *process_cert_request(OSSL_CMP_SRV_CTX *srv_ctx,
     msg = ossl_cmp_certrep_new(srv_ctx->ctx, bodytype, certReqId, si,
                                certOut, NULL /* enc */, chainOut, caPubs,
                                srv_ctx->sendUnprotectedErrors);
+    /* When supporting OSSL_CRMF_POPO_KEYENC, "enc" will need to be set */
     if (msg == NULL)
         ERR_raise(ERR_LIB_CMP, CMP_R_ERROR_CREATING_CERTREP);
 
@@ -553,6 +554,7 @@ OSSL_CMP_MSG *OSSL_CMP_SRV_process_request(OSSL_CMP_SRV_CTX *srv_ctx,
             rsp = process_pollReq(srv_ctx, req);
         break;
     default:
+        /* Other request message types are not supported */
         ERR_raise(ERR_LIB_CMP, CMP_R_UNEXPECTED_PKIBODY);
         break;
     }
@@ -564,6 +566,7 @@ OSSL_CMP_MSG *OSSL_CMP_SRV_process_request(OSSL_CMP_SRV_CTX *srv_ctx,
         int flags = 0;
         unsigned long err = ERR_peek_error_data(&data, &flags);
         int fail_info = 1 << OSSL_CMP_PKIFAILUREINFO_badRequest;
+        /* fail_info is not very specific */
         OSSL_CMP_PKISI *si = NULL;
 
         if (ctx->transactionID == NULL) {
@@ -607,6 +610,8 @@ OSSL_CMP_MSG *OSSL_CMP_SRV_process_request(OSSL_CMP_SRV_CTX *srv_ctx,
     case OSSL_CMP_PKIBODY_PKICONF:
     case OSSL_CMP_PKIBODY_GENP:
     case OSSL_CMP_PKIBODY_ERROR:
+        /* Other terminating response message types are not supported */
+        /* Prepare for next transaction, ignoring any errors here: */
         (void)OSSL_CMP_CTX_set1_transactionID(ctx, NULL);
         (void)OSSL_CMP_CTX_set1_senderNonce(ctx, NULL);
         ctx->status = OSSL_CMP_PKISTATUS_unspecified; /* transaction closed */
diff --git a/crypto/cmp/cmp_vfy.c b/crypto/cmp/cmp_vfy.c
index cc71c6ce04..d41e9e742e 100644
--- a/crypto/cmp/cmp_vfy.c
+++ b/crypto/cmp/cmp_vfy.c
@@ -456,6 +456,7 @@ static int check_msg_find_cert(OSSL_CMP_CTX *ctx, const OSSL_CMP_MSG *msg)
     if (sender == NULL || msg->body == NULL)
         return 0; /* other NULL cases already have been checked */
     if (sender->type != GEN_DIRNAME) {
+        /* So far, only X509_NAME is supported */
         ERR_raise(ERR_LIB_CMP, CMP_R_SENDER_GENERALNAME_TYPE_NOT_SUPPORTED);
         return 0;
     }
diff --git a/crypto/crmf/crmf_asn.c b/crypto/crmf/crmf_asn.c
index 3a5bc3e71c..85b4213934 100644
--- a/crypto/crmf/crmf_asn.c
+++ b/crypto/crmf/crmf_asn.c
@@ -84,6 +84,7 @@ ASN1_CHOICE(OSSL_CRMF_POPOPRIVKEY) = {
     ASN1_IMP(OSSL_CRMF_POPOPRIVKEY, value.dhMAC, ASN1_BIT_STRING, 2),
     ASN1_IMP(OSSL_CRMF_POPOPRIVKEY, value.agreeMAC, OSSL_CRMF_PKMACVALUE, 3),
     ASN1_IMP(OSSL_CRMF_POPOPRIVKEY, value.encryptedKey, ASN1_NULL, 4),
+    /* When supported, ASN1_NULL needs to be replaced by CMS_ENVELOPEDDATA */
 } ASN1_CHOICE_END(OSSL_CRMF_POPOPRIVKEY)
 IMPLEMENT_ASN1_FUNCTIONS(OSSL_CRMF_POPOPRIVKEY)
 
diff --git a/crypto/crmf/crmf_lib.c b/crypto/crmf/crmf_lib.c
index 4e90cbe02c..e57192fc83 100644
--- a/crypto/crmf/crmf_lib.c
+++ b/crypto/crmf/crmf_lib.c
@@ -505,6 +505,12 @@ int OSSL_CRMF_MSGS_verify_popo(const OSSL_CRMF_MSGS *reqs,
                 ERR_raise(ERR_LIB_CRMF, CRMF_R_POPO_INCONSISTENT_PUBLIC_KEY);
                 return 0;
             }
+
+            /*
+             * Should check at this point the contents of the authInfo sub-field
+             * as requested in FR #19807 according to RFC 4211 section 4.1.
+             */
+
             it = ASN1_ITEM_rptr(OSSL_CRMF_POPOSIGNINGKEYINPUT);
             asn = sig->poposkInput;
         } else {
@@ -521,6 +527,12 @@ int OSSL_CRMF_MSGS_verify_popo(const OSSL_CRMF_MSGS *reqs,
             return 0;
         break;
     case OSSL_CRMF_POPO_KEYENC:
+        /*
+         * When OSSL_CMP_certrep_new() supports encrypted certs,
+         * should return 1 if the type of req->popo->value.keyEncipherment
+         * is OSSL_CRMF_POPOPRIVKEY_SUBSEQUENTMESSAGE and
+         * its value.subsequentMessage == OSSL_CRMF_SUBSEQUENTMESSAGE_ENCRCERT
+         */
     case OSSL_CRMF_POPO_KEYAGREE:
     default:
         ERR_raise(ERR_LIB_CRMF, CRMF_R_UNSUPPORTED_POPO_METHOD);
diff --git a/crypto/crmf/crmf_local.h b/crypto/crmf/crmf_local.h
index f8d028442b..e8937b4231 100644
--- a/crypto/crmf/crmf_local.h
+++ b/crypto/crmf/crmf_local.h
@@ -188,6 +188,7 @@ typedef struct ossl_crmf_popoprivkey_st {
         ASN1_BIT_STRING *dhMAC; /* 2 */ /* Deprecated */
         OSSL_CRMF_PKMACVALUE *agreeMAC; /* 3 */
         ASN1_NULL *encryptedKey; /* 4 */
+        /* When supported, ASN1_NULL needs to be replaced by CMS_ENVELOPEDDATA */
     } value;
 } OSSL_CRMF_POPOPRIVKEY;
 DECLARE_ASN1_FUNCTIONS(OSSL_CRMF_POPOPRIVKEY)
@@ -329,7 +330,7 @@ struct ossl_crmf_certtemplate_st {
 struct ossl_crmf_certrequest_st {
     ASN1_INTEGER *certReqId;
     OSSL_CRMF_CERTTEMPLATE *certTemplate;
-    STACK_OF(OSSL_CRMF_ATTRIBUTETYPEANDVALUE) *controls;
+    STACK_OF(OSSL_CRMF_ATTRIBUTETYPEANDVALUE /* Controls expanded */) *controls;
 } /* OSSL_CRMF_CERTREQUEST */;
 DECLARE_ASN1_FUNCTIONS(OSSL_CRMF_CERTREQUEST)
 DECLARE_ASN1_DUP_FUNCTION(OSSL_CRMF_CERTREQUEST)
diff --git a/crypto/crmf/crmf_pbm.c b/crypto/crmf/crmf_pbm.c
index ec32e30598..d4c7af38cb 100644
--- a/crypto/crmf/crmf_pbm.c
+++ b/crypto/crmf/crmf_pbm.c
@@ -123,6 +123,7 @@ OSSL_CRMF_PBMPARAMETER *OSSL_CRMF_pbmp_new(OSSL_LIB_CTX *libctx, size_t slen,
  * |outlen| if not NULL, will set variable to the length of the mac on success
  * returns 1 on success, 0 on error
  */
+/* could be combined with other MAC calculations in the library */
 int OSSL_CRMF_pbm_new(OSSL_LIB_CTX *libctx, const char *propq,
                       const OSSL_CRMF_PBMPARAMETER *pbmp,
                       const unsigned char *msg, size_t msglen,
@@ -203,6 +204,7 @@ int OSSL_CRMF_pbm_new(OSSL_LIB_CTX *libctx, const char *propq,
         ERR_raise(ERR_LIB_CRMF, CRMF_R_UNSUPPORTED_ALGORITHM);
         goto err;
     }
+    /* could be generalized to allow non-HMAC: */
     if (EVP_Q_mac(libctx, "HMAC", propq, hmac_mdname, NULL, basekey, bklen,
                   msg, msglen, mac_res, EVP_MAX_MD_SIZE, outlen) == NULL)
         goto err;