tor-keymgr: More API changes to support certs
This is a (potentially controversial) refactoring around tor-key-forge
and tor-keymgr
.
This all came out of trying to make the cert management code introduced in !2644 (merged) function correctly,
and parse the certs read from the keystore, before finally validating them using the provided ToEncodableCert::validate()
implementation. The full context is in !2644 (comment 3138175)
To parse a byte slice as a Tor ed25519 cert, we can use Ed25519Cert::decode()
, which gives us a KeyUnknownCert
. This means we have an asymmetry between the cert type that can be encoded and written to the keystore (EncodedEd25519Cert
), and the type of a not-yet-validated cert read from the keystore (KeyUnknownCert
). There is no way to go from KeyUnknownCert
-> EncodedEd25519Cert
without validating the cert first (and FTR, I don't think there should be one, as that would mean introducing some ugly dangerously_assume_valid
APIs). This means we have two encodings for the same certificate type, which is very very inconvenient, because the EncodableItem
trait assumes a keystore item can be encoded to/from the same type in one operation, which is a problem. We can solve by having two cert types on ToEncodableCert
, one which is an EncodableItem
, and one which is not (this is c9207aed). The two cert types will look like for the ToEncodableCert
impl for the future RelaySigningKeyCert
type:
look like this:
pub struct RelaySigningKeyCert(EncodedEd25519Cert);
impl ToEncodableCert<RelaySigningKeypair> for RelaySigningKeyCert {
// This one doesn't implement EncodableItem (we can't encode it!)
type ParsedCert = KeyUnknownCert;
// This one *does* implement EncodableItem (it's trivially encodable)
type EncodableCert = EncodedEd25519Cert;
type SigningKey = RelayIdentityKeypair;
fn validate(
cert: Self::ParsedCert,
subject: &RelaySigningKeypair,
signed_with: &Self::SigningKey,
) -> Result<Self, InvalidCertError> {
// TODO: validate `KeyUnknownCert`
// and convert it to an EncodedEd25519Cert
// (we don't yet an easy way to perform this conversion)
}
fn to_encodable_cert(self) -> Self::EncodableCert {
self.0
}
}
I have also collapsed ToEncodableCert::validate
and ToEncodableCert::from_encodable_cert
into the same operation (see 90425b32), because the validation and conversion are usually one and the same operation (because our validation logic usually involves transforming an UnvalidatedFooCert
into a ValidatedFooCert
).
Finally, the commit that actually fixes #1768 is c8290f9f.
There are several alternative approaches to this, but IMO they are all uglier and end up with "should not be reachable" error code paths. For example, I have a separate branch where I experimented using a new cert type that is an enum over the two KeyUnknownCert
and EncodedEd25519Cert
certificate representations. In that branch, I've let the enum type implement EncodableItem
as before, but that meant having an error case for KeyUnknownCert
, because KeyUnknownCert
can't possibly be encoded to bytes (it doesn't have a to_bytes()
method, and besides, we don't want to be writing potentially invalid certs to the keystore). Here is an example of unreachable code path from this alternative approach gabi-250/arti@b0bec6ea (and here is the enum type I mentioned gabi-250/arti@c712d881)
Closes #1768