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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kamulos
Copy link

@kamulos kamulos commented May 23, 2024

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

der/src/reader.rs Outdated Show resolved Hide resolved
@dishmaker
Copy link

dishmaker commented May 28, 2024

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
Copy link

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
Copy link
Member

baloo commented Jun 11, 2024

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

@kamulos
Copy link
Author

kamulos commented Jun 25, 2024

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

@tarcieri
Copy link
Member

@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
Copy link
Author

kamulos commented Sep 13, 2024

@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
Copy link
Member

@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 kamulos force-pushed the private_long_form_tags branch 2 times, most recently from a1ef847 to 39e8b97 Compare October 2, 2024 14:32
@kamulos kamulos force-pushed the private_long_form_tags branch from 39e8b97 to 1f43a08 Compare October 2, 2024 14:33
@kamulos
Copy link
Author

kamulos commented Oct 2, 2024

@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.

@kamulos kamulos marked this pull request as ready for review October 2, 2024 14:50
@dishmaker
Copy link

dishmaker commented Oct 4, 2024

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
Copy link

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

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.

der: Encoding higher tag number
4 participants