Repository: randombit/botan
Pull request #1995 (2019-06-12 08:11:29)

add option to specify ocsp max age
This PR adds the ability to specify a maximum OCSP age to the `PKIX` module by adding it as a default parameter and to `Path_Validation_Restrictions`.

codecov-io: # [Codecov](https://codecov.io/gh/randombit/botan/pull/1995?src=pr&el=h1) Report
> Merging [#1995](https://codecov.io/gh/randombit/botan/pull/1995?src=pr&el=desc) into [master](https://codecov.io/gh/randombit/botan/commit/f032968e6637a2897ee66473d181ac5b0d7f3c5c?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `75%`.

[![Impacted file tree graph](https://codecov.io/gh/randombit/botan/pull/1995/graphs/tree.svg?width=650&token=VR6iYmSPH8&height=150&src=pr)](https://codecov.io/gh/randombit/botan/pull/1995?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1995      +/-   ##
==========================================
+ Coverage    92.3%   92.31%   +<.01%     
==========================================
  Files         537      537              
  Lines       57932    57933       +1     
  Branches     6152     6153       +1     
==========================================
+ Hits        53476    53481       +5     
+ Misses       4456     4452       -4
```


| [Impacted Files](https://codecov.io/gh/randombit/botan/pull/1995?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/lib/x509/x509path.cpp](https://codecov.io/gh/randombit/botan/pull/1995/diff?src=pr&el=tree#diff-c3JjL2xpYi94NTA5L3g1MDlwYXRoLmNwcA==) | `77.13% <75%> (+0.05%)` | :arrow_up: |
| [src/lib/misc/cryptobox/cryptobox.cpp](https://codecov.io/gh/randombit/botan/pull/1995/diff?src=pr&el=tree#diff-c3JjL2xpYi9taXNjL2NyeXB0b2JveC9jcnlwdG9ib3guY3Bw) | `93.65% <0%> (-1.59%)` | :arrow_down: |
| [src/lib/pubkey/elgamal/elgamal.cpp](https://codecov.io/gh/randombit/botan/pull/1995/diff?src=pr&el=tree#diff-c3JjL2xpYi9wdWJrZXkvZWxnYW1hbC9lbGdhbWFsLmNwcA==) | `95.38% <0%> (-1.54%)` | :arrow_down: |
| [src/lib/pbkdf/bcrypt\_pbkdf/bcrypt\_pbkdf.cpp](https://codecov.io/gh/randombit/botan/pull/1995/diff?src=pr&el=tree#diff-c3JjL2xpYi9wYmtkZi9iY3J5cHRfcGJrZGYvYmNyeXB0X3Bia2RmLmNwcA==) | `91.66% <0%> (ø)` | :arrow_up: |
| [src/bogo\_shim/bogo\_shim.cpp](https://codecov.io/gh/randombit/botan/pull/1995/diff?src=pr&el=tree#diff-c3JjL2JvZ29fc2hpbS9ib2dvX3NoaW0uY3Bw) | `88.76% <0%> (+1.1%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/randombit/botan/pull/1995?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/randombit/botan/pull/1995?src=pr&el=footer). Last update [f032968...799b40c](https://codecov.io/gh/randombit/botan/pull/1995?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

randombit: Couple minor things otherwise lgtm
randombit: Oh just realized, no tests. Needs a test.
alnkpa: adapted to comments. tests don't work, yet
alnkpa: fixed tests. should be ready for review, now.

Patch:
diff --git a/src/lib/x509/x509path.cpp b/src/lib/x509/x509path.cpp
index a889aa4d4..d0604e1c6 100644
--- a/src/lib/x509/x509path.cpp
+++ b/src/lib/x509/x509path.cpp
@@ -205,7 +205,7 @@ PKIX::check_ocsp(const std::vector<std::shared_ptr<const X509_Certificate>>& cer
                  const std::vector<std::shared_ptr<const OCSP::Response>>& ocsp_responses,
                  const std::vector<Certificate_Store*>& trusted_certstores,
                  std::chrono::system_clock::time_point ref_time,
-                 std::chrono::seconds max_age)
+                 std::chrono::seconds max_ocsp_age)
    {
    if(cert_path.empty())
       throw Invalid_Argument("PKIX::check_ocsp cert_path empty");
@@ -228,7 +228,7 @@ PKIX::check_ocsp(const std::vector<std::shared_ptr<const X509_Certificate>>& cer
             if(ocsp_signature_status == Certificate_Status_Code::OCSP_SIGNATURE_OK)
                {
                // Signature ok, so check the claimed status
-               Certificate_Status_Code ocsp_status = ocsp_responses.at(i)->status_for(*ca, *subject, ref_time, max_age);
+               Certificate_Status_Code ocsp_status = ocsp_responses.at(i)->status_for(*ca, *subject, ref_time, max_ocsp_age);
                status.insert(ocsp_status);
                }
             else
@@ -351,7 +351,8 @@ PKIX::check_ocsp_online(const std::vector<std::shared_ptr<const X509_Certificate
                         const std::vector<Certificate_Store*>& trusted_certstores,
                         std::chrono::system_clock::time_point ref_time,
                         std::chrono::milliseconds timeout,
-                        bool ocsp_check_intermediate_CAs)
+                        bool ocsp_check_intermediate_CAs,
+                        std::chrono::seconds max_ocsp_age)
    {
    if(cert_path.empty())
       throw Invalid_Argument("PKIX::check_ocsp_online cert_path empty");
@@ -410,7 +411,7 @@ PKIX::check_ocsp_online(const std::vector<std::shared_ptr<const X509_Certificate
       ocsp_responses.push_back(ocsp_response_futures[i].get());
       }
 
-   return PKIX::check_ocsp(cert_path, ocsp_responses, trusted_certstores, ref_time);
+   return PKIX::check_ocsp(cert_path, ocsp_responses, trusted_certstores, ref_time, max_ocsp_age);
    }
 
 CertificatePathStatusCodes
@@ -876,7 +877,7 @@ Path_Validation_Result x509_path_validate(
 
       if(ocsp_resp.size() > 0)
          {
-         ocsp_status = PKIX::check_ocsp(cert_path, ocsp_resp, trusted_roots, ref_time);
+         ocsp_status = PKIX::check_ocsp(cert_path, ocsp_resp, trusted_roots, ref_time, restrictions.max_ocsp_age());
          }
 
       if(ocsp_status.empty() && ocsp_timeout != std::chrono::milliseconds(0))
@@ -958,14 +959,16 @@ Path_Validation_Result x509_path_validate(
    }
 
 Path_Validation_Restrictions::Path_Validation_Restrictions(bool require_rev,
-                                                           size_t key_strength,
-                                                           bool ocsp_intermediates) :
+      size_t key_strength,
+      bool ocsp_intermediates,
+      std::chrono::seconds max_ocsp_age) :
    m_require_revocation_information(require_rev),
    m_ocsp_all_intermediates(ocsp_intermediates),
-   m_minimum_key_strength(key_strength)
+   m_minimum_key_strength(key_strength),
+   m_max_ocsp_age(max_ocsp_age)
    {
    if(key_strength <= 80)
-      m_trusted_hashes.insert("SHA-160");
+      { m_trusted_hashes.insert("SHA-160"); }
 
    m_trusted_hashes.insert("SHA-224");
    m_trusted_hashes.insert("SHA-256");
diff --git a/src/lib/x509/x509path.h b/src/lib/x509/x509path.h
index 3e457b970..ed2cbd592 100644
--- a/src/lib/x509/x509path.h
+++ b/src/lib/x509/x509path.h
@@ -47,10 +47,13 @@ class BOTAN_PUBLIC_API(2,0) Path_Validation_Restrictions final
       *        128 bit strength requires ~3k bit RSA or P-256
       * @param ocsp_all_intermediates Make OCSP requests for all CAs as
       * well as end entity (if OCSP enabled in path validation request)
+      * @param max_ocsp_age maximum age of OCSP responses w/o next_update.
+      *        If zero, there is no maximum age
       */
       Path_Validation_Restrictions(bool require_rev = false,
                                    size_t minimum_key_strength = 110,
-                                   bool ocsp_all_intermediates = false);
+                                   bool ocsp_all_intermediates = false,
+                                   std::chrono::seconds max_ocsp_age = std::chrono::seconds::zero());
 
       /**
       * @param require_rev if true, revocation information is required
@@ -62,15 +65,19 @@ class BOTAN_PUBLIC_API(2,0) Path_Validation_Restrictions final
       * @param trusted_hashes a set of trusted hashes. Any signatures
       *        created using a hash other than one of these will be
       *        rejected.
+      * @param max_ocsp_age maximum age of OCSP responses w/o next_update.
+      *        If zero, there is no maximum age
       */
       Path_Validation_Restrictions(bool require_rev,
                                    size_t minimum_key_strength,
                                    bool ocsp_all_intermediates,
-                                   const std::set<std::string>& trusted_hashes) :
+                                   const std::set<std::string>& trusted_hashes,
+                                   std::chrono::seconds max_ocsp_age = std::chrono::seconds::zero()) :
          m_require_revocation_information(require_rev),
          m_ocsp_all_intermediates(ocsp_all_intermediates),
          m_trusted_hashes(trusted_hashes),
-         m_minimum_key_strength(minimum_key_strength) {}
+         m_minimum_key_strength(minimum_key_strength),
+         m_max_ocsp_age(max_ocsp_age) {}
 
       /**
       * @return whether revocation information is required
@@ -97,11 +104,19 @@ class BOTAN_PUBLIC_API(2,0) Path_Validation_Restrictions final
       size_t minimum_key_strength() const
          { return m_minimum_key_strength; }
 
+      /**
+      * @return maximum age of OCSP responses w/o next_update.
+      * If zero, there is no maximum age
+      */
+      std::chrono::seconds max_ocsp_age() const
+         { return m_max_ocsp_age; }
+
    private:
       bool m_require_revocation_information;
       bool m_ocsp_all_intermediates;
       std::set<std::string> m_trusted_hashes;
       size_t m_minimum_key_strength;
+      std::chrono::seconds m_max_ocsp_age;
    };
 
 /**
@@ -346,14 +361,16 @@ BOTAN_PUBLIC_API(2,0) check_chain(const std::vector<std::shared_ptr<const X509_C
 * @param certstores trusted roots
 * @param ref_time whatever time you want to perform the validation against
 * (normally current system clock)
+* @param max_ocsp_age maximum age of OCSP responses w/o next_update. If zero,
+* there is no maximum age
 * @return revocation status
 */
 CertificatePathStatusCodes
-BOTAN_PUBLIC_API(2,0) 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::chrono::seconds max_age = std::chrono::seconds::zero());
+BOTAN_PUBLIC_API(2, 0) 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::chrono::seconds max_ocsp_age = std::chrono::seconds::zero());
 
 /**
 * Check CRLs for revocation information
@@ -396,14 +413,17 @@ BOTAN_PUBLIC_API(2,0) check_crl(const std::vector<std::shared_ptr<const X509_Cer
 * 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.
+* @param max_ocsp_age maximum age of OCSP responses w/o next_update. If zero,
+* there is no maximum age
 * @return revocation status
 */
 CertificatePathStatusCodes
-BOTAN_PUBLIC_API(2,0) 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);
+BOTAN_PUBLIC_API(2, 0) 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::chrono::seconds max_ocsp_age = std::chrono::seconds::zero());
 
 /**
 * Check CRL using online (HTTP) access. Current version creates a thread and


Test patch:
diff --git a/src/tests/test_x509_path.cpp b/src/tests/test_x509_path.cpp
index 71118ab2f..8715f3f54 100644
--- a/src/tests/test_x509_path.cpp
+++ b/src/tests/test_x509_path.cpp
@@ -714,6 +714,187 @@ std::vector<Test::Result> BSI_Path_Validation_Tests::run()
 
 BOTAN_REGISTER_TEST("x509_path_bsi", BSI_Path_Validation_Tests);
 
+class Path_Validation_With_OCSP_Tests final : public Test
+   {
+   public:
+      Botan::X509_Certificate load_test_X509_cert(const std::string& path)
+         {
+         return Botan::X509_Certificate(Test::data_file(path));
+         }
+
+      std::shared_ptr<const Botan::OCSP::Response> load_test_OCSP_resp(const std::string& path)
+         {
+         return std::make_shared<const Botan::OCSP::Response>(Test::read_binary_data_file(path));
+         }
+
+      Test::Result validate_with_ocsp_with_next_update_without_max_age()
+         {
+         Test::Result result("path check with ocsp with next_update w/o max_age");
+         Botan::Certificate_Store_In_Memory trusted;
+
+         auto restrictions = Botan::Path_Validation_Restrictions(false, 110, false);
+
+         auto ee = load_test_X509_cert("x509/ocsp/randombit.pem");
+         auto ca = load_test_X509_cert("x509/ocsp/letsencrypt.pem");
+         auto trust_root = load_test_X509_cert("x509/ocsp/identrust.pem");
+         trusted.add_certificate(trust_root);
+
+         const std::vector<Botan::X509_Certificate> cert_path = { ee, ca, trust_root };
+
+         std::shared_ptr<const Botan::OCSP::Response> ocsp = load_test_OCSP_resp("x509/ocsp/randombit_ocsp.der");
+
+         auto check_path = [&](const std::chrono::system_clock::time_point valid_time,
+                               const Botan::Certificate_Status_Code expected)
+            {
+            const auto path_result = Botan::x509_path_validate(cert_path, restrictions, trusted, "", Botan::Usage_Type::UNSPECIFIED,
+                                     valid_time, std::chrono::milliseconds(0), {ocsp});
+
+            return result.confirm(std::string("Status: '") + Botan::to_string(expected)
+                                  + "' should match '" + Botan::to_string(path_result.result()) + "'",
+                                  path_result.result()==expected);
+            };
+
+         check_path(Botan::calendar_point(2016, 11, 11, 12, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OCSP_NOT_YET_VALID);
+         check_path(Botan::calendar_point(2016, 11, 18, 12, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OK);
+         check_path(Botan::calendar_point(2016, 11, 20, 8, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OK);
+         check_path(Botan::calendar_point(2016, 11, 28, 8, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OCSP_HAS_EXPIRED);
+
+         return result;
+         }
+
+      Test::Result validate_with_ocsp_with_next_update_with_max_age()
+         {
+         Test::Result result("path check with ocsp with next_update with max_age");
+         Botan::Certificate_Store_In_Memory trusted;
+
+         auto restrictions = Botan::Path_Validation_Restrictions(false, 110, false,
+                            std::chrono::minutes(59));
+
+         auto ee = load_test_X509_cert("x509/ocsp/randombit.pem");
+         auto ca = load_test_X509_cert("x509/ocsp/letsencrypt.pem");
+         auto trust_root = load_test_X509_cert("x509/ocsp/identrust.pem");
+         trusted.add_certificate(trust_root);
+
+         const std::vector<Botan::X509_Certificate> cert_path = { ee, ca, trust_root };
+
+         std::shared_ptr<const Botan::OCSP::Response> ocsp = load_test_OCSP_resp("x509/ocsp/randombit_ocsp.der");
+
+         auto check_path = [&](const std::chrono::system_clock::time_point valid_time,
+                               const Botan::Certificate_Status_Code expected)
+            {
+            const auto path_result = Botan::x509_path_validate(cert_path, restrictions, trusted, "", Botan::Usage_Type::UNSPECIFIED,
+                                     valid_time, std::chrono::milliseconds(0), {ocsp});
+
+            return result.confirm(std::string("Status: '") + Botan::to_string(expected)
+                                  + "' should match '" + Botan::to_string(path_result.result()) + "'",
+                                  path_result.result()==expected);
+            };
+
+         check_path(Botan::calendar_point(2016, 11, 11, 12, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OCSP_NOT_YET_VALID);
+         check_path(Botan::calendar_point(2016, 11, 18, 12, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OK);
+         check_path(Botan::calendar_point(2016, 11, 20, 8, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OK);
+         check_path(Botan::calendar_point(2016, 11, 28, 8, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OCSP_HAS_EXPIRED);
+
+         return result;
+         }
+
+      Test::Result validate_with_ocsp_without_next_update_without_max_age()
+         {
+         Test::Result result("path check with ocsp w/o next_update w/o max_age");
+         Botan::Certificate_Store_In_Memory trusted;
+
+         auto restrictions = Botan::Path_Validation_Restrictions(false, 110, false);
+
+         auto ee = load_test_X509_cert("x509/ocsp/patrickschmidt.pem");
+         auto ca = load_test_X509_cert("x509/ocsp/bdrive_encryption.pem");
+         auto trust_root = load_test_X509_cert("x509/ocsp/bdrive_root.pem");
+
+         trusted.add_certificate(trust_root);
+
+         const std::vector<Botan::X509_Certificate> cert_path = { ee, ca, trust_root };
+
+         std::shared_ptr<const Botan::OCSP::Response> ocsp = load_test_OCSP_resp("x509/ocsp/patrickschmidt_ocsp.der");
+
+         auto check_path = [&](const std::chrono::system_clock::time_point valid_time,
+                               const Botan::Certificate_Status_Code expected)
+            {
+            const auto path_result = Botan::x509_path_validate(cert_path, restrictions, trusted, "", Botan::Usage_Type::UNSPECIFIED,
+                                     valid_time, std::chrono::milliseconds(0), {ocsp});
+
+            return result.confirm(std::string("Status: '") + Botan::to_string(expected)
+                                  + "' should match '" + Botan::to_string(path_result.result()) + "'",
+                                  path_result.result()==expected);
+            };
+
+         check_path(Botan::calendar_point(2019, 5, 28, 7, 0, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OCSP_NOT_YET_VALID);
+         check_path(Botan::calendar_point(2019, 5, 28, 7, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OK);
+         check_path(Botan::calendar_point(2019, 5, 28, 8, 0, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OK);
+
+         return result;
+         }
+
+      Test::Result validate_with_ocsp_without_next_update_with_max_age()
+         {
+         Test::Result result("path check with ocsp w/o next_update with max_age");
+         Botan::Certificate_Store_In_Memory trusted;
+
+         auto restrictions = Botan::Path_Validation_Restrictions(false, 110, false,
+                            std::chrono::minutes(59));
+
+         auto ee = load_test_X509_cert("x509/ocsp/patrickschmidt.pem");
+         auto ca = load_test_X509_cert("x509/ocsp/bdrive_encryption.pem");
+         auto trust_root = load_test_X509_cert("x509/ocsp/bdrive_root.pem");
+
+         trusted.add_certificate(trust_root);
+
+         const std::vector<Botan::X509_Certificate> cert_path = { ee, ca, trust_root };
+
+         std::shared_ptr<const Botan::OCSP::Response> ocsp = load_test_OCSP_resp("x509/ocsp/patrickschmidt_ocsp.der");
+
+         auto check_path = [&](const std::chrono::system_clock::time_point valid_time,
+                               const Botan::Certificate_Status_Code expected)
+            {
+            const auto path_result = Botan::x509_path_validate(cert_path, restrictions, trusted, "", Botan::Usage_Type::UNSPECIFIED,
+                                     valid_time, std::chrono::milliseconds(0), {ocsp});
+
+            return result.confirm(std::string("Status: '") + Botan::to_string(expected)
+                                  + "' should match '" + Botan::to_string(path_result.result()) + "'",
+                                  path_result.result()==expected);
+            };
+
+         check_path(Botan::calendar_point(2019, 5, 28, 7, 0, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OCSP_NOT_YET_VALID);
+         check_path(Botan::calendar_point(2019, 5, 28, 7, 30, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OK);
+         check_path(Botan::calendar_point(2019, 5, 28, 8, 0, 0).to_std_timepoint(),
+                    Botan::Certificate_Status_Code::OCSP_IS_TOO_OLD);
+
+         return result;
+         }
+
+      std::vector<Test::Result> run() override
+         {
+         return  {validate_with_ocsp_with_next_update_without_max_age(),
+                  validate_with_ocsp_with_next_update_with_max_age(),
+                  validate_with_ocsp_without_next_update_without_max_age(),
+                  validate_with_ocsp_without_next_update_with_max_age()};
+         }
+
+   };
+
+BOTAN_REGISTER_TEST("x509_path_with_ocsp", Path_Validation_With_OCSP_Tests);
+
 #endif
 
 }