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

Tighten up string type representations to prevent illegal values #181

Closed
cpu opened this issue Oct 27, 2023 · 7 comments · Fixed by #214
Closed

Tighten up string type representations to prevent illegal values #181

cpu opened this issue Oct 27, 2023 · 7 comments · Fixed by #214

Comments

@cpu
Copy link
Member

cpu commented Oct 27, 2023

In several places Rcgen defines representations of ASN.1 string types, but in a way that doesn't enforce the values meet the restrictions imposed by the type in use.

For example, consider the DnValue enum with TeletexString, PrintableString, UniversalString, Utf8String and BmpString variants. Those variants express their values as either String or Vec<u8>, but in many cases the ASN.1 definitions for these string types introduce further restrictions. As one example, a PrintableString can only contain A-Z, a-z, 0-9, '()+,-./:=? and <SPACE> where as a Rust String can contain any valid UTF-8.

Because of this mismatch invalid values can be expressed in CertificateParams, and won't be caught by any validation. In the cases where we use a yasna writer and write_bytes generically, we'll emit an invalid encoding for the type in use. In the cases where we use a more specific yasna helper like write_ia5_string or write_printable_string, invalid values will cause a panic.

I think we should look at creating a different representation that can impose more validation in order to reject invalid values at construction time as opposed to emitting invalid encodings or panicing when serializing.

github-merge-queue bot pushed a commit that referenced this issue Oct 30, 2023
This branch adds basic support emitting and parsing distinguished name
values that are Ia5Strings. For example, email address attributes in a
certificate subject distinguished name.

Note that because of #181 this code will panic when emitting invalid
Ia5String values. This problem is general to rcgen's handling of ASN.1
string types and so isn't addressed with additional care in this branch.
A broader rework is required.

Along the way I also fixed a warning from
#176 related to where we were
defining the custom `profile.dev.package.num-bigint-dig` profile
metadata.

Resolves #180
@cpu cpu mentioned this issue Dec 8, 2023
4 tasks
@Tudyx
Copy link
Contributor

Tudyx commented Jan 16, 2024

Does using newtype would make the job?

Something in the idea of the nutype crate but without using derive macro

@djc
Copy link
Member

djc commented Jan 16, 2024

Yeah, we could probably do newtypes for each of the inner variants (that need additional constraints), and have TryFrom impls for them?

@cpu
Copy link
Member Author

cpu commented Jan 16, 2024

That sounds like the right approach to me 👍

@Tudyx
Copy link
Contributor

Tudyx commented Jan 16, 2024

The created types will look like a lot to the one from the der::asn1 module of the der crate. See for instance the PrintableString struct.
Should we consider adding the der crate as a dependency or it's not worth it?

@est31
Copy link
Member

est31 commented Jan 16, 2024

I don't think we should depend on the der crate for that alone. It has a huge list of dependencies of its own.

@Tudyx
Copy link
Contributor

Tudyx commented Jan 17, 2024

Actually, it has no dependency for our use case. All the dependencies listed in their Cargo.toml are optional, and we only need the alloc feature that doesn't activate any. I have also checked with cargo tree to be sure.

@cpu
Copy link
Member Author

cpu commented Jan 17, 2024

These types will end up in the public API of rcgen and so I think it would be preferential to not tie them to a third party crate.

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 a pull request may close this issue.

4 participants