Skip to content

Conversation

@npmccallum
Copy link
Contributor

This crate consolidates the attribute and name types from the x509 and
pkcs10 crates. The pkcs10 crate now depends on x501 rather than x509.
It should now be simpler to release a version of x501 and pkcs10 without
requiring a full release of x509.

Note that unlike spki, x501 requires both derive and alloc.

Signed-off-by: Nathaniel McCallum nathaniel@profian.com

@tarcieri
Copy link
Member

tarcieri commented Feb 1, 2022

Note that unlike spki, x501 requires both derive and alloc.

Yeah that's cool, it's how we're treating x509 as well.

@carl-wallace
Copy link
Contributor

I'll mod x509 to reference that soonish and drop the corresponding types from there. OCSP can be born referencing this.

@npmccallum
Copy link
Contributor Author

@carl-wallace x509 is already included in this commit.

@tarcieri
Copy link
Member

tarcieri commented Feb 1, 2022

Another fun application would be adding an attributes = ["x501"] feature to pkcs8, which presently ignores attributes.

If nothing else, it would make it easier to round trip RFC test vectors that include attributes.

@npmccallum
Copy link
Contributor Author

I forgot to fix up the GitHub Actions workflows. Adding those now.

@carl-wallace
Copy link
Contributor

carl-wallace commented Feb 1, 2022

OK. I'll update x509, mod certval/pittv3 and OCSP will reference this from the outset.

@npmccallum
Copy link
Contributor Author

Ok. All the actions are updated and all the tests pass. I think this is ready for review.

x501/src/name.rs Outdated
/// ```
///
/// [RFC 5280 Section 4.1.2.4]: https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.4
pub type RDNSequence<'a> = alloc::vec::Vec<RelativeDistinguishedName<'a>>;
Copy link
Member

Choose a reason for hiding this comment

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

Idiomatic Rust naming has generally moved to "capitalized acronyms", which is to say the first letter of an acronym is capitalized. See https://rust-lang.github.io/api-guidelines/naming.html

In UpperCamelCase, acronyms and contractions of compound words count as one word: use Uuid rather than UUID, Usize rather than USize or Stdin rather than StdIn.

Suggested change
pub type RDNSequence<'a> = alloc::vec::Vec<RelativeDistinguishedName<'a>>;
pub type RdnSequence<'a> = alloc::vec::Vec<RelativeDistinguishedName<'a>>;

There are clippy lints for this on structs, but apparently not on type aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was all caps in the previous version. So I just copied that. Fixed now.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

One nit re: RDNSequence naming but otherwise looks good.

Edit: two nits!

Comment on lines 1 to 18
[package]
name = "x501"
version = "0.1.0" # Also update html_root_url in lib.rs when bumping this
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dev-dependencies]
hex-literal = "0.3"

[dependencies]
der = { version = "=0.6.0-pre.0", features = ["derive", "alloc", "oid"], path = "../der" }
Copy link
Member

Choose a reason for hiding this comment

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

You might include a slightly more complete Cargo.toml looking off one of the other crates as a reference.

We generally include license information, links to the repository, keywords, categories, specify the README.md name, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! I meant to do that before pushing but obviously forgot. Fixed now.

This crate consolidates the attribute and name types from the `x509` and
`pkcs10` crates. The `pkcs10` crate now depends on `x501` rather than `x509`.
It should now be simpler to release a version of `x501` and `pkcs10` without
requiring a full release of `x509`.

Note that unlike `spki`, x501 requires both `derive` and `alloc`.

Signed-off-by: Nathaniel McCallum <nathaniel@profian.com>
@tarcieri tarcieri merged commit 1d51451 into RustCrypto:master Feb 2, 2022
@npmccallum npmccallum deleted the x501 branch March 10, 2022 20:46
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.

3 participants