Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

der: Any vs AnyRef: issues with generating AlgorithmIdentifier's parameters on the fly #731

Closed
lumag opened this issue Oct 8, 2022 · 11 comments

Comments

@lumag
Copy link
Contributor

lumag commented Oct 8, 2022

I tried writing a function that returns complete AlgorithmIdentifier for the RSASSA-PSS algorithm. However my attempts to write a sensible one have failed up to now, as it's not possible to embed an owned value into the parameters field (AnyRef). I ended up declaring static byte array with the PSS parameters in DER encoding, but it is not really intuitive.

@tarcieri
Copy link
Member

tarcieri commented Oct 8, 2022

Yeah, in the next breaking release of spki I'd like to make parameters generic (with a default of AnyRef) so you can plug in your own parameters type

@tarcieri
Copy link
Member

tarcieri commented Oct 8, 2022

Note that this might be possible to do in a non-breaking manner today if you'd like to experiment with it

@lumag
Copy link
Contributor Author

lumag commented Oct 8, 2022

Do you have any particular ideas/suggestions?

I wanted to make add the ability to use any ASN.1 tag in place of Any/AnyRef, with AnyRef being used only during parsing. However this might be a very intrusive change.

@tarcieri
Copy link
Member

tarcieri commented Oct 8, 2022

I'm suggesting replacing AlgorithmIdentifier::parameters, which is currently Option<AnyRef<'a>>, with a generic parameter e.g. P: Decode<'a> + Encode + Tagged.

AnyRef<'a> can be the default, and in theory this change could be made in a non-breaking manner.

@lumag
Copy link
Contributor Author

lumag commented Oct 8, 2022

@tarcieri although it's a separate issue, the Certificate has more or less the same issue. I can create an instance in a function, but then I can not set the signature field in the same function, as the BitStringRef doesn't take data ownership. I feel like the der crate needs some redesign.

(in case of Certificate I ended up returning encoded Vec<u8> rather than the Certificate itself)

@tarcieri
Copy link
Member

@lumag the alternative to using a BitStringRef is using an owned BitString. That's the purpose of having separate owned and borrowed types in the der crate. I'm not sure what potential "redesign" of the der crate you're alluding to other than having owned types, which already exist.

There's a tradeoff to be made there: owned types aren't zero-copy, but if you're decoding from PEM zero-copy isn't possible anyway.

The reason the *Ref types are preferred in most of the crates in this repo is to enable no_std support for embedded heapless targets. However, the x509-cert crate has a hard dependency on alloc anyway, so switching from the borrowed to owned types should be fine.

Making AlgorithmIdentifier::parameters a generic type parameter would let you use either owned or borrowed types depending on what best fits a particular application.

@tarcieri
Copy link
Member

tarcieri commented Oct 10, 2022

I took a look at trying to make the parameters generic.

AFAICT it's not possible without a breaking change, because it involves removing the 'a lifetime, since it's never used in the type itself.

While I could add a PhantomData so it is used, that would involve adding another field to AlgorithmIdentifier, which would also be a breaking change since it presently has all pub fields.

So I think our best bet is to remove the lifetime in the next breaking release, and add a AlgorithmIdentifierRef or thereabouts which has a lifetime and uses AnyRef as the concrete parameter.

@lumag
Copy link
Contributor Author

lumag commented Oct 10, 2022

@tarcieri I did not try to implement any actual code proposals. However having the distinct Foo and FooRef means that we have to duplicate the code. Afer AlgorithmIdentifierRef we'll have to add e.g. TbsCertificateRef, CerficateRef, etc. I'm not sure that this is the best approach. Just think about the PKCS#7. Having to duplicate all these types would be a complete disaster IMO.

@tarcieri
Copy link
Member

tarcieri commented Oct 10, 2022

The owned vs borrowed distinction is pretty fundamental to Rust: the duplication occurs because borrowed types have a lifetime and owned types don't.

It's also something you'll see in most Rust serialization frameworks including serde. The der crate's Decode vs DecodeOwned are modeled off serde's Deserialize/DeserializeOwned for example.

Generics make it possible to abstract over the distinction in many cases, including AlgorithmIdentifier::parameters. In the spki crate it's important to support both as it targets heapless no_std embedded devices where it's necessary to borrow from the input in some cases, e.g. in the pkcs5 crate. The Borrow trait is also useful for abstracting over T vs &T, especially as it's auto-impl'd for both.

However, for the x509-cert crate we don't need to duplicate TbsCertificateRef, CerficateRef etc as that crate targets alloc as a baseline, so we can just use owned types for everything, as is common with serialization frameworks like prost and serde.

Or rather, if we do (re-)add reference-based types to x509-cert, they would be added later in service of a hypothetical no-alloc heapless mode: #689

@tarcieri
Copy link
Member

tarcieri commented Oct 10, 2022

Alternatively, there are crates for abstracting over the owned/borrowed distinction, for example: https://docs.rs/yoke/

That might work well with der::Document, although that type is immutable, so the main thing it would provide is the ability to parse a der::Document once and retain a data structure which borrows from its contents, erasing the lifetime and allowing the whole thing to behave as if it's owned.

Edit: opened #734 to discuss a potential yoke integration.

@tarcieri
Copy link
Member

I think this has been addressed by #769, #790, and #799

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants