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 #214

Conversation

Tudyx
Copy link
Contributor

@Tudyx Tudyx commented Jan 17, 2024

Fix #181

Mainly inspired by the der and the asn1_rs crates.

@djc
Copy link
Member

djc commented Jan 18, 2024

This looks okay so far. One thought is that I think maybe the TryFrom impls should be for String instead of &str, so as to make it obvious an owned type is required (giving callers the opportunity to move a value rather than effectively having to clone it).

@Tudyx
Copy link
Contributor Author

Tudyx commented Jan 18, 2024

Maybe we could provide both? I think the TryFrom<&str> is quite nice for users because it allows moving from (old API)

PrintableString("BR".into());

to

PrintableString("BR".try_into()?)

If we only provide TryFrom<String>, users would be obligated to do

PrintableString(String::from("BR").try_into()?)

@Tudyx
Copy link
Contributor Author

Tudyx commented Jan 18, 2024

giving callers the opportunity to move a value rather than effectively having to clone it)

Having both would also solve this one.

so as to make it obvious an owned type is required

I see your point but I think it's okay because users are used to the standard String type which has a similar API ("foo".into() or String::from("foo"))

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a quick review pass. So far it's looking good. Thanks for picking this up

rcgen/src/lib.rs Show resolved Hide resolved
rcgen/src/string_types.rs Outdated Show resolved Hide resolved
rcgen/src/string_types.rs Outdated Show resolved Hide resolved
rcgen/src/string_types.rs Outdated Show resolved Hide resolved
rcgen/src/string_types.rs Outdated Show resolved Hide resolved
@Tudyx
Copy link
Contributor Author

Tudyx commented Jan 20, 2024

I've updated the PR with the missing methods, and integrate it with subject alternative names.
Some notes:

  • All the types can be created with an UTF-8 encoded string ( &str/ String ). For instance, BmpString::try_from("❤πü2?")? . A check is performed to see if all input's characters are in the valid charset.
  • Instead of providing TryFrom<&[u8]> I prefer to be explicit and use from_<encoding> like String::from_utf8

The PR is still a draft because I would like to add more tests but I haven't found a good way yet.

rcgen/src/error.rs Outdated Show resolved Hide resolved
rcgen/src/string_types.rs Show resolved Hide resolved
rcgen/src/string_types.rs Outdated Show resolved Hide resolved
rcgen/src/string_types.rs Outdated Show resolved Hide resolved
rcgen/src/string_types.rs Outdated Show resolved Hide resolved
@Tudyx Tudyx marked this pull request as ready for review January 21, 2024 21:46
rcgen/src/error.rs Outdated Show resolved Hide resolved
rcgen/src/string_types.rs Outdated Show resolved Hide resolved
@Tudyx Tudyx force-pushed the tighten_up_string_type_representations_to_prevent_illegal_values branch from 8450995 to c352ad8 Compare January 23, 2024 11:32
@Tudyx
Copy link
Contributor Author

Tudyx commented Jan 23, 2024

I rebased on top of #213. I've also cleanup the git history a bit

rustls-cert-gen/src/main.rs Outdated Show resolved Hide resolved
@Tudyx Tudyx force-pushed the tighten_up_string_type_representations_to_prevent_illegal_values branch from c352ad8 to 8ff55b1 Compare January 23, 2024 12:34
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you

rcgen/src/error.rs Outdated Show resolved Hide resolved
@cpu
Copy link
Member

cpu commented Jan 23, 2024

@Tudyx If you rebase I think this is ready for merge. Thank you!

@Tudyx Tudyx force-pushed the tighten_up_string_type_representations_to_prevent_illegal_values branch from d53eb3a to b427983 Compare January 23, 2024 17:10
@Tudyx
Copy link
Contributor Author

Tudyx commented Jan 23, 2024

That's should be it

@cpu cpu added this pull request to the merge queue Jan 23, 2024
@cpu
Copy link
Member

cpu commented Jan 23, 2024

Thanks again. I appreciate you wrangling this tech debt to unblock the feature work you were originally interested in doing. You went the extra mile 🏆

Merged via the queue into rustls:main with commit 28ec9fa Jan 23, 2024
19 of 21 checks passed
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.

Tighten up string type representations to prevent illegal values
4 participants