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

pkcs8: provide PrivateKeyInfoRef/PrivateKeyInfoOwned #1183

Closed

Conversation

baloo
Copy link
Member

@baloo baloo commented Aug 4, 2023

see #1117

der/src/bytes_owned.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

tarcieri commented Aug 4, 2023

Why are you exposing BytesRef/BytesOwned?

Why not OctetString/OctetStringRef ala spki (i.e. the ASN.1 type, though it's BIT STRING there), or if not that then &'a [u8] and Vec<u8>?

Really BytesRef and BytesOwned exist for one reason: because they pre-check that the Length is valid and thus permit operations which would otherwise be fallible. Otherwise they're just analogues for &'a [u8] and Vec<u8>.

@baloo baloo force-pushed the baloo/pkcs8/owned-ref-private-key branch from 7ae4fa6 to 1cf577e Compare August 4, 2023 19:09
@baloo
Copy link
Member Author

baloo commented Aug 4, 2023

I was actually looking for a &'a [u8]/Box<[u8]> tuple. But the BytesRef/BytesOwned was looking like the closest to that.

I don't think I'm too comfortable with OctetString, went with just the &'a [u8]/Box<[u8]> and implement RefToOwned there.

tarcieri and others added 11 commits August 8, 2023 20:55
Removes lifetimes from all types in the `pkcs5` crate, making them own
their data.
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
@dishmaker
Copy link

dishmaker commented Jun 4, 2024

Why not OctetString/OctetStringRef

Why not merge OctetString and OctetStringRef into something like Cow<[u8]> ?
With minor CPU overhead an enum would be easier to maintain.

Regular OctetString would just contain Cow<'static, [u8]> which is Vec<u8>
Ref OctetString would contain &'a [u8]

https://doc.rust-lang.org/std/vec/struct.Vec.html#impl-From%3CCow%3C'a,+%5BT%5D%3E%3E-for-Vec%3CT%3E

Also, conversions from/to owned would just swap an enum.

@tarcieri
Copy link
Member

tarcieri commented Jun 4, 2024

@dishmaker this crate is designed to support heapless targets that don’t have alloc available, which OctetStringRef is capable of working on

@dishmaker
Copy link

dishmaker commented Jun 4, 2024

Ohhh so such Cow feature needs a kill switch 😢

struct BytesRef<'a> {

    pub length: Length,

    #[cfg(feature = "alloc")]
    pub inner: Cow<'a, [u8]>,

    #[cfg(not(feature = "alloc"))]
    pub inner: &'a [u8],
}

@tarcieri
Copy link
Member

tarcieri commented Jun 4, 2024

But then you haven't actually gotten rid of the *Ref type, and since it's immutable anyway, there's no "write" case to worry about, so Cow doesn't buy you anything.

Anyway, if you'd like to keep discussing this, it would probably be better to open a separate issue.

@baloo baloo closed this Aug 18, 2024
@tarcieri
Copy link
Member

@baloo are you going to redo this? I think it would be great to get in before the next stable release series

@baloo
Copy link
Member Author

baloo commented Aug 18, 2024

I guess I could, I honestly lost track.

@tarcieri
Copy link
Member

Aah ok, I'm down to take it then. Seems like it will be a bit of a mess but worth it.

@baloo
Copy link
Member Author

baloo commented Aug 18, 2024

The rebase looks like it's a mess indeed.

I can give it a shot, you have a fair amount of things on your hand.

@tarcieri
Copy link
Member

Sure!

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

Successfully merging this pull request may close these issues.

3 participants