Add extension request support #78

Geobert · 2021-03-02T15:07:42Z

Address #77 , on behalf of Isode Ltd.

Some precision on this PR:

attributes field type

the attributes field ended up being Implicit<Option<ApplicationTag0<Attribute>>> instead of
Implicit<Option<ApplicationTag0<Asn1SetOf<Attribute>>>> because after testing the output of openssl, it seems that the Asn1SetOf is not present.

using the attached files, you can use
openssl req -config req.txt -new -key key.pem -out req.pem

to generate the CSR and see that there's no SET after [0]

I was curious on what's the output of openssl if we add a challengePassword to the request: add attributes = req_attributes to the [ req ] section and rerun the openssl command, you'll see:

so after the [0], no SET and two consecutive SEQUENCE.

We don't need challengePassword yet, but if we add support later, I think ApplicationTag0 will need some way to have a SET without serializing it.

Unit test

I've added a Unit test for extensions using the config attached and openssl to create the PEM test data.
As attributes has change default value, the deserialize_csr test is broken, I think it needs to be updated but I prefer to ask before doing anything on this.
EDIT: maybe it's not the test, but the encoding, I've lost the [0] so ignore this point

Copyright

The legal team of my company is happy to license the contribution under the dual-license MIT-Apache-2.0 but would like to have Isode Ltd. mentioned somewhere. Some crates have a Contributor.txt file listing contributors. Would you (and Devolutions) agree to add this file to the repository? It would look like this (header copied from oxide-auth crate's repository):

# A list of people who have contributed to this project, with optional metadata
# The list is sorted chronologically, excluding the current maintainer who should always be listed first
# Format: Foo Bar <optional mail address> (comment e.g. company affiliation)

Benoît Cortier (current maintainer)
Jonathan Trépanier (on behalf of Devolutions)
Marc-André Moreau (on behalf of Devolutions)
Jean-Francois Theroux
François Dubois
Sébastien Duquette (on behalf of Devolutions)
Johann Dufaud
Richard Markiewicz
Ionuț Mihalcea
Kim Altintop
Joe Ellis
Hugues de Valon
Geobert Quach (on behalf of Isode Ltd.)

I took the time to sort the different contributors chronologically :)

Conclusion and Recap

As the message is quite large, here is a summary on what's left:

agree on the type of attributes
fix the unit test deserialize_csr because of change of attributes type
agree on adding a contributors list file

req.zip

Geobert · 2021-03-02T17:27:57Z

On the test failing, I tried to change the type from Implicit<Option<ApplicationTag0<Attribute>>> to Implicit<ApplicationTag0<Option<Attribute>>> to try to get back that [0] but it seems that I need the HeaderOnly<ApplicationTag0<()>> construct for the serialization to work?

CBenoit · 2021-03-02T19:58:29Z

Hi @Geobert , thank you! Great first PR 🎉

the attributes field ended up being Implicit<Option<ApplicationTag0>> instead of Implicit<Option<ApplicationTag0<Asn1SetOf>>> because after testing the output of openssl, it seems that the Asn1SetOf is not present.
...
I was curious on what's the output of openssl if we add a challengePassword to the request: add attributes = req_attributes to the [ req ] section and rerun the openssl command, you'll see:
…
so after the [0], no SET and two consecutive SEQUENCE.

[EDIT: my explanation was wrong so I removed it to avoid misleading other people]

I asked my team leader @awakecoding about the Contributor.txt. We usually simply use the package.authors field in Cargo.toml for this purpose. I believe that's the standard way to proceed in the Rust ecosystem, and names are displayed on crates.io:

Given that you contributed to picky-asn1-x509 crate you would add your name in this Cargo.toml.
Would this be alright for you?
If it's okay for you, you could also add

"Sergey Noskov <snoskov@avito.ru>"
"Kim Altintop <kim@monadic.xyz>"
"Joe Ellis <joe.ellis@arm.com>"
"Hugues de Valon <hugues.devalon@arm.com>"
"Geobert Quach <geobert.quach@isode.com>"

to picky-asn-x509 and

"Ionut Mihalcea <ionut.mihalcea@arm.com>"
"Kim Altintop <kim@monadic.xyz>"

to picky. 🙂

EDIT: I'll have a deeper look at the code once you fixed the SET part 🙂

awakecoding · 2021-03-02T20:16:40Z

@Geobert just to clarify about the copyright - Rust projects normally use the Cargo.toml file to declare authors, so if we add a Contributor.txt file, we'd have to maintain the list of authors in two places. Is there a reason why declaring authors in the Cargo.toml file would not be suitable?

Geobert · 2021-03-03T13:52:43Z

Copyright

@Geobert just to clarify about the copyright - Rust projects normally use the Cargo.toml file to declare authors, so if we add a Contributor.txt file, we'd have to maintain the list of authors in two places. Is there a reason why declaring authors in the Cargo.toml file would not be suitable?

Thank you for your answer! I think having the copyright in Cargo.toml is perfect :) I totally forgot this field ^^'

ApplicationTag

I tried pub attributes: ApplicationTag0<Asn1SetOf<Attribute>>,

which leads to:

pub fn new(subject: Name, subject_public_key_info: SubjectPublicKeyInfo) -> Self {
        // It shall be 0 for this version of the standard.
        Self {
            version: 0,
            subject,
            subject_public_key_info,
            attributes: ApplicationTag0(Asn1SetOf(Vec::new())),
        }
    }

but this gives:

The SET tag is not replaced with the ApplicationTag0 tag :-/

Geobert · 2021-03-03T15:00:27Z

From what Benoît said, my understanding of ApplicationTag is that its tag replace the tag of the object it contains, so ApplicationTag0<Asn1SetOf<Attribute>> should serialize by replacing the 0x31 tag from the SET by 0xA0 from the ApplicationTag but we can see that it is not happening.

I'm trying to understand how ApplicationTag0 is serialized and I can only see in ser::Serializer::serialize_newtype_struct that in the case of Tag0, we "encapsulate" which I'm not sure to understand :-/

Is the ApplicationTag* ser broken? This seems unlikely as you use the crate with no issue and have plenty of tests to back that up. So I'm confused :-/

CBenoit · 2021-03-03T15:41:41Z

From what Benoît said, my understanding of ApplicationTag is that its tag replace the tag of the object it contains, so ApplicationTag0<Asn1SetOf<Attribute>> should serialize by replacing the 0x31 tag from the SET by 0xA0 from the ApplicationTag but we can see that it is not happening.

I'm trying to understand how ApplicationTag0 is serialized and I can only see in ser::Serializer::serialize_newtype_struct that in the case of Tag0, we "encapsulate" which I'm not sure to understand :-/

Is the ApplicationTag* ser broken? This seems unlikely as you use the crate with no issue and have plenty of tests to back that up. So I'm confused :-/

Oh, let me check, maybe something is wrong somewhere.

Geobert · 2021-03-03T16:23:01Z

Oh, let me check, maybe something is wrong somewhere.

I don't know if it can help, but I noticed that is_context_specific is called in h_write_header but I can't find any call to is_application anywhere

EDIT: by modifying in ser::Serializer::h_write_header and adding the testing of is_application:

Some(last_encapsulator_tag)
                if last_encapsulator_tag.is_context_specific() || last_encapsulator_tag.is_application() =>
            {
                written = self.h_write_encapsulator(len)?;
            }

the serialization seems correct, but for a weird reason, it breaks the deserialization:

thread 'certification_request::tests::deserialize_csr' panicked at 'failed certification_request_info deserialization: TruncatedData',

CBenoit · 2021-03-03T17:20:58Z

Oh, let me check, maybe something is wrong somewhere.

I don't know if it can help, but I noticed that is_context_specific is called in h_write_header but I can't find any call to is_application anywhere

I think I misunderstood something 🤦
Tag is replaced only if field is marked as IMPLICIT. If EXPLICIT, application tag is basically wrapping similarly to a SEQUENCE.
Apparently I implemented application tags as always EXPLICIT and context specific tags as always IMPLICIT.
Sadly, we need an "IMPLICIT" 0xA0. I think I'll need to perform a refactor at some point to fix that issue. For now, let's use something like Implicit<Option<ApplicationTag0<Attribute>>>. If it's okay with you I'll push modifications to make it work until I refactor properly.

CBenoit · 2021-03-03T17:23:44Z

EDIT: by modifying in ser::Serializer::h_write_header and adding the testing of is_application:
Some(last_encapsulator_tag)
                if last_encapsulator_tag.is_context_specific() || last_encapsulator_tag.is_application() =>
            {
                written = self.h_write_encapsulator(len)?;
            }
the serialization seems correct, but for a weird reason, it breaks the deserialization:
thread 'certification_request::tests::deserialize_csr' panicked at 'failed certification_request_info deserialization: TruncatedData',

Yes, deserializer would need to be modified the same way. However, as said above, issue is a bit deeper here 😕

Geobert · 2021-03-03T17:36:12Z

However, as said above, issue is a bit deeper here

In that case, the PR is complete with Implicit<Option<ApplicationTag0<Attribute>>> instead of Implicit<ApplicationTag0<Asn1SetOf<Attribute>>> which needs refactoring to work :)

The unit test for extensions works, but the empty ApplicationTag0 test is broken of course

CBenoit · 2021-03-03T17:49:57Z

However, as said above, issue is a bit deeper here

In that case, the PR is complete with Implicit<Option<ApplicationTag0<Attribute>>> instead of Implicit<ApplicationTag0<Asn1SetOf<Attribute>>> which needs refactoring to work :)

The unit test for extensions works, but the empty ApplicationTag0 test is broken of course

Good! I'll fix that, merge and publish 🙂

CBenoit

I implemented a workaround so this behaves like a proper IMPLICIT [0] on a sequence.

Geobert and others added 3 commits March 3, 2021 15:36

picky-asn1-x509: initial csr attributes support

43bbd90

Update authors in Cargo metadata

8ed8212

picky-asn1-x509: workaround for CSR attributes

f865ff2

CBenoit force-pushed the cri-attributes branch from 87a470f to f865ff2 Compare March 3, 2021 20:37

CBenoit previously approved these changes Mar 3, 2021

View reviewed changes

picky-asn1-x509: clarify comment about workaround

19011f4

CBenoit dismissed their stale review via 19011f4 March 3, 2021 20:42

CBenoit approved these changes Mar 3, 2021

View reviewed changes

CBenoit merged commit d232339 into Devolutions:master Mar 3, 2021

CBenoit mentioned this pull request Mar 3, 2021

picky: CSR attributes support #79

Merged

CBenoit mentioned this pull request Aug 13, 2021

Powershell scripts signing and verifying with extended validation CA #95

Merged

CBenoit mentioned this pull request Jan 15, 2024

DER Serialization Problem #249

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extension request support #78

Add extension request support #78

Geobert commented Mar 2, 2021 •

edited by CBenoit

Loading

Geobert commented Mar 2, 2021

CBenoit commented Mar 2, 2021 •

edited

Loading

awakecoding commented Mar 2, 2021

Geobert commented Mar 3, 2021

Geobert commented Mar 3, 2021

CBenoit commented Mar 3, 2021

Geobert commented Mar 3, 2021 •

edited

Loading

CBenoit commented Mar 3, 2021 •

edited

Loading

CBenoit commented Mar 3, 2021

Geobert commented Mar 3, 2021 •

edited

Loading

CBenoit commented Mar 3, 2021

CBenoit left a comment

Add extension request support #78

Add extension request support #78

Conversation

Geobert commented Mar 2, 2021 • edited by CBenoit Loading

attributes field type

Unit test

Copyright

Conclusion and Recap

Geobert commented Mar 2, 2021

CBenoit commented Mar 2, 2021 • edited Loading

awakecoding commented Mar 2, 2021

Geobert commented Mar 3, 2021

Copyright

ApplicationTag

Geobert commented Mar 3, 2021

CBenoit commented Mar 3, 2021

Geobert commented Mar 3, 2021 • edited Loading

CBenoit commented Mar 3, 2021 • edited Loading

CBenoit commented Mar 3, 2021

Geobert commented Mar 3, 2021 • edited Loading

CBenoit commented Mar 3, 2021

CBenoit left a comment

Choose a reason for hiding this comment

Geobert commented Mar 2, 2021 •

edited by CBenoit

Loading

CBenoit commented Mar 2, 2021 •

edited

Loading

Geobert commented Mar 3, 2021 •

edited

Loading

CBenoit commented Mar 3, 2021 •

edited

Loading

Geobert commented Mar 3, 2021 •

edited

Loading