Address review comments from @cordney

Primarily doc updates but also expose some more logic in PKIX namespace,
overall_status and merge_revocation_status. This allows calling more or less all
of the logic used by the monolitic x509_path_validate in any way needed by an
application.

Add Certificate_Store_In_Memory::add_crl variant taking shared_ptr

Add optional Certificate_Store_In_Memory* pointer to check_crl_online,
valid CRLs are saved there.

5 files changed, 249 insertions, 114 deletions

diff --git a/src/lib/x509/certstor.cpp b/src/lib/x509/certstor.cpp
index 51abf640a..1f7275675 100644
--- a/src/lib/x509/certstor.cpp
+++ b/src/lib/x509/certstor.cpp
@@ -89,21 +89,27 @@ Certificate_Store_In_Memory::find_cert_by_pubkey_sha1(const std::vector<byte>& k
 
 void Certificate_Store_In_Memory::add_crl(const X509_CRL& crl)
    {
-   X509_DN crl_issuer = crl.issuer_dn();
+   std::shared_ptr<const X509_CRL> crl_s = std::make_shared<const X509_CRL>(crl);
+   return add_crl(crl_s);
+   }
+
+void Certificate_Store_In_Memory::add_crl(std::shared_ptr<const X509_CRL> crl)
+   {
+   X509_DN crl_issuer = crl->issuer_dn();
 
    for(size_t i = 0; i != m_crls.size(); ++i)
       {
       // Found an update of a previously existing one; replace it
       if(m_crls[i]->issuer_dn() == crl_issuer)
          {
-         if(m_crls[i]->this_update() <= crl.this_update())
-            m_crls[i] = std::make_shared<X509_CRL>(crl);
+         if(m_crls[i]->this_update() <= crl->this_update())
+            m_crls[i] = crl;
          return;
          }
       }
 
    // Totally new CRL, add to the list
-   m_crls.push_back(std::make_shared<X509_CRL>(crl));
+   m_crls.push_back(crl);
    }
 
 std::shared_ptr<const X509_CRL> Certificate_Store_In_Memory::find_crl_for(const X509_Certificate& subject) const
diff --git a/src/lib/x509/certstor.h b/src/lib/x509/certstor.h
index 07f02dfd2..ba71334c5 100644
--- a/src/lib/x509/certstor.h
+++ b/src/lib/x509/certstor.h
@@ -100,6 +100,12 @@ class BOTAN_DLL Certificate_Store_In_Memory : public Certificate_Store
       void add_crl(const X509_CRL& crl);
 
       /**
+      * Add a certificate revocation list (CRL) to the store as a shared_ptr
+      * @param crl CRL to be added
+      */
+      void add_crl(std::shared_ptr<const X509_CRL> crl);
+
+      /**
       * @return DNs for all certificates managed by the store
       */
       std::vector<X509_DN> all_subjects() const override;
diff --git a/src/lib/x509/ocsp.h b/src/lib/x509/ocsp.h
index b86b6a5e4..64e86b82f 100644
--- a/src/lib/x509/ocsp.h
+++ b/src/lib/x509/ocsp.h
@@ -73,24 +73,32 @@ class BOTAN_DLL Response
 
       /**
       * Parses an OCSP response.
-      * @param request the OCSP request this is a respone to
       * @param response_bits response bits received
       */
       Response(const std::vector<byte>& response_bits);
 
-      /*
+      /**
       * Check signature and return status
       * The optional cert_path is the (already validated!) certificate path of
       * the end entity which is being inquired about
+      * @param trust_roots list of certstores containing trusted roots
+      * @param cert_path optionally, the (already verified!) certificate path for the certificate
+      * this is an OCSP response for. This is necessary to find the correct intermediate CA in
+      * some cases.
       */
       Certificate_Status_Code check_signature(const std::vector<Certificate_Store*>& trust_roots,
                                               const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path = {}) const;
 
-      /*
+      /**
       * Verify that issuer's key signed this response
+      * @param issuer certificate of issuer
+      * @return if signature valid OCSP_SIGNATURE_OK else an error code
       */
       Certificate_Status_Code verify_signature(const X509_Certificate& issuer) const;
 
+      /**
+      * @return the time this OCSP response was supposedly produced at
+      */
       const X509_Time& produced_at() const { return m_produced_at; }
 
       /**
@@ -107,6 +115,7 @@ class BOTAN_DLL Response
        * Searches the OCSP response for issuer and subject certificate.
        * @param issuer issuer certificate
        * @param subject subject certificate
+       * @param ref_time the reference time
        * @return OCSP status code, possible values:
        *         CERT_IS_REVOKED,
        *         OCSP_NOT_YET_VALID,
diff --git a/src/lib/x509/x509path.cpp b/src/lib/x509/x509path.cpp
index d966978fa..8a34ee4db 100644
--- a/src/lib/x509/x509path.cpp
+++ b/src/lib/x509/x509path.cpp
@@ -321,6 +321,7 @@ PKIX::check_ocsp_online(const std::vector<std::shared_ptr<const X509_Certificate
 std::vector<std::set<Certificate_Status_Code>>
 PKIX::check_crl_online(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
                        const std::vector<Certificate_Store*>& certstores,
+                       Certificate_Store_In_Memory* crl_store,
                        std::chrono::system_clock::time_point ref_time,
                        std::chrono::milliseconds timeout)
    {
@@ -391,7 +392,22 @@ PKIX::check_crl_online(const std::vector<std::shared_ptr<const X509_Certificate>
          }
       }
 
-   return PKIX::check_crl(cert_path, crls, ref_time);
+   const std::vector<std::set<Certificate_Status_Code>> crl_status = PKIX::check_crl(cert_path, crls, ref_time);
+
+   if(crl_store)
+      {
+      for(size_t i = 0; i != crl_status.size(); ++i)
+         {
+         if(crl_status[i].count(Certificate_Status_Code::VALID_CRL_CHECKED))
+            {
+            // better be non-null, we supposedly validated it
+            BOTAN_ASSERT_NONNULL(crls[i]);
+            crl_store->add_crl(crls[i]);
+            }
+         }
+      }
+
+   return crl_status;
    }
 
 #endif
@@ -473,15 +489,16 @@ PKIX::build_certificate_path(std::vector<std::shared_ptr<const X509_Certificate>
       }
    }
 
-namespace {
-
-void merge_results(std::vector<std::set<Certificate_Status_Code>>& results,
-                   const std::vector<std::set<Certificate_Status_Code>>& crl,
-                   const std::vector<std::set<Certificate_Status_Code>>& ocsp,
-                   bool require_rev_on_end_entity,
-                   bool require_rev_on_intermediates)
+void PKIX::merge_revocation_status(std::vector<std::set<Certificate_Status_Code>>& chain_status,
+                                   const std::vector<std::set<Certificate_Status_Code>>& crl,
+                                   const std::vector<std::set<Certificate_Status_Code>>& ocsp,
+                                   bool require_rev_on_end_entity,
+                                   bool require_rev_on_intermediates)
    {
-   for(size_t i = 0; i != results.size() - 1; ++i)
+   if(chain_status.empty())
+      throw Invalid_Argument("PKIX::merge_revocation_status chain_status was empty");
+
+   for(size_t i = 0; i != chain_status.size() - 1; ++i)
       {
       bool had_crl = false, had_ocsp = false;
 
@@ -493,7 +510,7 @@ void merge_results(std::vector<std::set<Certificate_Status_Code>>& results,
                {
                had_crl = true;
                }
-            results[i].insert(code);
+            chain_status[i].insert(code);
             }
          }
 
@@ -506,7 +523,7 @@ void merge_results(std::vector<std::set<Certificate_Status_Code>>& results,
                had_ocsp = true;
                }
 
-            results[i].insert(code);
+            chain_status[i].insert(code);
             }
          }
 
@@ -515,13 +532,34 @@ void merge_results(std::vector<std::set<Certificate_Status_Code>>& results,
          if((require_rev_on_end_entity && i == 0) ||
             (require_rev_on_intermediates && i > 0))
             {
-            results[i].insert(Certificate_Status_Code::NO_REVOCATION_DATA);
+            chain_status[i].insert(Certificate_Status_Code::NO_REVOCATION_DATA);
             }
          }
       }
    }
 
-}
+Certificate_Status_Code PKIX::overall_status(const std::vector<std::set<Certificate_Status_Code>>& cert_status)
+   {
+   if(cert_status.empty())
+      throw Invalid_Argument("PKIX::overall_status empty cert status");
+
+   Certificate_Status_Code overall_status = Certificate_Status_Code::OK;
+
+   // take the "worst" error as overall
+   for(const std::set<Certificate_Status_Code>& s : cert_status)
+      {
+      if(!s.empty())
+         {
+         auto worst = *s.rbegin();
+         // Leave informative OCSP/CRL confirmations on cert-level status only
+         if(worst >= Certificate_Status_Code::FIRST_ERROR_STATUS && worst > overall_status)
+            {
+            overall_status = worst;
+            }
+         }
+      }
+   return overall_status;
+   }
 
 Path_Validation_Result BOTAN_DLL x509_path_validate(
    const std::vector<X509_Certificate>& end_certs,
@@ -546,6 +584,7 @@ Path_Validation_Result BOTAN_DLL x509_path_validate(
    Certificate_Status_Code path_building_result =
       PKIX::build_certificate_path(cert_path, trusted_roots, end_entity, end_entity_extra);
 
+   // If we cannot successfully build a chain to a trusted self-signed root, stop now
    if(path_building_result != Certificate_Status_Code::OK)
       {
       return Path_Validation_Result(path_building_result);
@@ -557,9 +596,6 @@ Path_Validation_Result BOTAN_DLL x509_path_validate(
                         restrictions.minimum_key_strength(),
                         restrictions.trusted_hashes());
 
-   if(path_building_result != Certificate_Status_Code::OK)
-      status[0].insert(path_building_result);
-
    std::vector<std::set<Certificate_Status_Code>> crl_status =
       PKIX::check_crl(cert_path, trusted_roots, ref_time);
 
@@ -576,9 +612,9 @@ Path_Validation_Result BOTAN_DLL x509_path_validate(
 #endif
       }
 
-   merge_results(status, crl_status, ocsp_status,
-                 restrictions.require_revocation_information(),
-                 restrictions.ocsp_all_intermediates());
+   PKIX::merge_revocation_status(status, crl_status, ocsp_status,
+                                 restrictions.require_revocation_information(),
+                                 restrictions.ocsp_all_intermediates());
 
    return Path_Validation_Result(status, std::move(cert_path));
    }
@@ -648,23 +684,10 @@ Path_Validation_Restrictions::Path_Validation_Restrictions(bool require_rev,
 
 Path_Validation_Result::Path_Validation_Result(std::vector<std::set<Certificate_Status_Code>> status,
                                                std::vector<std::shared_ptr<const X509_Certificate>>&& cert_chain) :
-   m_overall(Certificate_Status_Code::OK),
    m_all_status(status),
-   m_cert_path(cert_chain)
+   m_cert_path(cert_chain),
+   m_overall(PKIX::overall_status(m_all_status))
    {
-   // take the "worst" error as overall
-   for(const auto& s : m_all_status)
-      {
-      if(!s.empty())
-         {
-         auto worst = *s.rbegin();
-         // Leave informative OCSP/CRL confirmations on cert-level status only
-         if(worst >= Certificate_Status_Code::FIRST_ERROR_STATUS)
-            {
-            m_overall = worst;
-            }
-         }
-      }
    }
 
 const X509_Certificate& Path_Validation_Result::trust_root() const
diff --git a/src/lib/x509/x509path.h b/src/lib/x509/x509path.h
index 6b544dba3..e8d90b4a9 100644
--- a/src/lib/x509/x509path.h
+++ b/src/lib/x509/x509path.h
@@ -34,6 +34,9 @@ class BOTAN_DLL Path_Validation_Restrictions
       *        operations, eg 80 means 2^80) of a signature. Signatures
       *        weaker than this are rejected. If more than 80, SHA-1
       *        signatures are also rejected.
+      *        80 bit strength requires 1024 bit RSA
+      *        110 bit strength requires 2048 bit RSA
+      *        Using 128 requires ECC (P-256) or ~3000 bit RSA keys.
       * @param ocsp_all_intermediates Make OCSP requests for all CAs as
       * well as end entity (if OCSP enabled in path validation request)
       */
@@ -68,7 +71,8 @@ class BOTAN_DLL Path_Validation_Restrictions
          { return m_require_revocation_information; }
 
       /**
-      * FIXME add doc
+      * @return whether all intermediate CAs should also be OCSPed. If false
+      * then only end entity OCSP is required/requested.
       */
       bool ocsp_all_intermediates() const
          { return m_ocsp_all_intermediates; }
@@ -160,82 +164,11 @@ class BOTAN_DLL Path_Validation_Result
       explicit Path_Validation_Result(Certificate_Status_Code status) : m_overall(status) {}
 
    private:
-      Certificate_Status_Code m_overall;
       std::vector<std::set<Certificate_Status_Code>> m_all_status;
       std::vector<std::shared_ptr<const X509_Certificate>> m_cert_path;
+      Certificate_Status_Code m_overall;
    };
 
-namespace PKIX {
-
-/**
-* Build certificate path
-* @param cert_path_out output parameter, cert_path will be appended to this vector
-* @param trusted_certstores list of certificate stores that contain trusted certificates
-* @param end_entity the cert to be validated
-* @param end_entity_extra optional list of additional untrusted certs for path building
-* @return result of the path building operation (OK or error)
-*/
-Certificate_Status_Code
-BOTAN_DLL build_certificate_path(std::vector<std::shared_ptr<const X509_Certificate>>& cert_path_out,
-                                 const std::vector<Certificate_Store*>& trusted_certstores,
-                                 const std::shared_ptr<const X509_Certificate>& end_entity,
-                                 const std::vector<std::shared_ptr<const X509_Certificate>>& end_entity_extra);
-
-/**
-* Perform certificate validation
-* @param cert_path path built by build_certificate_path with OK result
-* @param ref_time whatever time you want to perform the validation
-* against (normally current system clock)
-* @param hostname the hostname
-* @param usage end entity usage checks
-* @param min_signature_algo_strength 80 or 128 typically
-* @param trusted_hashes set of trusted hash functions,
-* empty means accept any hash we have an OID for
-*/
-std::vector<std::set<Certificate_Status_Code>>
-BOTAN_DLL check_chain(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
-                      std::chrono::system_clock::time_point ref_time,
-                      const std::string& hostname,
-                      Usage_Type usage,
-                      size_t min_signature_algo_strength,
-                      const std::set<std::string>& trusted_hashes);
-
-std::vector<std::set<Certificate_Status_Code>>
-BOTAN_DLL check_ocsp(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
-                     const std::vector<std::shared_ptr<const OCSP::Response>>& ocsp_responses,
-                     const std::vector<Certificate_Store*>& certstores,
-                     std::chrono::system_clock::time_point ref_time);
-
-std::vector<std::set<Certificate_Status_Code>>
-BOTAN_DLL check_crl(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
-                    const std::vector<std::shared_ptr<const X509_CRL>>& crls,
-                    std::chrono::system_clock::time_point ref_time);
-
-std::vector<std::set<Certificate_Status_Code>>
-BOTAN_DLL check_crl(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
-                    const std::vector<Certificate_Store*>& certstores,
-                    std::chrono::system_clock::time_point ref_time);
-
-#if defined(BOTAN_HAS_ONLINE_REVOCATION_CHECKS)
-
-std::vector<std::set<Certificate_Status_Code>>
-BOTAN_DLL check_ocsp_online(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
-                            const std::vector<Certificate_Store*>& trusted_certstores,
-                            std::chrono::system_clock::time_point ref_time,
-                            std::chrono::milliseconds timeout,
-                            bool ocsp_check_intermediate_CAs);
-
-std::vector<std::set<Certificate_Status_Code>>
-BOTAN_DLL check_crl_online(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
-                           const std::vector<Certificate_Store*>& trusted_certstores,
-                           std::chrono::system_clock::time_point ref_time,
-                           std::chrono::milliseconds timeout);
-
-#endif
-
-}
-
-
 /**
 * PKIX Path Validation
 * @param end_certs certificate chain to validate
@@ -316,6 +249,164 @@ Path_Validation_Result BOTAN_DLL x509_path_validate(
    std::chrono::system_clock::time_point validation_time = std::chrono::system_clock::now(),
    std::chrono::milliseconds ocsp_timeout = std::chrono::milliseconds(0));
 
+
+/**
+* namespace PKIX holds the building blocks that are called by x509_path_validate.
+* This allows custom validation logic to be written by applications and makes
+* for easier testing, but unless you're positive you know what you're doing you
+* probably want to just call x509_path_validate instead.
+*/
+namespace PKIX {
+
+/**
+* Build certificate path
+* @param cert_path_out output parameter, cert_path will be appended to this vector
+* @param trusted_certstores list of certificate stores that contain trusted certificates
+* @param end_entity the cert to be validated
+* @param end_entity_extra optional list of additional untrusted certs for path building
+* @return result of the path building operation (OK or error)
+*/
+Certificate_Status_Code
+BOTAN_DLL build_certificate_path(std::vector<std::shared_ptr<const X509_Certificate>>& cert_path_out,
+                                 const std::vector<Certificate_Store*>& trusted_certstores,
+                                 const std::shared_ptr<const X509_Certificate>& end_entity,
+                                 const std::vector<std::shared_ptr<const X509_Certificate>>& end_entity_extra);
+
+/**
+* Check the certificate chain, but not any revocation data
+*
+* @param cert_path path built by build_certificate_path with OK result
+* @param ref_time whatever time you want to perform the validation
+* against (normally current system clock)
+* @param hostname the hostname
+* @param usage end entity usage checks
+* @param min_signature_algo_strength 80 or 110 typically
+* Note 80 allows 1024 bit RSA and SHA-1. 110 allows 2048 bit RSA and SHA-2.
+* Using 128 requires ECC (P-256) or ~3000 bit RSA keys.
+* @param trusted_hashes set of trusted hash functions, empty means accept any
+* hash we have an OID for
+* @return vector of results on per certificate in the path, each containing a set of
+* results. If all codes in the set are < Certificate_Status_Code::FIRST_ERROR_STATUS,
+* then the result for that certificate is successful. If all results are
+*/
+std::vector<std::set<Certificate_Status_Code>>
+BOTAN_DLL check_chain(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
+                      std::chrono::system_clock::time_point ref_time,
+                      const std::string& hostname,
+                      Usage_Type usage,
+                      size_t min_signature_algo_strength,
+                      const std::set<std::string>& trusted_hashes);
+
+/**
+* Check OCSP responses for revocation information
+* @param cert_path path already validated by check_chain
+* @param ocsp_responses the OCSP responses to consider
+* @param certstores trusted roots
+* @param ref_time whatever time you want to perform the validation against
+* (normally current system clock)
+* @return revocation status
+*/
+std::vector<std::set<Certificate_Status_Code>>
+BOTAN_DLL check_ocsp(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
+                     const std::vector<std::shared_ptr<const OCSP::Response>>& ocsp_responses,
+                     const std::vector<Certificate_Store*>& certstores,
+                     std::chrono::system_clock::time_point ref_time);
+
+/**
+* Check CRLs for revocation infomration
+* @param cert_path path already validated by check_chain
+* @param crls the list of CRLs to check, it is assumed that crls[i] (if not null)
+* is the associated CRL for the subject in cert_path[i].
+* @param ref_time whatever time you want to perform the validation against
+* (normally current system clock)
+* @return revocation status
+*/
+std::vector<std::set<Certificate_Status_Code>>
+BOTAN_DLL check_crl(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
+                    const std::vector<std::shared_ptr<const X509_CRL>>& crls,
+                    std::chrono::system_clock::time_point ref_time);
+
+/**
+* Check CRLs for revocation infomration
+* @param cert_path path already validated by check_chain
+* @param certstores a list of certificate stores to query for the CRL
+* @param ref_time whatever time you want to perform the validation against
+* (normally current system clock)
+* @return revocation status
+*/
+std::vector<std::set<Certificate_Status_Code>>
+BOTAN_DLL check_crl(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
+                    const std::vector<Certificate_Store*>& certstores,
+                    std::chrono::system_clock::time_point ref_time);
+
+#if defined(BOTAN_HAS_ONLINE_REVOCATION_CHECKS)
+
+/**
+* Check OCSP using online (HTTP) access. Current version creates a thread and
+* network connection per OCSP request made.
+*
+* @param cert_path path already validated by check_chain
+* @param trusted_certstores a list of certstores with trusted certs
+* @param ref_time whatever time you want to perform the validation against
+* (normally current system clock)
+* @param timeout for timing out the responses, though actually this function
+* may block for up to timeout*cert_path.size()*C for some small C.
+* @param ocsp_check_intermediate_CAs if true also performs OCSP on any intermediate
+* CA certificates. If false, only does OCSP on the end entity cert.
+* @return revocation status
+*/
+std::vector<std::set<Certificate_Status_Code>>
+BOTAN_DLL check_ocsp_online(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
+                            const std::vector<Certificate_Store*>& trusted_certstores,
+                            std::chrono::system_clock::time_point ref_time,
+                            std::chrono::milliseconds timeout,
+                            bool ocsp_check_intermediate_CAs);
+
+/**
+* Check CRL using online (HTTP) access. Current version creates a thread and
+* network connection per CRL access.
+
+* @param cert_path path already validated by check_chain
+* @param trusted_certstores a list of certstores with trusted certs
+* @param certstore_to_recv_crls optional (nullptr to disable), all CRLs
+* retreived will be saved to this cert store.
+* @param ref_time whatever time you want to perform the validation against
+* (normally current system clock)
+* @param timeout for timing out the responses, though actually this function
+* may block for up to timeout*cert_path.size()*C for some small C.
+* @return revocation status
+*/
+std::vector<std::set<Certificate_Status_Code>>
+BOTAN_DLL check_crl_online(const std::vector<std::shared_ptr<const X509_Certificate>>& cert_path,
+                           const std::vector<Certificate_Store*>& trusted_certstores,
+                           Certificate_Store_In_Memory* certstore_to_recv_crls,
+                           std::chrono::system_clock::time_point ref_time,
+                           std::chrono::milliseconds timeout);
+
+#endif
+
+/**
+* Find overall status (OK, error) of a validation
+* @param cert_status result of merge_revocation_status or check_chain
+*/
+Certificate_Status_Code BOTAN_DLL overall_status(const std::vector<std::set<Certificate_Status_Code>>& cert_status);
+
+/**
+* Merge the results from CRL and/or OCSP checks into chain_status
+* @param chain_status the certificate status
+* @param crl_status results from check_crl
+* @param ocsp_status results from check_ocsp
+* @param require_rev_on_end_entity require valid CRL or OCSP on end-entity cert
+* @param require_rev_on_intermediates require valid CRL or OCSP on all intermediate certificates
+*/
+void BOTAN_DLL merge_revocation_status(std::vector<std::set<Certificate_Status_Code>>& chain_status,
+                                       const std::vector<std::set<Certificate_Status_Code>>& crl_status,
+                                       const std::vector<std::set<Certificate_Status_Code>>& ocsp_status,
+                                       bool require_rev_on_end_entity,
+                                       bool require_rev_on_intermediates);
+
+}
+
 }
 
 #endif