auth: Move key validity check out of `fromISCMap()`

It doesn't make a lot of sense to check the key validity at every
call of `fromISCMap()`, and it hurts performance a lot when keys
are not cached.

* Add separate `DNSSECKeeper::checkKeys()` and
`DNSCryptoKeyEngine::checkKeys()` methods
* Key validity is checked on import-zone-key, check-zone and
test-algorithm(s)

diff --git a/pdns/dbdnsseckeeper.cc b/pdns/dbdnsseckeeper.cc
index 5f430d40021445be8f01efec6b9bc27f9d8549fd..e62180c89f2c268e7654435c865e0cd80edbf150 100644 (file)
--- a/pdns/dbdnsseckeeper.cc
+++ b/pdns/dbdnsseckeeper.cc
@@ -468,6 +468,22 @@ DNSSECKeeper::keyset_t DNSSECKeeper::getKeys(const DNSName& zone, bool useCache)
   return retkeyset;
 }
 
+bool DNSSECKeeper::checkKeys(const DNSName& zone)
+{
+  vector<DNSBackend::KeyData> dbkeyset;
+  d_keymetadb->getDomainKeys(zone, 0, dbkeyset);
+
+  for(const DNSBackend::KeyData &keydata : dbkeyset) {
+    DNSKEYRecordContent dkrc;
+    shared_ptr<DNSCryptoKeyEngine> dke(DNSCryptoKeyEngine::makeFromISCString(dkrc, keydata.content));
+    if (!dke->checkKeys()) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
 bool DNSSECKeeper::getPreRRSIGs(UeberBackend& db, const DNSName& signer, const DNSName& qname,
         const DNSName& wildcardname, const QType& qtype,
         DNSResourceRecord::Place signPlace, vector<DNSResourceRecord>& rrsigs, uint32_t signTTL)

diff --git a/pdns/dnssecinfra.cc b/pdns/dnssecinfra.cc
index 859dc824db00912d008eae828fc67d14856b2ee7..30a350cc05831971685f5356fde4d1e06df79351 100644 (file)
--- a/pdns/dnssecinfra.cc
+++ b/pdns/dnssecinfra.cc
@@ -38,7 +38,12 @@ DNSCryptoKeyEngine* DNSCryptoKeyEngine::makeFromISCFile(DNSKEYRecordContent& drc
     isc += sline;
   }
   fclose(fp);
-  return makeFromISCString(drc, isc);
+  DNSCryptoKeyEngine* dke = makeFromISCString(drc, isc);
+  if(!dke->checkKeys()) {
+    delete dke;
+    throw runtime_error("Invalid DNS Private Key in file '"+string(fname));
+  }
+  return dke;
 }
 
 DNSCryptoKeyEngine* DNSCryptoKeyEngine::makeFromISCString(DNSKEYRecordContent& drc, const std::string& content)
@@ -252,6 +257,9 @@ pair<unsigned int, unsigned int> DNSCryptoKeyEngine::testMakers(unsigned int alg
       stormap[toLower(key)]=raw;
     }
     dckeSign->fromISCMap(dkrc, stormap);
+    if(!dckeSign->checkKeys()) {
+      throw runtime_error("Verification of keys with creator "+dckeCreate->getName()+" with signer "+dckeSign->getName()+" and verifier "+dckeVerify->getName()+" failed");
+    }
   }
 
   string message("Hi! How is life?");

diff --git a/pdns/dnssecinfra.hh b/pdns/dnssecinfra.hh
index 9d5cdd82b9d146ccf68311d528fe911a86f2a2d9..b9620f63e1f28cac450930b42825032f6d1d7f32 100644 (file)
--- a/pdns/dnssecinfra.hh
+++ b/pdns/dnssecinfra.hh
@@ -38,7 +38,10 @@ class DNSCryptoKeyEngine
       throw std::runtime_error("Can't import from PEM string");
     }
     virtual void fromPublicKeyString(const std::string& content) = 0;
-    
+    virtual bool checkKeys() const
+    {
+      return true;
+    }
     static DNSCryptoKeyEngine* makeFromISCFile(DNSKEYRecordContent& drc, const char* fname);
     static DNSCryptoKeyEngine* makeFromISCString(DNSKEYRecordContent& drc, const std::string& content);
     static DNSCryptoKeyEngine* makeFromPEMString(DNSKEYRecordContent& drc, const std::string& raw);

diff --git a/pdns/dnsseckeeper.hh b/pdns/dnsseckeeper.hh
index 8eab6177de99d8d098a7474a774298c459c851a4..e71beee6d80fdd8b414655bacd8a8a90b1694427 100644 (file)
--- a/pdns/dnsseckeeper.hh
+++ b/pdns/dnsseckeeper.hh
@@ -167,6 +167,7 @@ public:
   bool removeKey(const DNSName& zname, unsigned int id);
   bool activateKey(const DNSName& zname, unsigned int id);
   bool deactivateKey(const DNSName& zname, unsigned int id);
+  bool checkKeys(const DNSName& zname);
 
   bool getNSEC3PARAM(const DNSName& zname, NSEC3PARAMRecordContent* n3p=0, bool* narrow=0);
   bool setNSEC3PARAM(const DNSName& zname, const NSEC3PARAMRecordContent& n3p, const bool& narrow=false);

diff --git a/pdns/opensslsigners.cc b/pdns/opensslsigners.cc
index 49679f8c5e92e3d0e9f574d5f62a36b97d135fea..02c4028ddef27187f9f2656711b04e374694f477 100644 (file)
--- a/pdns/opensslsigners.cc
+++ b/pdns/opensslsigners.cc
@@ -99,6 +99,7 @@ public:
   std::string getPublicKeyString() const;
   void fromISCMap(DNSKEYRecordContent& drc, std::map<std::string, std::string>& stormap);
   void fromPublicKeyString(const std::string& content);
+  bool checkKeys() const override;
 
   static DNSCryptoKeyEngine* maker(unsigned int algorithm)
   {
@@ -353,18 +354,17 @@ void OpenSSLRSADNSCryptoKeyEngine::fromISCMap(DNSKEYRecordContent& drc, std::map
     throw runtime_error(getName()+" tried to feed an algorithm "+std::to_string(drc.d_algorithm)+" to a "+std::to_string(d_algorithm)+" key");
   }
 
-  int ret = RSA_check_key(key);
-  if (ret != 1) {
-    RSA_free(key);
-    throw runtime_error(getName()+" invalid public key");
-  }
-
   if (d_key)
     RSA_free(d_key);
 
   d_key = key;
 }
 
+bool OpenSSLRSADNSCryptoKeyEngine::checkKeys() const
+{
+  return (RSA_check_key(d_key) == 1);
+}
+
 void OpenSSLRSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& input)
 {
   string exponent, modulus;
@@ -409,8 +409,7 @@ void OpenSSLRSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& input)
     RSA_free(key);
     throw runtime_error(getName()+" error loading n value of public key");
   }
-  /* we cannot use RSA_check_key(), because it requires the private key information
-     to be present. */
+
   if (d_key)
     RSA_free(d_key);
 
@@ -472,6 +471,7 @@ public:
   std::string getPublicKeyString() const;
   void fromISCMap(DNSKEYRecordContent& drc, std::map<std::string, std::string>& stormap);
   void fromPublicKeyString(const std::string& content);
+  bool checkKeys() const override;
 
   static DNSCryptoKeyEngine* maker(unsigned int algorithm)
   {
@@ -678,14 +678,12 @@ void OpenSSLECDSADNSCryptoKeyEngine::fromISCMap(DNSKEYRecordContent& drc, std::m
   }
 
   EC_POINT_free(pub_key);
-
-  ret = EC_KEY_check_key(d_eckey);
-  if (ret != 1) {
-    throw runtime_error(getName()+" invalid public key");
-  }
-
 }
 
+bool OpenSSLECDSADNSCryptoKeyEngine::checkKeys() const
+{
+  return (EC_KEY_check_key(d_eckey) == 1);
+}
 
 void OpenSSLECDSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& input)
 {
@@ -717,11 +715,6 @@ void OpenSSLECDSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& inpu
   }
 
   EC_POINT_free(pub_key);
-
-  ret = EC_KEY_check_key(d_eckey);
-  if (ret != 1) {
-    throw runtime_error(getName()+" invalid public key");
-  }
 }
 
 

diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc
index fbe6e955b1934b2a20bc323156b0ef6ee801cd0c..96c32eef15736818c49d1a96ed47b1c03c20f7cf 100644 (file)
--- a/pdns/pdnsutil.cc
+++ b/pdns/pdnsutil.cc
@@ -401,6 +401,7 @@ int checkZone(DNSSECKeeper &dk, UeberBackend &B, const DNSName& zone, const vect
 
   bool isSecure=dk.isSecuredZone(zone);
   bool presigned=dk.isPresigned(zone);
+  bool validKeys=dk.checkKeys(zone);
 
   DNSResourceRecord rr;
   uint64_t numrecords=0, numerrors=0, numwarnings=0;
@@ -410,6 +411,11 @@ int checkZone(DNSSECKeeper &dk, UeberBackend &B, const DNSName& zone, const vect
     cout<<"[Error] zone '" << zone.toStringNoDot() << "' has NSEC3 semantics but is too long to have the hash prepended. Zone name is " << zone.wirelength() << " bytes long, whereas the maximum is 222 bytes." << endl;
   }
 
+  if (!validKeys) {
+    numerrors++;
+    cout<<"[Error] zone '" << zone.toStringNoDot() << "' has at least one invalid DNS Private Key." << endl;
+  }
+
   // Check for delegation in parent zone
   DNSName parent(zone);
   while(parent.chopOff()) {