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

core: Update to asn1_der v0.7 #2000

Merged
merged 8 commits into from
Mar 22, 2021
Merged

core: Update to asn1_der v0.7 #2000

merged 8 commits into from
Mar 22, 2021

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Mar 15, 2021

Replaces #1751.

As previously described in #1751 asn1_der v0.7 introduces some breaking changes to its API. See #1751 (comment) by @KizzyCode.

The changes proposed in this pull request uphold the status quo and could thus be merged as is. Still I am struggling a bit with the new API, see comments below, which makes me wonder whether I am correctly using it.

@KizzyCode In case you have some time I would very much appreciate a review, especially on the TODOs highlighted below.

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
Comment on lines 141 to 143
// TODO: `DerObject::write` is a hidden method. Should one use `new` or `new_from_source`?
// If so, I don't see a way to do that given that `S: Into<&'a [u8]` would be required.
asn1_der::DerObject::write(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the way to go?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the use of write is correct when serializing a new type; I use it for the same purpose and it should not be hidden 🙈

However when defining a new type you should put the implementation into a DerTypeView; e.g.:

/// A raw OID
#[derive(Copy, Clone)]
struct Asn1RawOid<'a> {
    object: DerObject<'a>
}
impl<'a> Asn1RawOid<'a> {
    /// Wraps an OID byte literal
    pub fn new<S: Sink + Into<&'a[u8]>>(oid: &[u8], sink: S) -> Result<Self, asn1_der::Asn1DerError> {
        let object = DerObject::new(Self::TAG, oid, sink)?;
        Ok(Self { object })
    }

    /// The underlying OID as byte literal
    pub fn oid(&self) -> &[u8] {
        self.object.value()
    }

    /// Writes an OID raw `value` as DER-object to `sink`
    pub fn write<S: Sink>(value: &[u8], sink: &mut S) -> Result<(), Asn1DerError> {
        DerObject::write(Self::TAG, value.len(), &mut value.iter(), sink)
    }
}
impl<'a> asn1_der::typed::DerTypeView<'a> for Asn1RawOid<'a> {
    const TAG: u8 = 6;

    fn object(&self) -> DerObject<'a> {
        self.object
    }
}
impl<'a> asn1_der::typed::DerEncodable for Asn1RawOid<'a> {
    fn encode<S: asn1_der::Sink>(&self, sink: &mut S) -> Result<(), asn1_der::Asn1DerError> {
        self.object.encode(sink)
    }
}
impl<'a> asn1_der::typed::DerDecodable<'a> for Asn1RawOid<'a> {
    fn load(object: DerObject<'a>) -> Result<Self, Asn1DerError> {
        // Validate the tag
        if object.tag() != Self::TAG {
            return Err(Asn1DerError::new(
                Asn1DerErrorVariant::InvalidData("DER object tag is not the object identifier tag."),
            ));
        }

        // Load self
        Ok(Self { object })
    }
}

(This is because the general approach of this crate is view-based; i.e. the core implementations do not own any bytes).

Now you can easily implement a native type upon it:

/// An 'rsaEncryption' OID
#[derive(Copy, Clone)]
struct Asn1OidRsaEncryption;
impl Asn1OidRsaEncryption {
    /// The DER encoding of the object identifier (OID) 'rsaEncryption' for
    /// RSA public keys defined for X.509 in [RFC-3279] and used in
    /// SubjectPublicKeyInfo structures defined in [RFC-5280].
    ///
    /// [RFC-3279]: https://tools.ietf.org/html/rfc3279#section-2.3.1
    /// [RFC-5280]: https://tools.ietf.org/html/rfc5280#section-4.1
    const OID: &'static[u8] = b"\x2A\x86\x48\x86\xF7\x0D\x01\x01\x01";
}
impl<'a> DerDecodable<'a> for Asn1OidRsaEncryption {
    fn load(object: DerObject<'a>) -> Result<Self, Asn1DerError> {
        match Asn1RawOid::load(object)?.oid() {
            Self::OID => Ok(Self),
            _ => Err(Asn1DerError::new(
                Asn1DerErrorVariant::InvalidData("DER object is not the 'rsaEncryption' identifier."),
            ))
        }
    }
}
impl DerEncodable for Asn1OidRsaEncryption {
    fn encode<S: Sink>(&self, sink: &mut S) -> Result<(), Asn1DerError> {
        Asn1RawOid::write(Self::OID, sink)
    }
}

If it makes things easier for you, I can also add a raw OID type to the core crate; I'm still a bit reluctant about something that understands OIDs, but an adapter to work with the raw bytes is no problem anymore IMO.

Comment on lines 179 to 181
// TODO: Is there a way to write to `sink` directly?
let mut algorithm = Vec::new();
self.algorithm.encode(&mut algorithm)?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the way to go?

Copy link

@KizzyCode KizzyCode Mar 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in general yes; however I've added an additional API to directly create a DerObject from an encodable object:

/// The ASN.1 AlgorithmIdentifier for "rsaEncryption".
struct Asn1RsaEncryption {
    algorithm: Asn1OidRsaEncryption,
    parameters: ()
}
impl<'a> DerDecodable<'a> for Asn1RsaEncryption {
    fn load(object: DerObject<'a>) -> Result<Self, Asn1DerError> {
        let sequence = Sequence::load(object)?;

        Ok(Self {
            algorithm: sequence.get_as(0)?,
            parameters: sequence.get_as(1)?
        })
    }
}
impl DerEncodable for Asn1RsaEncryption {
    fn encode<S: Sink>(&self, sink: &mut S) -> Result<(), Asn1DerError> {
        // I've published an update to allow this; take a look at 0.7.4
        // The buffers are still necessary because a DerObject is a simple view
        let mut algorithm_buf = Vec::new();
        let algorithm = self.algorithm.der_object(VecBacking(&mut algorithm_buf))?;

        let mut parameters_buf = Vec::new();
        let parameters = self.parameters.der_object(VecBacking(&mut parameters_buf))?;
        
        // Create a sequence
        Sequence::write(&[algorithm, parameters], sink)
    }
}

I agree that this is not very straight forward/elegant; the API was mainly designed with serde_asn1_der in mind. However if you're interested I can also revive asn1_der_derive, if serde is no option for you?

Comment on lines 182 to 184
// TODO: Is the decode after the encode step needed? Passing a `Vec<u8>` to
// `Sequence::write` would not encode the payload with the right tag.
let algorithm = DerObject::decode(&algorithm)?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the way to go?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. Generally looks ok to me, subject to addressing the remaining TODOs, of course. The encode -> decode -> write combination is indeed a bit strange and would be good to improve upon, if possible.

KizzyCode added a commit to KizzyCode/asn1_der-rust that referenced this pull request Mar 18, 2021
KizzyCode added a commit to KizzyCode/asn1_der-rust that referenced this pull request Mar 18, 2021
@mxinden
Copy link
Member Author

mxinden commented Mar 19, 2021

@KizzyCode thank you very much for the elaborate review as well as the many suggestions. Of course, while likely obvious, but still worth saying, thanks for all the work you put into asn1_der. I applied all your suggestions. In case you have another moment, let me know if I understood everything correctly.

The encode -> decode -> write combination is indeed a bit strange and would be good to improve upon, if possible.

@romanb Thanks for the review. Agreed, but if I understood correctly, there is no way around it today, as one needs to know the length of a Sequence before writing it to a Sink, @KizzyCode please correct me if I am wrong. @romanb could you give this pull request another review as well?

core/Cargo.toml Outdated
@@ -47,6 +47,8 @@ libp2p-tcp = { path = "../transports/tcp" }
multihash = { version = "0.13", default-features = false, features = ["arb"] }
quickcheck = "0.9.0"
wasm-timer = "0.2"
# TODO: For testing only. Remove.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove this before merging. Leaving it for now to ensure I don't introduce a regression.

@@ -221,8 +300,9 @@ mod tests {
const KEY2: &'static [u8] = include_bytes!("test/rsa-3072.pk8");
const KEY3: &'static [u8] = include_bytes!("test/rsa-4096.pk8");

// TODO: Remove libp2p_core_v026. For compatibility testing only.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove this before merging. Leaving it for now to ensure I don't introduce a regression.

@KizzyCode
Copy link

@KizzyCode thank you very much for the elaborate review as well as the many suggestions. Of course, while likely obvious, but still worth saying, thanks for all the work you put into asn1_der.

Thank you very much 😊

I applied all your suggestions. In case you have another moment, let me know if I understood everything correctly.

Yes, the current version looks pretty good IMO 👍🏻

The encode -> decode -> write combination is indeed a bit strange and would be good to improve upon, if possible.

@romanb Thanks for the review. Agreed, but if I understood correctly, there is no way around it today, as one needs to know the length of a Sequence before writing it to a Sink, @KizzyCode please correct me if I am wrong. @romanb could you give this pull request another review as well?

That's correct. In theory it would be possible to add sth. like an encoded_size API which could be used to predict the length during sequence creation, but this would make the API even more complex, so I decided to omit this.

I actually had this approach somewhere in an earlier version, and the performance difference was negligible (unless you work with very large data structures). For the 0.7 versions there should even be less impact because the decode operation is basically zero copy and really cheap now.
However if you experience a performance bottle neck due to this, please let me know!

Copy link
Contributor

@romanb romanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mxinden mxinden merged commit ba90827 into libp2p:master Mar 22, 2021
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.

None yet

3 participants