Skip to content

Enforce validity of PRINTABLE STRING in names - #12812

Merged
reaperhulk merged 1 commit into
pyca:mainfrom
alex:printable-string-name
May 2, 2025
Merged

reaperhulk merged 1 commit into
pyca:mainfrom
alex:printable-string-name

Conversation

@alex

@alex alex commented Apr 27, 2025

Copy link
Copy Markdown
Member

No description provided.

@alex alex linked an issue Apr 27, 2025 that may be closed by this pull request
@alex
alex requested a review from Copilot April 27, 2025 17:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to enforce the validity of PRINTABLE STRING values in X.509 name entries by updating the underlying attribute representation and introducing explicit handling for PrintableString.

  • Refactored attribute value handling in name encoding/decoding to use a new AttributeValue enum with a dedicated PrintableString variant.
  • Updated dependency references in Cargo.toml to use a git version of the asn1 crate.
  • Adjusted the error handling and tag extraction logic for string types in both x509 and cryptography-x509 modules.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/rust/src/x509/common.rs Updated encode/decode functions to support PrintableString and refined attribute encoding.
src/rust/cryptography-x509/src/common.rs Introduced AttributeValue enum and adjusted tag handling for improved type safety.
Cargo.toml Changed the asn1 dependency to a git source, reflecting the latest version updates.
Comments suppressed due to low confidence (3)

src/rust/src/x509/common.rs:80

  • [nitpick] Consider replacing the informal panic message with a more descriptive error message to aid debugging if this unexpected branch is ever reached.
            ).expect("I don't think this can ever fail?")

src/rust/src/x509/common.rs:91

  • [nitpick] Consider using a more informative error message instead of the informal text to improve clarity in case of failure.
            ).expect("I don't think this can ever fail?")

src/rust/src/x509/common.rs:239

  • [nitpick] Ensure that tests cover the PrintableString branch to verify that the value conforms to expected character set and encoding rules.
        AttributeValue::PrintableString(printable_string) => {

@alex
alex force-pushed the printable-string-name branch from 4449158 to c1a78f9 Compare April 27, 2025 20:23
@alex
alex requested a review from Copilot April 27, 2025 20:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enforces that names only contain valid PRINTABLE STRING characters by adding a new test case and updating the attribute handling in the Rust code.

  • Adds a new unit test to ensure that CRL issuers using invalid printable strings raise a ValueError.
  • Refactors the encoding and parsing of name attributes to use a richer AttributeValue enum and support new ASN.1 types.
  • Updates the asn1 dependency in Cargo.toml to use the latest git version.

Reviewed Changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated no comments.

File Description
tests/x509/test_x509.py Added a unit test for invalid printable strings in CRL names.
src/rust/src/x509/common.rs Refactored attribute encoding and parsing to support multiple string types.
src/rust/cryptography-x509/src/common.rs Updated type definitions with the new AttributeValue enum and improved tag resolution.
Cargo.toml Replaced the published asn1 dependency with the git version.
Files not reviewed (2)
  • CHANGELOG.rst: Language not supported
  • docs/development/test-vectors.rst: Language not supported
Comments suppressed due to low confidence (2)

Cargo.toml:23

  • Consider removing the commented out asn1 dependency line to improve clarity in the dependency section.
# asn1 = { version = "0.21.0", default-features = false }

src/rust/src/x509/common.rs:80

  • [nitpick] Replace the vague panic message with a more descriptive error message that explains the circumstances under which this failure might occur.
.expect("I don't think this can ever fail?")

@alex
alex force-pushed the printable-string-name branch 2 times, most recently from 18fb4a2 to de85363 Compare April 28, 2025 01:05
@alex
alex force-pushed the printable-string-name branch from de85363 to 1332799 Compare May 2, 2025 19:36
@alex
alex marked this pull request as ready for review May 2, 2025 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Cryptography parsed the '<>' symbols stored in PrintableString.

3 participants