pdns
pdns copied to clipboard
PowerDNS
auth: Speed up ECDSA and RSA signatures
Short description
For ECDSA, and likely for RSA, computing the public key is not a cheap operation. So instead of computing it twice to get the lookup key for our signatures cache, reuse the computed public key and only compute its digest.
In addition, since ed* algorithms were already using the whole key instead of a digest, place the cut off at public keys larger than 64 bytes, meaning that only RSA ones (128+ bytes) will be hashed.
This provides an additional speedup for ECDSA keys (32 or 48 bytes) since they no longer need to be hashed, and simplifies the signers code as the hashing can be moved to signature key cache now that it only depends on they key size.
For reference the size of a SHA-1 digest is 20 bytes.
In my tests this reduces by 30% the cost of calling addRRSigs() for ECDSA signatures when the signature is already present in the cache.
Checklist
I have:
- [x] read the CONTRIBUTING.md document
- [x] compiled this code
- [x] tested this code
- [ ] included documentation (including possible behaviour changes)
- [x] documented the code
- [ ] added or modified regression test(s)
- [ ] added or modified unit test(s)
My understanding is that because ECDSA keys are "short" (< 64 bytes), hashing them is a waste so we can save on that.
Yes, but that's only a part of the gain in this pull request (1/5th, if I recall correctly). The biggest win is that we no longer compute the public key from the private key twice:
- in
DNSKEYRecordContent drc = dpk.getDNSKEY(); - in
getPubKeyHash()
Instead we now use the public key copy contained in drc.
But for RSA keys which may or may not be short, we now decide to hash depending on the key length, which seems reasonable. Is my understanding correct? If it is, doesn't that also offer an occasional performance improvement for RSA keys?
It would for very small RSA keys, but 512-bit keys are already just above the 64-byte limit so I don't think it will actually help in practice. But the other optimization, computing the public key once, should help.
One last point, is SHA1 still a good thing for hashing? I assume yes, but just not good for "cryptographically-secure hashing" anymore, correct?
SHA-1 is clearly broken for cryptographic applications indeed. In that case the likelihood of getting a collision between the keys that are in our database combined with a collisions on the content to sign seems small. I guess the threat model would be an authenticated user deciding to compute a collision on a key from a different zone hosted on the same server to cause a denial of service of that zone? If we are really worried about that we could add a random salt computed on startup, or something like that, I suppose?
If we are really worried about that we could add a random salt computed on startup, or something like that, I suppose?
That would be a solution but I don't know how likely (or how dangerous) the scenario you describe is.
Just a note as I was myself confused about that recently while discussing this PR: the computation that this PR is making faster happens before the lookup into the signature cache, as this is the hash that we actually use as (part of) the key into the signature cache.
I rebased this branch on master to avoid the CI failure due to python's protobuf update. I also added a new commit pre-computing the DNSKEYRecordContent (which involves computing the public key) once, when DNSSECPrivateKey::setKet() is called, instead of doing it every time. This results in a very nice speed-up in my tests, but one big drawback is that setKey() should NO LONGER be used before the algo and flags have been set. Perhaps it would make sense to modify setKey()'s signature so that it takes these as arguments, making it clear that they are required to be set?
one big drawback is that
setKey()should NO LONGER be used before the algo and flags have been set. Perhaps it would make sense to modifysetKey()'s signature so that it takes these as arguments, making it clear that they are required to be set?
Fixed that by in a new commit:
- adding a comment to the
setKeydeclaration - requiring the flags as a
setKeyparameter
This will require quite the rebase for my openssl-3.0 branch but it's OK. Let's get it merged and I'll deal with it.
