From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Sun, 12 Feb 2017 22:45:39 +0000 (+0100)
Subject: Refactoring of the TSIG handling
X-Git-Tag: rec-4.1.0-alpha1~254^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F5003%2Fhead;p=thirdparty%2Fpdns.git

Refactoring of the TSIG handling

* Merge all the TSIG checks into `validateTSIG()` to remove
code duplication and make it easier to audit
* Add unit tests
---

diff --git a/pdns/Makefile.am b/pdns/Makefile.am
index d2c7ca64db..193ee24d0c 100644
--- a/pdns/Makefile.am
+++ b/pdns/Makefile.am
@@ -1157,6 +1157,7 @@ testrunner_SOURCES = \
 	test-recpacketcache_cc.cc \
 	test-sha_hh.cc \
 	test-statbag_cc.cc \
+	test-tsig.cc \
 	test-zoneparser_tng_cc.cc \
 	testrunner.cc \
 	ueberbackend.cc \
diff --git a/pdns/dnspacket.cc b/pdns/dnspacket.cc
index 64619d7210..cc77200b6d 100644
--- a/pdns/dnspacket.cc
+++ b/pdns/dnspacket.cc
@@ -354,7 +354,7 @@ void DNSPacket::wrapup()
   }
   
   if(d_trc.d_algoName.countLabels())
-    addTSIG(pw, &d_trc, d_tsigkeyname, d_tsigsecret, d_tsigprevious, d_tsigtimersonly);
+    addTSIG(pw, d_trc, d_tsigkeyname, d_tsigsecret, d_tsigprevious, d_tsigtimersonly);
   
   d_rawpacket.assign((char*)&packet[0], packet.size()); // XXX we could do this natively on a vector..
 
@@ -460,11 +460,11 @@ void DNSPacket::setTSIGDetails(const TSIGRecordContent& tr, const DNSName& keyna
   d_tsigtimersonly=timersonly;
 }
 
-bool DNSPacket::getTSIGDetails(TSIGRecordContent* trc, DNSName* keyname, string* message) const
+bool DNSPacket::getTSIGDetails(TSIGRecordContent* trc, DNSName* keyname, uint16_t* tsigPosOut) const
 {
   MOADNSParser mdp(d_isQuery, d_rawpacket);
-
-  if(!mdp.getTSIGPos()) 
+  uint16_t tsigPos = mdp.getTSIGPos();
+  if(!tsigPos)
     return false;
   
   bool gotit=false;
@@ -483,8 +483,10 @@ bool DNSPacket::getTSIGDetails(TSIGRecordContent* trc, DNSName* keyname, string*
   }
   if(!gotit)
     return false;
-  if(message)
-    *message = makeTSIGMessageFromTSIGPacket(d_rawpacket, mdp.getTSIGPos(), *keyname, *trc, "", false); // if you change rawpacket to getString it breaks!
+
+  if (tsigPosOut) {
+    *tsigPosOut = tsigPos;
+  }
   
   return true;
 }
@@ -640,50 +642,38 @@ void DNSPacket::commitD()
   d_rawpacket.replace(0,12,(char *)&d,12); // copy in d
 }
 
-bool checkForCorrectTSIG(const DNSPacket* q, UeberBackend* B, DNSName* keyname, string* secret, TSIGRecordContent* trc)
+bool DNSPacket::checkForCorrectTSIG(UeberBackend* B, DNSName* keyname, string* secret, TSIGRecordContent* trc) const
 {
-  string message;
-
-  if (!q->getTSIGDetails(trc, keyname, &message)) {
-    return false;
-  }
+  uint16_t tsigPos;
 
-  uint64_t delta = std::abs((int64_t)trc->d_time - (int64_t)time(0));
-  if(delta > trc->d_fudge) {
-    L<<Logger::Error<<"Packet for '"<<q->qdomain<<"' denied: TSIG (key '"<<*keyname<<"') time delta "<< delta <<" > 'fudge' "<<trc->d_fudge<<endl;
+  if (!this->getTSIGDetails(trc, keyname, &tsigPos)) {
     return false;
   }
 
-  DNSName algoName = trc->d_algoName; // FIXME400
-  if (algoName == DNSName("hmac-md5.sig-alg.reg.int"))
-    algoName = DNSName("hmac-md5");
+  TSIGTriplet tt;
+  tt.name = *keyname;
+  tt.algo = trc->d_algoName;
+  if (tt.algo == DNSName("hmac-md5.sig-alg.reg.int"))
+    tt.algo = DNSName("hmac-md5");
 
-  if (algoName == DNSName("gss-tsig")) {
-    if (!gss_verify_signature(*keyname, message, trc->d_mac)) {
-      L<<Logger::Error<<"Packet for domain '"<<q->qdomain<<"' denied: TSIG signature mismatch using '"<<*keyname<<"' and algorithm '"<<trc->d_algoName<<"'"<<endl;
+  string secret64;
+  if (tt.algo != DNSName("gss-tsig")) {
+    if(!B->getTSIGKey(*keyname, &tt.algo, &secret64)) {
+      L<<Logger::Error<<"Packet for domain '"<<this->qdomain<<"' denied: can't find TSIG key with name '"<<*keyname<<"' and algorithm '"<<tt.algo<<"'"<<endl;
       return false;
     }
-    return true;
+    B64Decode(secret64, *secret);
+    tt.secret = *secret;
   }
 
-  string secret64;
-  if(!B->getTSIGKey(*keyname, &algoName, &secret64)) {
-    L<<Logger::Error<<"Packet for domain '"<<q->qdomain<<"' denied: can't find TSIG key with name '"<<*keyname<<"' and algorithm '"<<algoName<<"'"<<endl;
-    return false;
-  }
-  if (trc->d_algoName == DNSName("hmac-md5"))
-    trc->d_algoName += DNSName("sig-alg.reg.int");
+  bool result;
 
-  TSIGHashEnum algo;
-  if(!getTSIGHashEnum(trc->d_algoName, algo)) {
-     L<<Logger::Error<<"Unsupported TSIG HMAC algorithm " << trc->d_algoName.toString() << endl;
-     return false;
+  try {
+    result = validateTSIG(d_rawpacket, tsigPos, tt, *trc, "", trc->d_mac, false);
   }
-
-  B64Decode(secret64, *secret);
-  bool result=calculateHMAC(*secret, message, algo) == trc->d_mac;
-  if(!result) {
-    L<<Logger::Error<<"Packet for domain '"<<q->qdomain<<"' denied: TSIG signature mismatch using '"<<*keyname<<"' and algorithm '"<<trc->d_algoName<<"'"<<endl;
+  catch(const std::runtime_error& err) {
+    L<<Logger::Error<<"Packet for '"<<this->qdomain<<"' denied: "<<err.what()<<endl;
+    return false;
   }
 
   return result;
diff --git a/pdns/dnspacket.hh b/pdns/dnspacket.hh
index be58581dd7..4b740344a9 100644
--- a/pdns/dnspacket.hh
+++ b/pdns/dnspacket.hh
@@ -159,11 +159,13 @@ public:
   bool d_dnssecOk;
   bool d_havetsig;
 
-  bool getTSIGDetails(TSIGRecordContent* tr, DNSName* keyname, string* message) const;
+  bool getTSIGDetails(TSIGRecordContent* tr, DNSName* keyname, uint16_t* tsigPos=nullptr) const;
   void setTSIGDetails(const TSIGRecordContent& tr, const DNSName& keyname, const string& secret, const string& previous, bool timersonly=false);
   bool getTKEYRecord(TKEYRecordContent* tr, DNSName* keyname) const;
 
   vector<DNSZoneRecord>& getRRS() { return d_rrs; }
+  bool checkForCorrectTSIG(UeberBackend* B, DNSName* keyname, string* secret, TSIGRecordContent* trc) const;
+
   static bool s_doEDNSSubnetProcessing;
   static uint16_t s_udpTruncationThreshold; //2
 private:
@@ -194,7 +196,4 @@ private:
   bool d_isQuery;
 };
 
-
-bool checkForCorrectTSIG(const DNSPacket* q, UeberBackend* B, DNSName* keyname, string* secret, TSIGRecordContent* trc);
-
 #endif
diff --git a/pdns/dnssecinfra.cc b/pdns/dnssecinfra.cc
index 79278fabd8..1b59bc0e8c 100644
--- a/pdns/dnssecinfra.cc
+++ b/pdns/dnssecinfra.cc
@@ -570,7 +570,7 @@ void decodeDERIntegerSequence(const std::string& input, vector<string>& output)
   }  
 }
 
-string calculateHMAC(const std::string& key, const std::string& text, TSIGHashEnum hasher) {
+static string calculateHMAC(const std::string& key, const std::string& text, TSIGHashEnum hasher) {
 
   const EVP_MD* md_type;
   unsigned int outlen;
@@ -606,7 +606,7 @@ string calculateHMAC(const std::string& key, const std::string& text, TSIGHashEn
   return string((char*) hash, outlen);
 }
 
-bool constantTimeStringEquals(const std::string& a, const std::string& b)
+static bool constantTimeStringEquals(const std::string& a, const std::string& b)
 {
   if (a.size() != b.size()) {
     return false;
@@ -627,35 +627,23 @@ bool constantTimeStringEquals(const std::string& a, const std::string& b)
 #endif
 }
 
-string makeTSIGMessageFromTSIGPacket(const string& opacket, unsigned int tsigOffset, const DNSName& keyname, const TSIGRecordContent& trc, const string& previous, bool timersonly, unsigned int dnsHeaderOffset)
+static string makeTSIGPayload(const string& previous, const char* packetBegin, size_t packetSize, const DNSName& tsigKeyName, const TSIGRecordContent& trc, bool timersonly)
 {
   string message;
-  string packet(opacket);
-
-  packet.resize(tsigOffset); // remove the TSIG record at the end as per RFC2845 3.4.1
-  packet[(dnsHeaderOffset + sizeof(struct dnsheader))-1]--; // Decrease ARCOUNT because we removed the TSIG RR in the previous line.
-  
-
-  // Replace the message ID with the original message ID from the TSIG record.
-  // This is needed for forwarded DNS Update as they get a new ID when forwarding (section 6.1 of RFC2136). The TSIG record stores the original ID and the
-  // signature was created with the original ID, so we replace it here to get the originally signed message.
-  // If the message is not forwarded, we simply override it with the same id.
-  uint16_t origID = htons(trc.d_origID);
-  packet.replace(0, 2, (char*)&origID, 2);
 
   if(!previous.empty()) {
     uint16_t len = htons(previous.length());
-    message.append((char*)&len, 2);
+    message.append(reinterpret_cast<const char*>(&len), sizeof(len));
     message.append(previous);
   }
-  
-  message.append(packet);
+
+  message.append(packetBegin, packetSize);
 
   vector<uint8_t> signVect;
   DNSPacketWriter dw(signVect, DNSName(), 0);
   auto pos=signVect.size();
   if(!timersonly) {
-    dw.xfrName(keyname, false);
+    dw.xfrName(tsigKeyName, false);
     dw.xfr16BitInt(QClass::ANY); // class
     dw.xfr32BitInt(0);    // TTL
     dw.xfrName(trc.d_algoName.makeLowerCase(), false);
@@ -673,54 +661,83 @@ string makeTSIGMessageFromTSIGPacket(const string& opacket, unsigned int tsigOff
   return message;
 }
 
-void addTSIG(DNSPacketWriter& pw, TSIGRecordContent* trc, const DNSName& tsigkeyname, const string& tsigsecret, const string& tsigprevious, bool timersonly)
+static string makeTSIGMessageFromTSIGPacket(const string& opacket, unsigned int tsigOffset, const DNSName& keyname, const TSIGRecordContent& trc, const string& previous, bool timersonly, unsigned int dnsHeaderOffset=0)
 {
-  TSIGHashEnum algo;
-  if (!getTSIGHashEnum(trc->d_algoName, algo)) {
-    throw PDNSException(string("Unsupported TSIG HMAC algorithm ") + trc->d_algoName.toString());
-  }
+  string message;
+  string packet(opacket);
 
-  string toSign;
-  if(!tsigprevious.empty()) {
-    uint16_t len = htons(tsigprevious.length());
-    toSign.append((char*)&len, 2);
-    
-    toSign.append(tsigprevious);
-  }
-  toSign.append(pw.getContent().begin(), pw.getContent().end());
-  
-  // now add something that looks a lot like a TSIG record, but isn't
-  vector<uint8_t> signVect;
-  DNSPacketWriter dw(signVect, DNSName(), 0);
-  auto pos=dw.size();
-  if(!timersonly) {
-    dw.xfrName(tsigkeyname, false);
-    dw.xfr16BitInt(QClass::ANY); // class
-    dw.xfr32BitInt(0);    // TTL
-    dw.xfrName(trc->d_algoName, false);
-  }  
-  uint32_t now = trc->d_time; 
-  dw.xfr48BitInt(now);
-  dw.xfr16BitInt(trc->d_fudge); // fudge
+  packet.resize(tsigOffset); // remove the TSIG record at the end as per RFC2845 3.4.1
+  packet[(dnsHeaderOffset + sizeof(struct dnsheader))-1]--; // Decrease ARCOUNT because we removed the TSIG RR in the previous line.
   
-  if(!timersonly) {
-    dw.xfr16BitInt(trc->d_eRcode); // extended rcode
-    dw.xfr16BitInt(trc->d_otherData.length()); // length of 'other' data
-    //    dw.xfrBlob(trc->d_otherData);
+
+  // Replace the message ID with the original message ID from the TSIG record.
+  // This is needed for forwarded DNS Update as they get a new ID when forwarding (section 6.1 of RFC2136). The TSIG record stores the original ID and the
+  // signature was created with the original ID, so we replace it here to get the originally signed message.
+  // If the message is not forwarded, we simply override it with the same id.
+  uint16_t origID = htons(trc.d_origID);
+  packet.replace(0, 2, (char*)&origID, 2);
+
+  return makeTSIGPayload(previous, packet.data(), packet.size(), keyname, trc, timersonly);
+}
+
+void addTSIG(DNSPacketWriter& pw, TSIGRecordContent& trc, const DNSName& tsigkeyname, const string& tsigsecret, const string& tsigprevious, bool timersonly)
+{
+  TSIGHashEnum algo;
+  if (!getTSIGHashEnum(trc.d_algoName, algo)) {
+    throw PDNSException(string("Unsupported TSIG HMAC algorithm ") + trc.d_algoName.toString());
   }
-  
-  toSign.append(signVect.begin() + pos, signVect.end());
+
+  string toSign = makeTSIGPayload(tsigprevious, reinterpret_cast<const char*>(pw.getContent().data()), pw.getContent().size(), tsigkeyname, trc, timersonly);
 
   if (algo == TSIG_GSS) {
-    if (!gss_add_signature(tsigkeyname, toSign, trc->d_mac)) {
+    if (!gss_add_signature(tsigkeyname, toSign, trc.d_mac)) {
       throw PDNSException(string("Could not add TSIG signature with algorithm 'gss-tsig' and key name '")+tsigkeyname.toString()+string("'"));
     }
   } else {
-    trc->d_mac = calculateHMAC(tsigsecret, toSign, algo);
-    //  d_trc->d_mac[0]++; // sabotage
+    trc.d_mac = calculateHMAC(tsigsecret, toSign, algo);
+    //  trc.d_mac[0]++; // sabotage
   }
   pw.startRecord(tsigkeyname, QType::TSIG, 0, QClass::ANY, DNSResourceRecord::ADDITIONAL, false);
-  trc->toPacket(pw);
+  trc.toPacket(pw);
   pw.commit();
 }
 
+bool validateTSIG(const std::string& packet, size_t sigPos, const TSIGTriplet& tt, const TSIGRecordContent& trc, const std::string& previousMAC, const std::string& theirMAC, bool timersOnly, unsigned int dnsHeaderOffset)
+{
+  uint64_t delta = std::abs((int64_t)trc.d_time - (int64_t)time(nullptr));
+  if(delta > trc.d_fudge) {
+    throw std::runtime_error("Invalid TSIG time delta " + std::to_string(delta) + " >  fudge " + std::to_string(trc.d_fudge));
+  }
+
+  TSIGHashEnum algo;
+  if (!getTSIGHashEnum(trc.d_algoName, algo)) {
+    throw std::runtime_error("Unsupported TSIG HMAC algorithm " + trc.d_algoName.toString());
+  }
+
+  TSIGHashEnum expectedAlgo;
+  if (!getTSIGHashEnum(tt.algo, expectedAlgo)) {
+    throw std::runtime_error("Unsupported TSIG HMAC algorithm expected " + tt.algo.toString());
+  }
+
+  if (algo != expectedAlgo) {
+    throw std::runtime_error("Signature with TSIG key '"+tt.name.toString()+"' does not match the expected algorithm (" + tt.algo.toString() + " / " + trc.d_algoName.toString() + ")");
+  }
+
+  string tsigMsg;
+  tsigMsg = makeTSIGMessageFromTSIGPacket(packet, sigPos, tt.name, trc, previousMAC, timersOnly, dnsHeaderOffset);
+
+  if (algo == TSIG_GSS) {
+    GssContext gssctx(tt.name);
+    if (!gss_verify_signature(tt.name, tsigMsg, theirMAC)) {
+      throw std::runtime_error("Signature with TSIG key '"+tt.name.toString()+"' failed to validate");
+    }
+  } else {
+    string ourMac = calculateHMAC(tt.secret, tsigMsg, algo);
+
+    if(!constantTimeStringEquals(ourMac, theirMAC)) {
+      throw std::runtime_error("Signature with TSIG key '"+tt.name.toString()+"' failed to validate");
+    }
+  }
+
+  return true;
+}
diff --git a/pdns/dnssecinfra.hh b/pdns/dnssecinfra.hh
index 41a6a0fd40..8688a743e8 100644
--- a/pdns/dnssecinfra.hh
+++ b/pdns/dnssecinfra.hh
@@ -156,10 +156,8 @@ void decodeDERIntegerSequence(const std::string& input, vector<string>& output);
 class DNSPacket;
 void addRRSigs(DNSSECKeeper& dk, UeberBackend& db, const std::set<DNSName>& authMap, vector<DNSZoneRecord>& rrs);
 
-string calculateHMAC(const std::string& key, const std::string& text, TSIGHashEnum hash);
-bool constantTimeStringEquals(const std::string& a, const std::string& b);
+void addTSIG(DNSPacketWriter& pw, TSIGRecordContent& trc, const DNSName& tsigkeyname, const string& tsigsecret, const string& tsigprevious, bool timersonly);
+bool validateTSIG(const std::string& packet, size_t sigPos, const TSIGTriplet& tt, const TSIGRecordContent& trc, const std::string& previousMAC, const std::string& theirMAC, bool timersOnly, unsigned int dnsHeaderOffset=0);
 
-string makeTSIGMessageFromTSIGPacket(const string& opacket, unsigned int tsigoffset, const DNSName& keyname, const TSIGRecordContent& trc, const string& previous, bool timersonly, unsigned int dnsHeaderOffset=0);
-void addTSIG(DNSPacketWriter& pw, TSIGRecordContent* trc, const DNSName& tsigkeyname, const string& tsigsecret, const string& tsigprevious, bool timersonly);
 uint64_t signatureCacheSize(const std::string& str);
 #endif
diff --git a/pdns/ixfr.cc b/pdns/ixfr.cc
index a325eaec5f..eff4587c21 100644
--- a/pdns/ixfr.cc
+++ b/pdns/ixfr.cc
@@ -54,7 +54,7 @@ vector<pair<vector<DNSRecord>, vector<DNSRecord> > > getIXFRDeltas(const ComboAd
     trc.d_fudge = 300;
     trc.d_origID=ntohs(pw.getHeader()->id);
     trc.d_eRcode=0;
-    addTSIG(pw, &trc, tt.name, tt.secret, "", false);
+    addTSIG(pw, trc, tt.name, tt.secret, "", false);
   }
   uint16_t len=htons(packet.size());
   string msg((const char*)&len, 2);
diff --git a/pdns/ixplore.cc b/pdns/ixplore.cc
index 689da9970a..41001dfcb0 100644
--- a/pdns/ixplore.cc
+++ b/pdns/ixplore.cc
@@ -88,7 +88,7 @@ uint32_t getSerialFromMaster(const ComboAddress& master, const DNSName& zone, sh
     trc.d_fudge = 300;
     trc.d_origID=ntohs(pw.getHeader()->id);
     trc.d_eRcode=0;
-    addTSIG(pw, &trc, tt.name, tt.secret, "", false);
+    addTSIG(pw, trc, tt.name, tt.secret, "", false);
   }
   
   Socket s(master.sin4.sin_family, SOCK_DGRAM);
diff --git a/pdns/mastercommunicator.cc b/pdns/mastercommunicator.cc
index c7fc5660f4..f4c343c123 100644
--- a/pdns/mastercommunicator.cc
+++ b/pdns/mastercommunicator.cc
@@ -249,7 +249,7 @@ void CommunicatorClass::sendNotification(int sock, const DNSName& domain, const
     trc.d_origID=ntohs(id);
     trc.d_eRcode=0;
     B64Decode(tsigsecret64, tsigsecret);
-    addTSIG(pw, &trc, tsigkeyname, tsigsecret, "", false);
+    addTSIG(pw, trc, tsigkeyname, tsigsecret, "", false);
   }
 
   if(sendto(sock, &packet[0], packet.size(), 0, (struct sockaddr*)(&remote), remote.getSocklen()) < 0) {
diff --git a/pdns/packethandler.cc b/pdns/packethandler.cc
index 055aed6869..e6190e2477 100644
--- a/pdns/packethandler.cc
+++ b/pdns/packethandler.cc
@@ -1151,7 +1151,7 @@ DNSPacket *PacketHandler::questionOrRecurse(DNSPacket *p, bool *shouldRecurse)
     DNSName keyname;
     string secret;
     TSIGRecordContent trc;
-    if(!checkForCorrectTSIG(p, &B, &keyname, &secret, &trc)) {
+    if(!p->checkForCorrectTSIG(&B, &keyname, &secret, &trc)) {
       r=p->replyPacket();  // generate an empty reply packet
       if(d_logDNSDetails)
         L<<Logger::Error<<"Received a TSIG signed message with a non-validating key"<<endl;
diff --git a/pdns/resolver.cc b/pdns/resolver.cc
index da25637b66..958c5e0c7e 100644
--- a/pdns/resolver.cc
+++ b/pdns/resolver.cc
@@ -144,7 +144,7 @@ uint16_t Resolver::sendResolve(const ComboAddress& remote, const ComboAddress& l
     trc.d_fudge = 300;
     trc.d_origID=ntohs(randomid);
     trc.d_eRcode=0;
-    addTSIG(pw, &trc, tsigkeyname, tsigsecret, "", false);
+    addTSIG(pw, trc, tsigkeyname, tsigsecret, "", false);
   }
 
   int sock;
@@ -407,7 +407,7 @@ AXFRRetriever::AXFRRetriever(const ComboAddress& remote,
       d_trc.d_fudge = 300;
       d_trc.d_origID=ntohs(pw.getHeader()->id);
       d_trc.d_eRcode=0;
-      addTSIG(pw, &d_trc, tt.name, tt.secret, "", false);
+      addTSIG(pw, d_trc, tt.name, tt.secret, "", false);
     }
   
     uint16_t replen=htons(packet.size());
diff --git a/pdns/rfc2136handler.cc b/pdns/rfc2136handler.cc
index 587a108a5e..2bdeec075e 100644
--- a/pdns/rfc2136handler.cc
+++ b/pdns/rfc2136handler.cc
@@ -742,7 +742,7 @@ int PacketHandler::processUpdate(DNSPacket *p) {
       TSIGRecordContent trc;
       DNSName inputkey;
       string message;
-      if (! p->getTSIGDetails(&trc,  &inputkey, 0)) {
+      if (! p->getTSIGDetails(&trc,  &inputkey)) {
         L<<Logger::Error<<msgPrefix<<"TSIG key required, but packet does not contain key. Sending REFUSED"<<endl;
         return RCode::Refused;
       }
diff --git a/pdns/saxfr.cc b/pdns/saxfr.cc
index 040522c627..38ba5590fe 100644
--- a/pdns/saxfr.cc
+++ b/pdns/saxfr.cc
@@ -16,29 +16,6 @@
 
 StatBag S;
 
-bool validateTSIG(const string& message, const TSIGHashEnum& algo, const DNSName& key, const string& secret, const TSIGRecordContent *trc) {
-  int64_t now = time(0);
-  if(abs(static_cast<int64_t>(trc->d_time) - now) > trc->d_fudge) {
-    cerr<<"TSIG (key '"<<key<<"') time delta "<< abs(static_cast<int64_t>(trc->d_time) - now)<<" > 'fudge' "<<trc->d_fudge<<endl;
-    return false;
-  }
-  if (algo == TSIG_GSS) {
-    // authorization is done later
-    GssContext gssctx(key);
-    if (!gssctx.valid()) {
-      cerr<<"no context"<<endl;
-      return false;
-    }
-    if (!gssctx.verify(message, trc->d_mac)) {
-      cerr<<"invalid mac"<<endl;
-      return false;
-    }
-    return true;
-  }
-  return constantTimeStringEquals(calculateHMAC(secret, message, algo), trc->d_mac);
-}
-
-
 int main(int argc, char** argv)
 try
 {
@@ -194,7 +171,7 @@ try
     trc.d_fudge = 300;
     trc.d_origID=ntohs(pw.getHeader()->id);
     trc.d_eRcode=0;
-    addTSIG(pw, &trc, tsig_key, tsig_secret, "", false);
+    addTSIG(pw, trc, tsig_key, tsig_secret, "", false);
   }
 
   len = htons(packet.size());
diff --git a/pdns/slavecommunicator.cc b/pdns/slavecommunicator.cc
index 0df99b0754..3b9bf656f1 100644
--- a/pdns/slavecommunicator.cc
+++ b/pdns/slavecommunicator.cc
@@ -232,7 +232,7 @@ static bool processRecordForZS(const DNSName& domain, bool& firstNSEC3, DNSResou
    5) It updates the Empty Non Terminals
 */
 
-vector<DNSResourceRecord> doAxfr(const ComboAddress& raddr, const DNSName& domain, const TSIGTriplet& tt, const ComboAddress& laddr,  scoped_ptr<AuthLua>& pdl, ZoneStatus& zs)
+static vector<DNSResourceRecord> doAxfr(const ComboAddress& raddr, const DNSName& domain, const TSIGTriplet& tt, const ComboAddress& laddr,  scoped_ptr<AuthLua>& pdl, ZoneStatus& zs)
 {
   vector<DNSResourceRecord> rrs;
   AXFRRetriever retriever(raddr, domain, tt, (laddr.sin4.sin_family == 0) ? NULL : &laddr, ((size_t) ::arg().asNum("xfr-max-received-mbytes")) * 1024 * 1024);
@@ -729,8 +729,7 @@ void CommunicatorClass::slaveRefresh(PacketHandler *P)
     // get the TSIG key name
     TSIGRecordContent trc;
     DNSName tsigkeyname;
-    string message;
-    dp.getTSIGDetails(&trc, &tsigkeyname, &message);
+    dp.getTSIGDetails(&trc, &tsigkeyname);
     int res;
     res=P->trySuperMasterSynchronous(&dp, tsigkeyname);
     if(res>=0) {
diff --git a/pdns/tcpreceiver.cc b/pdns/tcpreceiver.cc
index 0255c5b4e5..deb7a38994 100644
--- a/pdns/tcpreceiver.cc
+++ b/pdns/tcpreceiver.cc
@@ -479,7 +479,7 @@ bool TCPNameserver::canDoAXFR(shared_ptr<DNSPacket> q)
     TSIGRecordContent trc;
     DNSName keyname;
     string secret;
-    if(!checkForCorrectTSIG(q.get(), s_P->getBackend(), &keyname, &secret, &trc)) {
+    if(!q->checkForCorrectTSIG(s_P->getBackend(), &keyname, &secret, &trc)) {
       return false;
     } else {
       getTSIGHashEnum(trc.d_algoName, q->d_tsig_algo);
@@ -683,7 +683,7 @@ int TCPNameserver::doAXFR(const DNSName &target, shared_ptr<DNSPacket> q, int ou
   DNSName tsigkeyname;
   string tsigsecret;
 
-  bool haveTSIGDetails = q->getTSIGDetails(&trc, &tsigkeyname, 0);
+  bool haveTSIGDetails = q->getTSIGDetails(&trc, &tsigkeyname);
 
   if(haveTSIGDetails && !tsigkeyname.empty()) {
     string tsig64;
@@ -1197,7 +1197,7 @@ int TCPNameserver::doIXFR(shared_ptr<DNSPacket> q, int outsock)
     DNSName tsigkeyname;
     string tsigsecret;
 
-    bool haveTSIGDetails = q->getTSIGDetails(&trc, &tsigkeyname, 0);
+    bool haveTSIGDetails = q->getTSIGDetails(&trc, &tsigkeyname);
 
     if(haveTSIGDetails && !tsigkeyname.empty()) {
       string tsig64;
diff --git a/pdns/test-tsig.cc b/pdns/test-tsig.cc
new file mode 100644
index 0000000000..e1398bf9f2
--- /dev/null
+++ b/pdns/test-tsig.cc
@@ -0,0 +1,243 @@
+
+/*
+    PowerDNS Versatile Database Driven Nameserver
+    Copyright (C) 2013 - 2015  PowerDNS.COM BV
+
+    This program is free software; you can redistribute it and/or modify
+    it under the terms of the GNU General Public License version 2
+    as published by the Free Software Foundation
+
+    Additionally, the license of this program contains a special
+    exception which allows to distribute the program in binary form when
+    it is linked against OpenSSL.
+
+    This program is distributed in the hope that it will be useful,
+    but WITHOUT ANY WARRANTY; without even the implied warranty of
+    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+    GNU General Public License for more details.
+
+    You should have received a copy of the GNU General Public License
+    along with this program; if not, write to the Free Software
+    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+*/
+
+#define BOOST_TEST_DYN_LINK
+#define BOOST_TEST_NO_MAIN
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include <boost/test/unit_test.hpp>
+
+#include "dnssecinfra.hh"
+#include "dnswriter.hh"
+#include "misc.hh"
+#include "tsigverifier.hh"
+
+BOOST_AUTO_TEST_SUITE(tsig)
+
+static vector<uint8_t> generateTSIGQuery(const DNSName& qname, const DNSName& tsigName, const DNSName& tsigAlgo, const string& tsigSecret, uint16_t fudge=300, time_t tsigTime=time(nullptr))
+{
+  vector<uint8_t> packet;
+  DNSPacketWriter pw(packet, qname, QType::A);
+  pw.getHeader()->qr=0;
+  pw.getHeader()->rd=0;
+  pw.getHeader()->id=42;
+  pw.startRecord(qname, QType::A);
+  pw.xfr32BitInt(0x01020304);
+  pw.addOpt(512, 0, 0);
+  pw.commit();
+
+  TSIGTriplet tt;
+  tt.name = tsigName;
+  tt.algo = tsigAlgo;
+  tt.secret = tsigSecret;
+
+  TSIGHashEnum the;
+  BOOST_REQUIRE(getTSIGHashEnum(tt.algo, the));
+
+  TSIGRecordContent trc;
+  trc.d_algoName = getTSIGAlgoName(the);
+  trc.d_time = tsigTime;
+  trc.d_fudge = fudge;
+  trc.d_origID = ntohs(pw.getHeader()->id);
+  trc.d_eRcode = 0;
+
+  addTSIG(pw, trc, tt.name, tt.secret, "", false);
+  return packet;
+}
+
+static void checkTSIG(const DNSName& tsigName, const DNSName& tsigAlgo, const string& tsigSecret, const vector<uint8_t>& packet, const string* overrideMac=nullptr, uint16_t* overrideExtendedRCode=nullptr, uint16_t* overrideOrigID=nullptr)
+{
+  string packetStr(reinterpret_cast<const char*>(packet.data()), packet.size());
+  MOADNSParser mdp(true, packetStr);
+
+  bool tsigFound = false;
+  string theirMac;
+  DNSName keyName;
+  TSIGRecordContent trc;
+
+  for(const auto& answer: mdp.d_answers) {
+    if(answer.first.d_type == QType::TSIG) {
+      BOOST_CHECK_EQUAL(answer.first.d_place, DNSResourceRecord::ADDITIONAL);
+      BOOST_CHECK_EQUAL(answer.first.d_class, QClass::ANY);
+      BOOST_CHECK_EQUAL(answer.first.d_ttl, 0);
+      BOOST_CHECK_EQUAL(tsigFound, false);
+
+      shared_ptr<TSIGRecordContent> rectrc = getRR<TSIGRecordContent>(answer.first);
+      if (rectrc) {
+        trc = *rectrc;
+        theirMac = rectrc->d_mac;
+        keyName = answer.first.d_name;
+        tsigFound = true;
+      }
+    }
+  }
+
+  if (overrideMac) {
+    theirMac = *overrideMac;
+  }
+
+  if (overrideOrigID) {
+    trc.d_origID = *overrideOrigID;
+  }
+
+  if (overrideExtendedRCode) {
+    trc.d_eRcode = *overrideExtendedRCode;
+  }
+
+  BOOST_REQUIRE(tsigFound);
+  TSIGTriplet tt;
+  tt.name = tsigName;
+  tt.algo = tsigAlgo;
+  tt.secret = tsigSecret;
+
+  BOOST_CHECK(validateTSIG(packetStr, mdp.getTSIGPos(), tt, trc, "", theirMac, false));
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_valid) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+
+  checkTSIG(tsigName, tsigAlgo, tsigSecret, packet);}
+
+
+BOOST_AUTO_TEST_CASE(test_TSIG_different_case_algo) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+
+  checkTSIG(tsigName, tsigAlgo.makeLowerCase(), tsigSecret, packet);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_different_name_same_algo) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+
+  checkTSIG(tsigName, DNSName("hmac-md5."), tsigSecret, packet);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_bad_key_name) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+
+  BOOST_CHECK_THROW(checkTSIG(DNSName("another.tsig.key.name"), tsigAlgo, tsigSecret, packet), std::runtime_error);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_bad_algo) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+
+  BOOST_CHECK_THROW(checkTSIG(tsigName, DNSName("hmac-sha512."), tsigSecret, packet), std::runtime_error);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_bad_secret) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+
+  BOOST_CHECK_THROW(checkTSIG(tsigName, tsigAlgo, "bad secret", packet), std::runtime_error);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_bad_ercode) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+  uint16_t badERcode = 1;
+
+  BOOST_CHECK_THROW(checkTSIG(tsigName, tsigAlgo, tsigSecret, packet, nullptr, &badERcode), std::runtime_error);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_bad_origID) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+  uint16_t badOrigID = 1;
+
+  BOOST_CHECK_THROW(checkTSIG(tsigName, tsigAlgo, tsigSecret, packet, nullptr, nullptr, &badOrigID), std::runtime_error);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_bad_mac) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret);
+
+  string badMac = "badmac";
+  BOOST_CHECK_THROW(checkTSIG(tsigName, tsigAlgo, tsigSecret, packet, &badMac), std::runtime_error);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_signature_expired) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret, 5, time(nullptr) - 10);
+
+  BOOST_CHECK_THROW(checkTSIG(tsigName, tsigAlgo, tsigSecret, packet), std::runtime_error);
+}
+
+BOOST_AUTO_TEST_CASE(test_TSIG_signature_too_far_in_the_future) {
+  DNSName tsigName("tsig.name");
+  DNSName tsigAlgo("HMAC-MD5.SIG-ALG.REG.INT");
+  DNSName qname("test.valid.tsig");
+  string tsigSecret("verysecret");
+
+  vector<uint8_t> packet = generateTSIGQuery(qname, tsigName, tsigAlgo, tsigSecret, 5, time(nullptr) + 20);
+
+  BOOST_CHECK_THROW(checkTSIG(tsigName, tsigAlgo, tsigSecret, packet), std::runtime_error);
+}
+
+BOOST_AUTO_TEST_SUITE_END();
diff --git a/pdns/tsig-tests.cc b/pdns/tsig-tests.cc
index b0306b593b..cf2c7809b6 100644
--- a/pdns/tsig-tests.cc
+++ b/pdns/tsig-tests.cc
@@ -55,42 +55,10 @@ try
   trc.d_origID=ntohs(pw.getHeader()->id);
   trc.d_eRcode=0;
 
-  addTSIG(pw, &trc, keyname, key, "", false);
+  addTSIG(pw, trc, keyname, key, "", false);
 
   Socket sock(AF_INET, SOCK_DGRAM);
   ComboAddress dest(argv[1] + (*argv[1]=='@'), atoi(argv[2]));
-#if 0
-  sock.sendTo(string((char*)&*packet.begin(), (char*)&*packet.end()), dest);
-  
-  string reply;
-  sock.recvFrom(reply, dest);
-
-  MOADNSParser mdp(false, reply);
-  cout<<"Reply to question for qname='"<<mdp.d_qname<<"', qtype="<<DNSRecordContent::NumberToType(mdp.d_qtype)<<endl;
-  cout<<"Rcode: "<<mdp.d_header.rcode<<", RD: "<<mdp.d_header.rd<<", QR: "<<mdp.d_header.qr;
-  cout<<", TC: "<<mdp.d_header.tc<<", AA: "<<mdp.d_header.aa<<", opcode: "<<mdp.d_header.opcode<<endl;
-
-  shared_ptr<TSIGRecordContent> trc2;
-  
-  for(MOADNSParser::answers_t::const_iterator i=mdp.d_answers.begin(); i!=mdp.d_answers.end(); ++i) {          
-    cout<<i->first.d_place-1<<"\t"<<i->first.d_label<<"\tIN\t"<<DNSRecordContent::NumberToType(i->first.d_type, i->first.d_class);
-    cout<<"\t"<<i->first.d_ttl<<"\t"<< i->first.d_content->getZoneRepresentation()<<"\n";
-    
-    if(i->first.d_type == QType::TSIG)
-      trc2 = std::dynamic_pointer_cast<TSIGRecordContent>(i->first.d_content);
-  }
-
-  if(mdp.getTSIGPos()) {    
-    string message = makeTSIGMessageFromTSIGPacket(reply, mdp.getTSIGPos(), keyname, trc, trc.d_mac, false); // insert our question MAC
-    
-    string hmac2=calculateMD5HMAC(key, message);
-    cerr<<"Calculated mac: "<<Base64Encode(hmac2)<<endl;
-    if(hmac2 == trc2->d_mac)
-      cerr<<"MATCH!"<<endl;
-    else 
-      cerr<<"Mismatch!"<<endl;
-  }
-#endif
   seedRandom("/dev/urandom");
   cerr<<"Keyname: '"<<keyname.toString()<<"', algo: '"<<trc.d_algoName.toString()<<"', key: '"<<Base64Encode(key)<<"'\n";
   TSIGTriplet tt;
diff --git a/pdns/tsigverifier.cc b/pdns/tsigverifier.cc
index 3daeef47b5..ce01c434b4 100644
--- a/pdns/tsigverifier.cc
+++ b/pdns/tsigverifier.cc
@@ -33,6 +33,8 @@ bool TSIGTCPVerifier::check(const string& data, const MOADNSParser& mdp)
         theirMac = trc->d_mac;
         d_trc.d_time = trc->d_time;
         d_trc.d_fudge = trc->d_fudge;
+        d_trc.d_eRcode = trc->d_eRcode;
+        d_trc.d_origID = trc->d_origID;
         checkTSIG = true;
       }
     }
@@ -47,34 +49,17 @@ bool TSIGTCPVerifier::check(const string& data, const MOADNSParser& mdp)
       throw std::runtime_error("No TSIG on AXFR response from "+d_remote.toStringWithPort()+" , should be signed with TSIG key '"+d_tt.name.toString()+"'");
     }
 
-    uint64_t delta = std::abs((int64_t)d_trc.d_time - (int64_t)time(nullptr));
-    if(delta > d_trc.d_fudge) {
-      throw std::runtime_error("Invalid TSIG time delta " + std::to_string(delta) + " >  fudge " + std::to_string(d_trc.d_fudge));
-    }
-    string message;
-    if (!d_prevMac.empty()) {
-      message = makeTSIGMessageFromTSIGPacket(d_signData, d_tsigPos, d_tt.name, d_trc, d_prevMac, true, d_signData.size()-data.size());
-    } else {
-      message = makeTSIGMessageFromTSIGPacket(d_signData, d_tsigPos, d_tt.name, d_trc, d_trc.d_mac, false);
-    }
-
-    TSIGHashEnum algo;
-    if (!getTSIGHashEnum(d_trc.d_algoName, algo)) {
-      throw std::runtime_error("Unsupported TSIG HMAC algorithm " + d_trc.d_algoName.toString());
-    }
-
-    if (algo == TSIG_GSS) {
-      GssContext gssctx(d_tt.name);
-      if (!gss_verify_signature(d_tt.name, message, theirMac)) {
-        throw std::runtime_error("Signature failed to validate on AXFR response from "+d_remote.toStringWithPort()+" signed with TSIG key '"+d_tt.name.toString()+"'");
+    try {
+      if (!d_prevMac.empty()) {
+        validateTSIG(d_signData, d_tsigPos, d_tt, d_trc, d_prevMac, theirMac, true, d_signData.size()-data.size());
       }
-    } else {
-      string ourMac=calculateHMAC(d_tt.secret, message, algo);
-
-      if(!constantTimeStringEquals(ourMac, theirMac)) {
-        throw std::runtime_error("Signature failed to validate on AXFR response from "+d_remote.toStringWithPort()+" signed with TSIG key '"+d_tt.name.toString()+"'");
+      else {
+        validateTSIG(d_signData, d_tsigPos, d_tt, d_trc, d_trc.d_mac, theirMac, false);
       }
     }
+    catch(const std::runtime_error& err) {
+      throw std::runtime_error("Error while validating TSIG signature on AXFR response from "+d_remote.toStringWithPort()+":"+err.what());
+    }
 
     // Reset and store some values for the next chunks.
     d_prevMac = theirMac;