-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
core/src/identity/rsa.rs
Outdated
// 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( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
core/src/identity/rsa.rs
Outdated
// TODO: Is there a way to write to `sink` directly? | ||
let mut algorithm = Vec::new(); | ||
self.algorithm.encode(&mut algorithm)?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
core/src/identity/rsa.rs
Outdated
// 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)?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2000 (comment) :)
There was a problem hiding this 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 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
@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 |
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. |
There was a problem hiding this comment.
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.
core/src/identity/rsa.rs
Outdated
@@ -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. |
There was a problem hiding this comment.
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.
Thank you very much 😊
Yes, the current version looks pretty good IMO 👍🏻
That's correct. In theory it would be possible to add sth. like an 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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
TODO
s highlighted below.