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

Merged
merged 3 commits into from May 3, 2016

Projects

None yet

3 participants

@rgacogne
Member
rgacogne commented May 3, 2016

As reported by @mind04, 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)

Closes #3409.

@rgacogne rgacogne 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)
8ca3ea3
@rgacogne rgacogne added the auth label May 3, 2016
@zeha
Collaborator
zeha commented May 3, 2016

Logic checks out. I'd think the operations in pdnsutil hsm that call makeFromISCString would also benefit from calling checkKey.

@Habbie Habbie and 1 other commented on an outdated diff May 3, 2016
pdns/opensslsigners.cc
if (d_key)
RSA_free(d_key);
d_key = key;
}
+bool OpenSSLRSADNSCryptoKeyEngine::checkKeys() const
@Habbie
Habbie May 3, 2016 Member

shouldn't this be checkKey?

@rgacogne
rgacogne May 3, 2016 Member

Could be, it checks a key-pair but we mostly refer to it as key, so you are probably right, I'll change that along with the hsm stuff!

@Habbie
Member
Habbie commented May 3, 2016

LGTM, if you are indeed done, go ahead and merge :)

@rgacogne rgacogne merged commit e1c3ccf into PowerDNS:master May 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgacogne rgacogne deleted the rgacogne:fromiscmap-nocheck branch May 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment