Consider the following:
sig := &Signature{
SigType: SigTypeDirectSignature,
PubKeyAlgo: publicV6Key.PubKeyAlgo,
Hash: crypto.SHA256,
FlagsValid: true,
FlagCertify: true,
}
err = sig.SetSalt([]byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15})
if err != nil {
t.Errorf("failed to set salt: %v", err)
}
err = sig.SignDirectKeyBinding(publicV6Key, privateV6Key, nil)
if err != nil {
t.Errorf("failed to sign key: %v", err)
}
sig now contains an invalid (unsalted) signature, but no errors have been triggered. How?
(tl;dr: sig.Version == 0)
This happens for all of the public signature methods, not just SignDirectKeyBinding, and produces the same result if you call sig.SetSalt explicitly or not.
AFAICT:
- sig.SetSalt() silently does nothing if sig.Version != 6
- sig.PrepareSign() will override a nil salt but only if sig.Version == 6
- sig.Sign() populates sig.Version from the signing key version, but it is always invoked after sig.PrepareSign().
Therefore, the high-level signing methods can use a nil salt if sig.Version is not explicitly initialised, even though they "successfully" produce a v6 signature.
(If you set sig.salt directly in the literal, it produces garbage, because the salt field in the signature packet is now populated, but it still isn't included in the digest. I do appreciate that most callers won't be able to write directly to a private member though.)
One solution would be to check for sig.Version == 0 in PrepareSign() - it would be relatively simple to replace the existing if with a switch. This would at least throw an error if the caller forgets to set sig.Version, but it wouldn't help if the caller set it to an inconsistent value (this is also an issue with the PubKeyAlgo field).
A more robust solution would be to pass the signing public key as an argument to PrepareSign(), and copy the line that initialises sig.Version from Sign() to PrepareSign(), ensuring that sig.Version is always set before it is used. I suspect this was the original intent of Sign(), but when PrepareSign() was added for v6 it broke the implicit assumptions. This would be a breaking API change though, because PrepareSign is a public method.
Consider the following:
signow contains an invalid (unsalted) signature, but no errors have been triggered. How?(tl;dr: sig.Version == 0)
This happens for all of the public signature methods, not just SignDirectKeyBinding, and produces the same result if you call sig.SetSalt explicitly or not.
AFAICT:
Therefore, the high-level signing methods can use a nil salt if sig.Version is not explicitly initialised, even though they "successfully" produce a v6 signature.
(If you set sig.salt directly in the literal, it produces garbage, because the salt field in the signature packet is now populated, but it still isn't included in the digest. I do appreciate that most callers won't be able to write directly to a private member though.)
One solution would be to check for sig.Version == 0 in PrepareSign() - it would be relatively simple to replace the existing if with a switch. This would at least throw an error if the caller forgets to set sig.Version, but it wouldn't help if the caller set it to an inconsistent value (this is also an issue with the PubKeyAlgo field).
A more robust solution would be to pass the signing public key as an argument to PrepareSign(), and copy the line that initialises sig.Version from Sign() to PrepareSign(), ensuring that sig.Version is always set before it is used. I suspect this was the original intent of Sign(), but when PrepareSign() was added for v6 it broke the implicit assumptions. This would be a breaking API change though, because PrepareSign is a public method.