RustCrypto / formats

Cryptography-related format encoders/decoders: DER, PEM, PKCS, PKIX
229 stars 122 forks source link

const-oid: an unfortunate tradeoff in `PartialEq`/`Eq` definitions #1293

Open tarcieri opened 6 months ago

tarcieri commented 6 months ago

1212 switched from derived PartialEq/Eq impls on ObjectIdentifier to handwritten ones which are generic and allow two ObjectIdentifiers with different backing storage to be compared, which makes it possible to used slice-backed storage for the database.

However, in trying to upgrade the other crates in this repo I ran into the following limitation: only constants with a derived PartialEq/Eq implementation can be used in match statements:

error: to use a constant of type `spki::ObjectIdentifier<Buffer<39>>` in a pattern, `spki::ObjectIdentifier<Buffer<39>>` must be annotated with `#[derive(PartialEq, Eq)]`
   --> pkcs5/src/pbes1.rs:192:13
    |
192 |             PBE_WITH_MD2_AND_DES_CBC_OID => Ok(Self::PbeWithMd2AndDesCbc),
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: the traits must be derived, manual `impl`s are not sufficient
    = note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralEq.html for details

To go this route, all of these match statements would need to be rewritten with if statements, which is doable but slightly less than ideal.

Perhaps instead of allowing the storage to be fully generic, it could just be const generic around the size of the backing buffer, which would still allow OIDs in match statements so long as their backing buffer size were the same.

However, the database requires OIDs to all be the same type, so that would preclude the optimization that the slice-backed storage was going for: making sure each OID only uses as much memory as is required, rather than having additional unused overhead.

tarcieri commented 6 months ago

For now I plan to revert #1212 which will get us the derived PartialEq/Eq back.

Going forward, I think it may make sense to make ObjectIdentifierRef its own type (i.e. a newtype around a byte slice), and make ObjectIdentifier simply const generic (with a default) around its buffer size, rather than the current approach of ObjectIdentifierRef being a type alias for an ObjectIdentifier which is generic around backing storage. The duplication wouldn't be too bad since much of the core functionality is in the Arcs struct anyway (which defines the iterator over the BER encoding).

That would allow a derived PartialEq/Eq impl, but also overlapping impls to compare ObjectIdentifier and ObjectIdentifierRef (and vice versa).

tarcieri commented 6 months ago

1299 backs out the usages of ObjectIdentifierRef in the database until such a time as the concerns in this issue have been addressed