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

Add PKCS#10 attributes to CSR serializer #296

Merged
merged 9 commits into from
Nov 13, 2024
Merged

Conversation

lvkv
Copy link
Contributor

@lvkv lvkv commented Oct 30, 2024

Coming from #285, this PR adds PKCS#10 attributes to rcgen's CSR serialization logic.
Of course, rcgen has already implicitly supported at most one attribute in the form of PKCS#9's extensionRequest1, but this allows you to add others.

I haven't split up the commits yet in anticipation of review comments, but the rough changes are (as of writing):

  • Add an Attribute type that represents an RFC 52802/RFC 29863 (take your pick) ATTRIBUTE.
  • Add a custom_csr_attributes field to CertificateParams, of type Attribute
  • Add serialization logic for custom_csr_attributes in CertificateParams::serialize_request
  • Add an end-to-end/roundtrip test for CSR serialization with custom attributes
  • Add to and update existing CSR serialization tests

Along the way, I've updated a few items that may or may not count as bugs. These are all subtle, so feel free to be pedantic when reviewing them:

  • Don't write out DER for SANs if no SANs are specified. The current output isn't technically malformed ("hey, here are 0 SANs"), but it might be unexpected. Edit: yeah, the current behavior violates RFC 5280, see Add PKCS#10 attributes to CSR serializer #296 (comment)
  • Do still write out DER for certificate extensions in the edge case that someone wants an EKU but hasn't specified any standard key usages. The current behavior forgets to put that in the CSR. I believe it's RFC-compliant to do this (i.e. neither PKCS#9, PKCS#10 nor RFC 5280 say you can't), so the current behavior feels like a bug to me.
  • Write CSR attributes using an implicit ASN.1 tag instead of an explicit one (see Can't write explicitly tagged sets qnighy/yasna.rs#8). This also correctly forces us to encode attributes as an implicit SET OF. I believe the current implementation encodes a single attribute correctly, but it's almost by accident, and definitely doesn't work if you want a number of attributes that isn't 0 or 1.

Footnotes

  1. https://datatracker.ietf.org/doc/html/rfc2985

  2. https://datatracker.ietf.org/doc/html/rfc5280

  3. https://datatracker.ietf.org/doc/html/rfc2986

@cpu
Copy link
Member

cpu commented Nov 1, 2024

Hope to give this a first pass soon. Thanks!

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.

I think this is going in the right direction. Thanks!

Would you mind getting it split into smaller commits and knocking out some of the first round of feedback? I think it'll be easier for others to review when the changes are isolated and describe their purpose in the commit message 👍 That will also give me some time to double check the one bit of ASN.1 I wasn't confident about.

rcgen/src/certificate.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
rcgen/tests/generic.rs Outdated Show resolved Hide resolved
@djc
Copy link
Member

djc commented Nov 4, 2024

Thanks @cpu for doing an initial round of review. I had some time off last week so didn't get to this, but would definitely want this restructured into basically one commit per bullet point in your PR description (or more, potentially), and reviewing would be a lot easier based on that.

@lvkv lvkv force-pushed the lvkv-cri-attrs branch 3 times, most recently from c3d644b to e3d8766 Compare November 9, 2024 21:12
@lvkv lvkv requested review from cpu and est31 November 9, 2024 21:13
rcgen/tests/generic.rs Show resolved Hide resolved
rcgen/tests/generic.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Outdated Show resolved Hide resolved
@cpu cpu self-assigned this Nov 11, 2024
@lvkv
Copy link
Contributor Author

lvkv commented Nov 12, 2024

@djc this is ready for another look!

@lvkv lvkv requested a review from djc November 12, 2024 03:07
rcgen/tests/generic.rs Outdated Show resolved Hide resolved
rcgen/src/certificate.rs Show resolved Hide resolved
@lvkv lvkv force-pushed the lvkv-cri-attrs branch 4 times, most recently from 9ea4db2 to da8342e Compare November 13, 2024 04:03
Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

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

could you remove the [1] syntax? One can also do [abcabc] and then later [abc]: https://example.com. The issue with [1] syntax is that it might be resolved in a global context instead of a local one, where we then have multiple URLs pointing to [1].

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.

Really nice work!

rcgen/tests/generic.rs Outdated Show resolved Hide resolved
@cpu cpu removed their assignment Nov 13, 2024
@lvkv
Copy link
Contributor Author

lvkv commented Nov 13, 2024

@est31 @cpu done!

@cpu cpu added this pull request to the merge queue Nov 13, 2024
Merged via the queue into rustls:main with commit 331b8fd Nov 13, 2024
15 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.

4 participants