Add Md5AuthString type - #96
Conversation
Adds wrapper type for MD5 passwords used with TCP (RFC 2385). Md5AuthString enforces constraints around length (1-80 bytes, inclusive) as well as content (printable ASCII), with proper Errors and memory zeroization. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
|
Before I take a look, can you explain the purpose of zeroize in here? |
Since the type is basically a wrapper on a password/secret, the idea was to add a little bit of extra security in the sense that the memory where the key was stored gets zeroed on drop. I don't feel super strongly about having it in the type, but zeroize is a small no_std crate and doesn't blow up the dependency graph by more than 1 dep, so I thought I'd see what folks think |
| } | ||
| } | ||
|
|
||
| impl ZeroizeOnDrop for Md5AuthString {} |
There was a problem hiding this comment.
Worth a comment here that the contract this marker implies is maintained by Md5AuthString containing a Zeroizing<String> as its sole member. Perhaps also worth a comment at the Md5AuthString definition that if the data structure is changed, we need to either make sure that ZeroizeOnDrop is maintained or remove the marker.
There was a problem hiding this comment.
Good call, although I did you one better. Zeroize has a zeroize_derive feature that allows the trait to be derived rather than implemented. So I've converted over to using that, since it will cause compilation failures if all members of the product type don't also implement ZeroizeOnDrop.
e.g.
error[E0599]: the method `zeroize_or_on_drop` exists for mutable reference `&mut Instant`,
but its trait bounds were not satisfied
16 | #[derive(Clone, Eq, PartialEq, ZeroizeOnDrop)]
| ^^^^^^^^^^^^^
= note: the following trait bounds were not satisfied:
`Instant: Zeroize`
= note: this error originates in the derive macro `ZeroizeOnDrop`
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
I think we should leave out zeroize. I don't see how it would be useful since the contained data does need to flow somewhere, and I don't think we're overly careful about removing every copy from memory. I would love to hear if you disagree.
| /// The [`Debug`](std::fmt::Debug) implementation redacts the key, and its | ||
| /// allocation is zeroized when the value is dropped. Converting it into a | ||
| /// [`String`] transfers responsibility for zeroizing that allocation to the | ||
| /// caller. Its serialized representation contains the key as a plain string. |
There was a problem hiding this comment.
| /// The [`Debug`](std::fmt::Debug) implementation redacts the key, and its | |
| /// allocation is zeroized when the value is dropped. Converting it into a | |
| /// [`String`] transfers responsibility for zeroizing that allocation to the | |
| /// caller. Its serialized representation contains the key as a plain string. | |
| /// The [`Debug`](std::fmt::Debug) implementation redacts the key, and its | |
| /// allocation is zeroed when the value is dropped. Converting it into a | |
| /// [`String`] should therefore be done with caution since no zeroing may occur. |
This raises a question for me. Where are we using this type where we are ensuring that places where the string occurs are zeroed? For example, if this is ever formatted into a query string or POST body by progenitor we're almost certainly not zeroing it. Is this even useful at all?
There was a problem hiding this comment.
Currently this type isn't used anywhere, but the intention is to integrate it into the mgd API as an optional parameter for BGP peers as a replacement for the current raw String. The flow of this data currently is that it comes in via dropshot, gets stored in a config struct owned by a per BGP peer, and that data is copied out when we make a call into libnet to interact with a PF_KEY socket. I don't believe the libnet type uses zeroize, but that could always be updated to match.
I'm not as familiar with the dropshot side of things or what would be needed to provide similar zeroing.
As I said before, I'm not so strongly opinionated here as to say zeroing is a must. I would be okay removing it if the consensus is that the utility is limited / non-existent.
| pub const MAX_LEN: usize = 80; | ||
|
|
||
| /// Creates an MD5 authentication string after validating its contents. | ||
| pub fn new(source: String) -> Result<Self, Md5AuthStringError> { |
There was a problem hiding this comment.
Not particularly. I'd lean towards having both over just one though.
| /// Returns the underlying string, transferring responsibility for | ||
| /// zeroizing it to the caller. |
There was a problem hiding this comment.
| /// Returns the underlying string, transferring responsibility for | |
| /// zeroizing it to the caller. | |
| /// Returns the underlying string, transferring responsibility for | |
| /// zeroing it to the caller. |
| string: Some(Box::new(schemars::schema::StringValidation { | ||
| max_length: Some(Self::MAX_LEN as u32), | ||
| min_length: Some(1), | ||
| pattern: Some(r"^[ -~]+$".to_string()), |
There was a problem hiding this comment.
definitely non-obvious and needs a comment
4 participants
Adds wrapper type for MD5 passwords used with TCP (RFC 2385). Md5AuthString enforces constraints around length (1-80 bytes, inclusive) as well as content (printable ASCII), with proper Errors and memory zeroization.