support private tags and tag numbers >30 that are stored in long form

[kamulos]

closes #1381

This PR implements two things:

• Allow the use of tag numbers >30. In the der representation they are stored in the long form as multiple bytes.
• Allow PRIVATE tags be annotated like context specific ones in the derive macros

Current State

Currently this is a draft to get feedback on the chosen approach. This code is in use in my project and works correctly for it's use-case: a lot of private tags in a deep hierarchy of choices and sequences.

For the most part this code should be in a state to be reviewed. There are a few parts that are still lacking:

• The code duplication of the private module, that is basically just a copy of the context_sensitive module
• The tag parsing and serializing code is too complicated
• The tests have not been adjusted / written

Overview

TagNumber is now represented by an u16. This should be enough for any use-case imaginable, but also small enough to be embedded-friendly. It's content is now pub instead of pub(super). This was necessary because der_derive wants to match against constant TagNumbers. In general having this public should not be an issue, because all 2^16 tag numbers are now valid and it is not possible to create an invalid state by having access to the u16.

Also the Private and PrivateRef types exist now. In the derive macros private can be annotated in the same way as context_specific. Internally this is represented as class: Option<Class> instead of context_sensitive: Option<TagNumber>.

Breaking changes that I found no way to avoid are:

• TagNumber::new() and TagNumber::value() use u16 now
• Conversions from and to u8 for Tag and TagNumber do not make sense anymore, this also applies to Tag::octet()
• The Reader trait now needs a function to peek multiple bytes in advance to be able to peek a tag (Reader::peek_offset())
• Length::for_tlv() just assumed, that a tag is always one byte, it now somehow needs to know about the specific tag we are looking at

[dishmaker]

[Spoiler] Unrelated to >30 tag breaking change

The code duplication of the private module, that is basically just a copy of the context_sensitive module

I've made it generic for my use case.

/// Application class
type Application<T, const TAG: u16> = CustomClass<T, TAG, 0b01>;

/// Context-specific class
type ContextSpecific<T, const TAG: u16> = CustomClass<T, TAG, 0b10>;

/// Private class
type Private<T, const TAG: u16> = CustomClass<T, TAG, 0b11>;


/// Application, Context-specific or Private class field which wraps an owned inner value.
///
/// This type decodes/encodes a field which is specific to a particular context
/// and is identified by a [`TagNumber`].
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
pub struct CustomClass<T, const TAG: u16, const CLASS: u8> {
    /// Tag mode: `EXPLICIT` VS `IMPLICIT`.
    pub tag_mode: TagMode,

    /// Value of the field.
    pub value: T,
}

Shouldn't tag_mode be generic too?

[dishmaker]

This crate is still unusable for the European Tachograph regulation.

There are many 2-byte tags.

https://eur-lex.europa.eu/legal-content/EN/TXT/?uri=celex%3A02016R0799-20200226

Field	Field ID	Tag	Length (bytes)	ASN.1 data type
ECC Certificate	C	7F 21	var	custom SEQUENCE
ECC Certificate Body	B	7F 4E	var	custom SEQUENCE
Certificate Profile Identifier	CPI	5F 29	1	INTEGER(0..255)
Certificate Authority Reference	CAR	42	8	KeyIdentifier
Certificate Holder Authorisation	CHA	5F 4C	7	CertificateHolder Authorisation
Public Key	PK	7F 49	var	custom SEQUENCE
Domain Parameters	DP	06	var	OBJECT IDENTIFIER
Public Point	PP	86	var	OCTET STRING
Certificate Holder Reference	CHR	5F 20	8	KeyIdentifier
Certificate Effective Date	CEfD	5F 25	4	TimeReal
Certificate Expiration Date	CExD	5F 24	4	TimeReal
ECC Certificate Signature	S	5F 37	var	OCTET STRING

[baloo]

@dishmaker we're still open for breaking changes in this release cycle. There is a legitimate use-case, PRs are welcome!

[kamulos]

Sorry for the slow reaction. In a week I am back and try to get in some more progress here.

[tarcieri]

@kamulos I've finished making the other changes I consider blockers for this one, though it's created quite a few conflicts.

If you can get this PR green though, I can make it a priority for the next to merge. I don't expect there will be many other major changes to der itself.

[kamulos]

@tarcieri Thanks for your work ❤️ And sorry for being very slow, the workload at the job was high and I also had vacations.

Currently we are actually releasing this code into production and things will calm down from here. So I will get back to this next week and hopefully get some version ready that is good quality.

[tarcieri]

@kamulos no worries, we'd like to do a release "soon", but that's probably in a month or two so you have some time

[kamulos]

@tarcieri I rebased and polished the implementation. The unit tests for the Private type are still broken, because I did not adjust them and I should definitely write some more unit Tests for the encoding and decoding of the Tag type.

But other than that I think it is ready for review. 👍

There are some noteworthy points:

1. The decode_with function in Private and ContextSpecific requires peeking for a Tag but maybe getting None as return. Therefore I added another function Tag::peek_optional that is only crate internal. This feels a little bit wring, but I did not want to modify the signature of public functions or traits.
2. Private is basically a copy of ContextSpecific with one difference: In decode_with I removed the skipping of lower tag numbers. This did not work for my use-case and I did not understand it.

[dishmaker]

[Spoiler] Unrelated to >30 tags

Private is basically a copy of ContextSpecific

Well, I also need Application 🥺

I think the best solution is using generics:

pub type ApplicationExplicit<const TAG: u16, T> = CustomClassExplicit<T, TAG, CLASS_APPLICATION>;
pub type ContextSpecificExplicit<const TAG: u16, T> = CustomClassExplicit<T, TAG, CLASS_CONTEXT_SPECIFIC>;
pub type PrivateExplicit<const TAG: u16, T> = CustomClassExplicit<T, TAG, CLASS_PRIVATE>;

pub type ApplicationImplicit<const TAG: u16, T> = CustomClassImplicit<T, TAG, CLASS_APPLICATION>;
pub type ContextSpecificImplicit<const TAG: u16, T> = CustomClassImplicit<T, TAG, CLASS_CONTEXT_SPECIFIC>;
pub type PrivateImplicit<const TAG: u16, T> = CustomClassImplicit<T, TAG, CLASS_PRIVATE>;

/// EXPLICIT
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
pub struct CustomClassExplicit<T: for<'a> Decode<'a>, const TAG: u16, const CLASS: u8> {
    pub value: T,
}

/// IMPLICIT
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
pub struct CustomClassImplicit<T: for<'a> DecodeValue<'a>, const TAG: u16, const CLASS: u8> {
    pub value: T,
}

pub const CLASS_APPLICATION: u8 = 0b01000000;
pub const CLASS_CONTEXT_SPECIFIC: u8 = 0b10000000;
pub const CLASS_PRIVATE: u8 = 0b11000000;

[dishmaker]

I've fixed all dependencies and added these 3 custom tags as generic ones :)

[tarcieri]

@kamulos if you are still interested in this, I would prefer a much more minimal PR which only changes the handling of the Tag type, and doesn't try to add higher-level APIs beyond that, which are really what's blocking getting this merged

[kamulos]

@tarcieri thanks for the message. I looked into it, rebased my commit and removed the stuff in a new commit.

With my last commit I added a three unit test cases, where the decoder is pretty forgiving at the moment. The second test case ("universal tag in long form") might be ok, but the other two accept invalid der encoding. Should they be rejected? I saw in the API that there is also a .from_ber() function now, and these encodings might be acceptable in the ber encoding. (not sure about that)

[tarcieri]

@kamulos thanks, though now we have three PRs open for this. #1651 is looking pretty good and has working tests

[kamulos]

@tarcieri Yeah, that PR is good. u32 instead of u16 seems reasonable to me.

But obviously, the issues I codified in my test cases here affect that one too, because the code is a copy. So you might still want to have a look at the cases.

The adjusted Test case in the other PR, where they put in ReasonFlags::from_der(&hex!("FF03079F80"));` is what made me realize that there might still be questions about the exact behavior open.

[tarcieri]

@kamulos can you perhaps open a PR to #1651 to add your test cases?

[turbocool3r]

@kamulos can you perhaps open a PR to #1651 to add your test cases?

I'll be unavailable for about 4 days, but I'll be happy to accept a PR or include the necessary changes myself. Also I should add @kamulos to PR authors somehow, is there any convention for this? Are you ok with this @kamulos?