[WIP] der: generic `Document` type #427

tarcieri · 2022-02-12T02:30:33Z

Changes Document from a trait into a generic type.

The previous approach necessitated a lot of repetitive boilerplate for each "document" type. Using a generic type eliminates the boilerplate.

The definition was somewhat tricky due to the lifetime on Decodable<'a>. For more context, see #421.

tarcieri · 2022-02-12T02:30:50Z

NOTE: early WIP. Not expected to pass tests yet, but it compiles.

tarcieri · 2022-02-12T02:33:09Z

der/src/document.rs

+/// Marker trait which identifies documents which contain sensitive
+/// information, such as private keys.
+pub trait Sensitivity {
+    /// Does this type contain potentially sensitive data?
+    ///
+    /// This enables hardened file permissions when persisting data to disk.
+    const SENSITIVE: bool;
+}


Marker trait for types which represent private keys. The effects should probably be better documented in rustdoc, but basically come down to:

Zeroizing der_bytes when the zeroize crate feature is enabled

Setting appropriate file permissions when writing DER or PEM data to disk

Alternatively this could be a const generic parameter. It's something to consider, but exposing the information under a trait might be useful in other settings.

If we gave this trait another name, we could also include the PEM "type" as a &'static str here too.

Changes `Document` from a trait into a generic type. The previous approach necessitated a lot of repetitive boilerplate for each "document" type. Using a generic type eliminates the boilerplate. The definition was somewhat tricky due to the lifetime on `Decodable<'a>`. For more context, see #421.

npmccallum

@tarcieri Over all, I like this direction! Here's some thoughts and various nitpicks.

npmccallum · 2022-02-12T14:00:25Z

der/src/document.rs

+/// Marker trait which identifies documents which contain sensitive
+/// information, such as private keys.
+pub trait Sensitivity {
+    /// Does this type contain potentially sensitive data?
+    ///
+    /// This enables hardened file permissions when persisting data to disk.
+    const SENSITIVE: bool;
+}


If we gave this trait another name, we could also include the PEM "type" as a &'static str here too.

npmccallum · 2022-02-12T14:01:39Z

der/src/document.rs

+    /// Borrow the inner serialized bytes of this document.
+    pub fn as_der(&self) -> &[u8] {
+        self.der_bytes.as_slice()
    }


We don't actually know if the bytes are der because we haven't decoded them yet. Perhaps we should just let AsRef or even Deref do the heavy lifting here?

If you take a look at the definition of from_der, you'll see it checks the document decodes using doc.try_decode() before returning it to the caller. The type definitely does maintain the invariant that self.der_bytes will always decode successfully, and as I noted in #421, it may be possible to store the parsed message to prevent repeat decoding (definitely not going to attempt that in this PR).

Also for some reason GitHub isn't letting me reply to your comment about the PEM label, but there's already a PemLabel trait for that.

And FWIW locally I'm trying to replace the trait with a const generic parameter, and liking that better for now.

npmccallum · 2022-02-12T14:05:32Z

der/src/document.rs

+        // Ensure document parses successfully
+        doc.try_decode()?;
+        Ok(doc)
+    }


I don't think we should try decoding the der. This has performance and security implications. I think the Document type should only have the invariant that it contains bytes successfully decoded from a PEM file. The important bit is that T is linked via the type system to the PEM document "name" and that we know the sensitivity of the bytes.

npmccallum · 2022-02-12T14:09:42Z

der/src/document.rs

+        T: Decodable<'a> + Sized,
+    {
+        T::from_der(self.as_der())
+    }


I think we should ditch that invariant altogether.

That defeats the whole point. Then it's no better than a Vec<u8>.

Document combines three functionalities:

Zeroing the Vec<u8>. This has clear value.

Ensuring file permissions on write. This has clear value. I have some minor concern, however, that this is only useful in a narrow set of cases (i.e. where you want to do full, synchronous io). If you want to do partial io (such as a certificate chain incremental io) or asynchronous io, this type drops this value.

Ensuring the bytes will successfully decode to T. The value of this is unclear to me. And it carries a performance and security cost.

Dropping the third guarantee still leaves Document with two features. So it is still better than a Vec<u8>.

Ensuring the bytes will successfully decode to T. The value of this is unclear to me. A

The goal is to have something that's both type safe and owned, as opposed to a newtype for a bag-of-bytes which may or may not parse successfully as DER. See your own comment about that above.

Similar concerns came up with code review on the yubikey crate.

And it carries a performance and security cost.

Security cost? How so?

The performance cost can be obviated by storing the parsed result, although that's tricky due to self-borrowing.

All that said, I'm not sure this will actually work out, but I'll leave a comment about that below.

npmccallum · 2022-02-12T14:15:13Z

der/src/document.rs

+            self.der_bytes.zeroize();
+        }
+    }
+}


It would probably be easier to just unconditionally Zeroize<Vec<u8>>. I presume you're worried about performance of zeroing large certificates. If you're willing to take the performance tradeoffs, zeroing even the non-sensitive bits has security benefits. For example, attackers often search for the public key to correlate the private key's memory position (or the position of some of its non-zeroed fragments). Zeroing both adds one additional hurdle.

It's an option, but the sensitivity information is already there for other purposes, so it seems ok to me to make use of it for this application as well.

tarcieri · 2022-02-12T14:33:49Z

Pushed up a version that uses a const generic SENSITIVE parameter.

I like it: it allows specializing things like trait impls depending on whether or not SENSITIVE is false or true. I used that for the Debug impls, which can also have slightly different bounds (a SENSITIVE document doesn't need a Debug impl for the inner type, for example)

npmccallum · 2022-02-12T14:47:20Z

Pushed up a version that uses a const generic SENSITIVE parameter.

I like it: it allows specializing things like trait impls depending on whether or not SENSITIVE is false or true. I used that for the Debug impls, which can also have slightly different bounds (a SENSITIVE document doesn't need a Debug impl for the inner type, for example)

However, the tradeoff here is that SENSITIVE is now detached from the underlying data type, which makes it error prone (with potentially dangerous failure modes). A PrivateKey is always SENSITIVE. The user of these types should not have the option of changing that.

npmccallum · 2022-02-12T15:10:12Z

One other feature to consider is how to read/write files that contain multiple Document objects. Certificate chain files are probably the most obvious case of this. And we probably want a PkiPath type in x509 that should be able to be serialized/deserialized to/from PEM.

tarcieri · 2022-02-12T15:37:25Z

However, the tradeoff here is that SENSITIVE is now detached from the underlying data type, which makes it error prone

That's potentially true, however if we define type aliases for the relevant documents, someone would need to go out of their way in order to turn it off.

I think there's pros and cons either way.

One other feature to consider is how to read/write files that contain multiple Document objects.

See #11

tarcieri · 2022-02-12T15:59:43Z

I'm not sure this will actually work out in practice. Several of the methods are using HRTBs, but the problem comes when you actually need to notate that lifetime for the borrowed T:

error[E0277]: the trait bound `for<'a> SubjectPublicKeyInfo<'_>: Decodable<'a>` is not satisfied
  --> spki/src/document.rs:21:12
   |
21 |         Ok(Self::from_der(bytes)?)
   |            ^^^^^^^^^^^^^^ the trait `for<'a> Decodable<'a>` is not implemented for `SubjectPublicKeyInfo<'_>`

So yes, alternatively this could just be a bag-of-bytes newtype which perhaps might (or might not) perform some basic sanity checks on the inner DER message, with no generic parameter.

That unfortunately loses any type safety around the message it contains, though, and will introduce more fallibility where there currently is none.

tarcieri · 2022-04-02T17:05:26Z

Closing in favor of #571

tarcieri commented Feb 12, 2022

View reviewed changes

tarcieri mentioned this pull request Feb 12, 2022

der: *Document type improvements #421

Closed

npmccallum reviewed Feb 12, 2022

View reviewed changes

Use a const generic SENSITIVE parameter

3cb486d

tarcieri force-pushed the der/generic-document-type branch from a34d000 to 3cb486d Compare February 12, 2022 14:34

tarcieri mentioned this pull request Mar 21, 2022

add certificate document support #518

Merged

tarcieri closed this Apr 2, 2022

tarcieri deleted the der/generic-document-type branch April 2, 2022 17:05

[WIP] der: generic Document type #427

[WIP] der: generic Document type #427

Uh oh!

Conversation

tarcieri commented Feb 12, 2022

Uh oh!

tarcieri commented Feb 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

npmccallum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Feb 12, 2022

Uh oh!

npmccallum commented Feb 12, 2022

Uh oh!

npmccallum commented Feb 12, 2022

Uh oh!

tarcieri commented Feb 12, 2022

Uh oh!

tarcieri commented Feb 12, 2022

Uh oh!

tarcieri commented Apr 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[WIP] der: generic `Document` type #427

[WIP] der: generic `Document` type #427