Revert D57737100: Move default cert flag to a separate method

Differential Revision:
D57737100

Original commit changeset: 166163ddd721

Original Phabricator Diff: D57737100

fbshipit-source-id: 9d62da7f00ab36a3250a82366dc30b4467eacf5a

fizz/experimental/ktls/test/AsyncFizzBaseKTLSTest.cpp

@@ -225,8 +225,10 @@ class OneshotRead : public folly::AsyncTransport::ReadCallback {
 static std::shared_ptr<fizz::server::FizzServerContext>
 makeTestServerContext() {
   auto certmanager = std::make_shared<fizz::server::CertManager>();
-  certmanager->addCertAndSetDefault(openssl::CertUtils::makeSelfCert(
-      fizz::test::kP256Certificate.str(), fizz::test::kP256Key.str()));
+  certmanager->addCert(
+      openssl::CertUtils::makeSelfCert(
+          fizz::test::kP256Certificate.str(), fizz::test::kP256Key.str()),
+      true);
 
   auto factory = std::make_shared<fizz::test::MockFactory>();
   auto certManager = std::make_shared<server::CertManager>();

fizz/extensions/delegatedcred/DelegatedCredentialCertManager.cpp

@@ -41,16 +41,11 @@ std::shared_ptr<SelfCert> DelegatedCredentialCertManager::getCert(
   return dcRes ? dcRes : CertManager::getCert(identity);
 }
 
-void DelegatedCredentialCertManager::addDelegatedCredentialAndSetDefault(
-    std::shared_ptr<SelfDelegatedCredential> cred) {
-  VLOG(8) << "Adding delegated credential";
-  dcMgr_.addCertAndSetDefault(std::move(cred));
-}
-
 void DelegatedCredentialCertManager::addDelegatedCredential(
-    std::shared_ptr<SelfDelegatedCredential> cred) {
+    std::shared_ptr<SelfDelegatedCredential> cred,
+    bool defaultCert) {
   VLOG(8) << "Adding delegated credential";
-  dcMgr_.addCert(std::move(cred));
+  dcMgr_.addCert(std::move(cred), defaultCert);
 }
 
 } // namespace extensions

fizz/extensions/delegatedcred/DelegatedCredentialCertManager.h

@@ -27,10 +27,9 @@ class DelegatedCredentialCertManager : public server::CertManager {
 
   std::shared_ptr<SelfCert> getCert(const std::string& identity) const override;
 
-  void addDelegatedCredentialAndSetDefault(
-      std::shared_ptr<SelfDelegatedCredential> cred);
-
-  void addDelegatedCredential(std::shared_ptr<SelfDelegatedCredential> cred);
+  void addDelegatedCredential(
+      std::shared_ptr<SelfDelegatedCredential> cred,
+      bool defaultCert = false);
 
  protected:
   server::CertManager dcMgr_;

fizz/extensions/delegatedcred/test/DelegatedCredentialCertManagerTest.cpp

@@ -10,7 +10,6 @@
 #include <folly/portability/GTest.h>
 
 #include <fizz/extensions/delegatedcred/DelegatedCredentialCertManager.h>
-
 #include <fizz/extensions/delegatedcred/test/Mocks.h>
 
 #include <fizz/protocol/test/Mocks.h>
@@ -89,19 +88,19 @@ TYPED_TEST(DelegatedCredentialCertManagerTestTyped, TestNoMatchDefault) {
       DelegatedCredentialCertManagerTest::getCert("blah.com", {}, kRsa);
   auto cert2 =
       DelegatedCredentialCertManagerTest::getCert("blah.com", {}, kRsa);
-  DelegatedCredentialCertManagerTest::manager_.addCertAndSetDefault(cert1);
+  DelegatedCredentialCertManagerTest::manager_.addCert(cert1, true);
   // Technically, the default flag doesn't really apply to DC certs.
   // It should always return the undelegated default cert if an exact
   // match can't be found..
-  DelegatedCredentialCertManagerTest::manager_.addCertAndSetDefault(cert2);
+  DelegatedCredentialCertManagerTest::manager_.addCert(cert2, true);
   auto res = DelegatedCredentialCertManagerTest::manager_.getCert(
       std::string("test.com"), kRsa, kRsa, TypeParam::Extensions());
   EXPECT_EQ(res->cert, cert1);
 }
 
 TYPED_TEST(DelegatedCredentialCertManagerTestTyped, TestNoSniDefault) {
   auto cert = DelegatedCredentialCertManagerTest::getCert("blah.com", {}, kRsa);
-  DelegatedCredentialCertManagerTest::manager_.addCertAndSetDefault(cert);
+  DelegatedCredentialCertManagerTest::manager_.addCert(cert, true);
   auto res = DelegatedCredentialCertManagerTest::manager_.getCert(
       folly::none, kRsa, kRsa, TypeParam::Extensions());
   EXPECT_EQ(res->cert, cert);
@@ -110,15 +109,15 @@ TYPED_TEST(DelegatedCredentialCertManagerTestTyped, TestNoSniDefault) {
 TYPED_TEST(DelegatedCredentialCertManagerTestTyped, TestWildcardDefault) {
   auto cert =
       DelegatedCredentialCertManagerTest::getCert("*.blah.com", {}, kRsa);
-  DelegatedCredentialCertManagerTest::manager_.addCertAndSetDefault(cert);
+  DelegatedCredentialCertManagerTest::manager_.addCert(cert, true);
   auto res = DelegatedCredentialCertManagerTest::manager_.getCert(
       folly::none, kRsa, kRsa, TypeParam::Extensions());
   EXPECT_EQ(res->cert, cert);
 }
 
 TYPED_TEST(DelegatedCredentialCertManagerTestTyped, TestUppercaseDefault) {
   auto cert = DelegatedCredentialCertManagerTest::getCert("BLAH.com", {}, kRsa);
-  DelegatedCredentialCertManagerTest::manager_.addCertAndSetDefault(cert);
+  DelegatedCredentialCertManagerTest::manager_.addCert(cert, true);
   auto res = DelegatedCredentialCertManagerTest::manager_.getCert(
       folly::none, kRsa, kRsa, TypeParam::Extensions());
   EXPECT_EQ(res->cert, cert);
@@ -418,7 +417,7 @@ TEST_F(DelegatedCredentialCertManagerTest, TestUndelegatedNeverGetsCredential) {
 
   manager_.addCert(cert1);
   manager_.addCert(cert2);
-  manager_.addCertAndSetDefault(cert3);
+  manager_.addCert(cert3, true);
 
   res = manager_.getCert(host1, kRsa, kRsa, {});
   EXPECT_EQ(res->cert, cert1);
@@ -456,7 +455,7 @@ TEST_F(
 
   manager_.addCert(cert1);
   manager_.addCert(cert2);
-  manager_.addCertAndSetDefault(cert3);
+  manager_.addCert(cert3, true);
 
   auto res = manager_.getCert(host1, kRsa, kRsa, DelegatedMode::Extensions());
   EXPECT_EQ(res->cert, cert1);
@@ -503,8 +502,8 @@ TEST_F(
 TEST_F(DelegatedCredentialCertManagerTest, TestDelegatedMatchWithDefaultSet) {
   auto cert1 = getCert("foo.test.com", {}, kRsa);
   auto cert2 = getCredential("foo.test.com", {}, kRsa);
-  manager_.addCertAndSetDefault(cert1);
-  manager_.addDelegatedCredentialAndSetDefault(cert2);
+  manager_.addCert(cert1, true);
+  manager_.addDelegatedCredential(cert2, true);
 
   auto res = manager_.getCert(
       std::string("foo_blah"), kRsa, kRsa, DelegatedMode::Extensions());
@@ -516,8 +515,8 @@ TEST_F(
     TestDelegatedNotMatchedWithDefaultNotSet) {
   auto cert1 = getCert("foo.test.com", {}, kRsa);
   auto cert2 = getCredential("foo.test.com", {}, kRsa);
-  manager_.addCertAndSetDefault(cert1);
-  manager_.addDelegatedCredential(cert2);
+  manager_.addCert(cert1, true);
+  manager_.addDelegatedCredential(cert2, false);
 
   auto res = manager_.getCert(
       std::string("foo_blah"), kRsa, kRsa, DelegatedMode::Extensions());

fizz/server/CertManager.cpp

@@ -120,14 +120,6 @@ void CertManager::addCertIdentity(
   }
 }
 
-void CertManager::addCert(std::shared_ptr<SelfCert> cert) {
-  addCert(std::move(cert), false);
-}
-
-void CertManager::addCertAndSetDefault(std::shared_ptr<SelfCert> cert) {
-  addCert(std::move(cert), true);
-}
-
 void CertManager::addCert(std::shared_ptr<SelfCert> cert, bool defaultCert) {
   auto primaryIdent = cert->getIdentity();
   addCertIdentity(cert, primaryIdent);

fizz/server/CertManager.h

@@ -36,9 +36,9 @@ class CertManager : public CertManagerBase {
    */
   virtual std::shared_ptr<SelfCert> getCert(const std::string& identity) const;
 
-  void addCertAndSetDefault(std::shared_ptr<SelfCert> cert);
-
-  void addCert(std::shared_ptr<SelfCert> cert);
+  virtual void addCert(
+      std::shared_ptr<SelfCert> cert,
+      bool defaultCert = false);
 
  protected:
   CertMatch findCert(
@@ -50,8 +50,6 @@ class CertManager : public CertManagerBase {
       std::shared_ptr<SelfCert> cert,
       const std::string& ident);
 
-  void addCert(std::shared_ptr<SelfCert> cert, bool defaultCert);
-
   static std::string getKeyFromIdent(const std::string& ident);
 
   std::unordered_map<std::string, SigSchemeMap> certs_;

fizz/server/test/CertManagerTest.cpp

@@ -39,28 +39,28 @@ class CertManagerTest : public Test {
 
 TEST_F(CertManagerTest, TestNoMatchDefault) {
   auto cert = getCert("blah.com", {}, kRsa);
-  manager_.addCertAndSetDefault(cert);
+  manager_.addCert(cert, true);
   auto res = manager_.getCert(std::string("test.com"), kRsa, kRsa, {});
   EXPECT_EQ(res->cert, cert);
 }
 
 TEST_F(CertManagerTest, TestNoSniDefault) {
   auto cert = getCert("blah.com", {}, kRsa);
-  manager_.addCertAndSetDefault(cert);
+  manager_.addCert(cert, true);
   auto res = manager_.getCert(folly::none, kRsa, kRsa, {});
   EXPECT_EQ(res->cert, cert);
 }
 
 TEST_F(CertManagerTest, TestWildcardDefault) {
   auto cert = getCert("*.blah.com", {}, kRsa);
-  manager_.addCertAndSetDefault(cert);
+  manager_.addCert(cert, true);
   auto res = manager_.getCert(folly::none, kRsa, kRsa, {});
   EXPECT_EQ(res->cert, cert);
 }
 
 TEST_F(CertManagerTest, TestUppercaseDefault) {
   auto cert = getCert("BLAH.com", {}, kRsa);
-  manager_.addCertAndSetDefault(cert);
+  manager_.addCert(cert, true);
   auto res = manager_.getCert(folly::none, kRsa, kRsa, {});
   EXPECT_EQ(res->cert, cert);
 }

fizz/server/test/Utils.h

@@ -44,7 +44,7 @@ class FizzTestServer : public folly::AsyncServerSocket::AcceptCallback {
         std::make_unique<openssl::OpenSSLSelfCertImpl<openssl::KeyType::P256>>(
             std::move(certData.key), std::move(certChain));
     auto certManager = std::make_unique<CertManager>();
-    certManager->addCertAndSetDefault(std::move(fizzCert));
+    certManager->addCert(std::move(fizzCert), true);
     ctx_ = std::make_shared<FizzServerContext>();
     ctx_->setCertManager(std::move(certManager));
 
@@ -102,7 +102,7 @@ class FizzTestServer : public folly::AsyncServerSocket::AcceptCallback {
 
   void setCertificate(std::unique_ptr<SelfCert> cert) {
     auto certManager = std::make_unique<CertManager>();
-    certManager->addCertAndSetDefault(std::move(cert));
+    certManager->addCert(std::move(cert), true);
     ctx_->setCertManager(std::move(certManager));
   }
 

fizz/test/BogoShim.cpp

@@ -306,7 +306,7 @@ std::unique_ptr<SelfCert> readSelfCert() {
 
 int serverTest() {
   auto certManager = std::make_shared<server::CertManager>();
-  certManager->addCertAndSetDefault(readSelfCert());
+  certManager->addCert(readSelfCert(), true);
 
   auto serverContext = std::make_shared<FizzServerContext>();
   serverContext->setCertManager(certManager);

fizz/test/HandshakeTest.h

@@ -70,9 +70,10 @@ class HandshakeTest : public Test {
         std::make_shared<ZlibCertificateCompressor>(9)};
     std::vector<ssl::X509UniquePtr> rsaCerts;
     rsaCerts.emplace_back(getCert(kRSACertificate));
-    certManager->addCertAndSetDefault(
+    certManager->addCert(
         std::make_shared<openssl::OpenSSLSelfCertImpl<openssl::KeyType::RSA>>(
-            getPrivateKey(kRSAKey), std::move(rsaCerts), compressors));
+            getPrivateKey(kRSAKey), std::move(rsaCerts), compressors),
+        true);
     std::vector<ssl::X509UniquePtr> p256Certs;
     std::vector<ssl::X509UniquePtr> p384Certs;
     std::vector<ssl::X509UniquePtr> p521Certs;

fizz/tool/FizzServerBenchmarkCommand.cpp

@@ -257,10 +257,10 @@ int fizzServerBenchmarkCommand(const std::vector<std::string>& args) {
       auto batchCert =
           std::make_shared<BatchSignatureAsyncSelfCert<Sha256>>(batcher);
       serverContext->setSupportedSigSchemes(batchCert->getSigSchemes());
-      certManager->addCertAndSetDefault(batchCert);
+      certManager->addCert(batchCert, true);
     } else {
       serverContext->setSupportedSigSchemes(sharedCert->getSigSchemes());
-      certManager->addCertAndSetDefault(sharedCert);
+      certManager->addCert(sharedCert, true);
     }
   }
   serverContext->setCertManager(std::move(certManager));

fizz/tool/FizzServerCommand.cpp

@@ -1094,11 +1094,7 @@ int fizzServerCommand(const std::vector<std::string>& args) {
 
       auto cert = openssl::CertUtils::makeSelfCert(
           std::move(certs), std::move(pkey.second), compressors);
-      if (first) {
-        certManager->addCertAndSetDefault(std::move(cert));
-      } else {
-        certManager->addCert(std::move(cert));
-      }
+      certManager->addCert(std::move(cert), first);
       if (first) {
         if (fallback) {
           // Fallback mode requires additional callback work for SNI to be
@@ -1118,7 +1114,7 @@ int fizzServerCommand(const std::vector<std::string>& args) {
     auto cert =
         std::make_unique<openssl::OpenSSLSelfCertImpl<openssl::KeyType::P256>>(
             std::move(certData.key), std::move(certChain), compressors);
-    certManager->addCertAndSetDefault(std::move(cert));
+    certManager->addCert(std::move(cert), true);
   }
 
   if (credentialMatchNeeded) {