Skip to content

x509asn1: improve ASN1tostr with unit tests and fixes#21013

Closed
bagder wants to merge 1 commit intomasterfrom
bagder/ASN1tostr-test
Closed

x509asn1: improve ASN1tostr with unit tests and fixes#21013
bagder wants to merge 1 commit intomasterfrom
bagder/ASN1tostr-test

Conversation

@bagder
Copy link
Copy Markdown
Member

@bagder bagder commented Mar 19, 2026

x509asn1

  • move defines to header file
  • make bit2str require < 8 unused bits
  • make UTime2str show + or - for custom time zones
  • removed unused 'type' argument to ASN1tostr() function
  • fix int2str for "negative" values

unit test 1667

Invoke ASN1tostr() with lots of variations of input and verify output.

@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 19, 2026

Maybe my tweak of the integer input is not entirely correct here as it now can't show negative at all...

@bagder bagder force-pushed the bagder/ASN1tostr-test branch 2 times, most recently from 480f113 to cbcb11d Compare March 20, 2026 08:03
@bagder bagder requested a review from Copilot March 20, 2026 08:04
@bagder bagder marked this pull request as ready for review March 20, 2026 08:04
Copy link
Copy Markdown

Copilot AI left a comment

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 enhances the X.509 ASN.1-to-string conversion utilities (ASN1tostr and helpers) and adds a dedicated unit test (1667) to validate many input/output combinations across ASN.1 tags, improving confidence in certinfo extraction formatting.

Changes:

  • Added unit test 1667 to exercise ASN1tostr() across strings, times, OIDs, NULL, octet/bit strings, integers/enumerated, and booleans.
  • Moved ASN.1 tag/type #defines from x509asn1.c into x509asn1.h for broader visibility.
  • Adjusted conversion behavior in x509asn1.c (BOOLEAN validation, BIT STRING unused-bits validation, UTC time zone rendering, integer formatting changes) and updated call sites for the ASN1tostr() signature change.

Reviewed changes

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

Show a summary per file
File Description
tests/unit/unit1667.c New unit test coverage for ASN1tostr() across many ASN.1 element types.
tests/unit/Makefile.inc Adds unit1667.c to the unit test build list.
tests/data/test1667 Registers the unit test as a runnable test case in the test harness.
tests/data/Makefile.am Includes test1667 in the distributed test list.
lib/vtls/x509asn1.h Exposes ASN.1 type constants via header for relevant TLS backends.
lib/vtls/x509asn1.c Refactors ASN1tostr() signature and tweaks several ASN.1-to-string conversion helpers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit/unit1667.c Outdated
Comment thread tests/unit/unit1667.c Outdated
Comment thread tests/unit/unit1667.c Outdated
Comment thread tests/unit/unit1667.c Outdated
Comment thread lib/vtls/x509asn1.c Outdated
Comment thread lib/vtls/x509asn1.c
Comment thread tests/unit/unit1667.c Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/vtls/x509asn1.c Outdated
Comment thread lib/vtls/x509asn1.c Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit/unit1667.c
Comment thread tests/unit/unit1667.c Outdated
Comment thread lib/vtls/x509asn1.c Outdated
@bagder bagder force-pushed the bagder/ASN1tostr-test branch from 7fedf04 to e20b291 Compare March 20, 2026 10:45
@bagder bagder requested a review from Copilot March 20, 2026 10:45
@bagder
Copy link
Copy Markdown
Member Author

bagder commented Mar 20, 2026

augment review

@augmentcode
Copy link
Copy Markdown

augmentcode bot commented Mar 20, 2026

🤖 Augment PR Summary

Summary: Improves and adds unit coverage for the ASN.1-to-string conversion helpers used by X.509 certificate info extraction.

Changes:

  • Move `CURL_ASN1_*` tag/type constants from `lib/vtls/x509asn1.c` into `lib/vtls/x509asn1.h`.
  • Harden BOOLEAN handling in `bool2str()` and validate BIT STRING unused-bits counts (must be 0..7).
  • Adjust UTC time formatting so explicit timezone offsets keep their leading `+`/`-` sign.
  • Rework `int2str()` to sign-extend up to 32-bit values and print small values as decimal.
  • Simplify `ASN1tostr()` by dropping the forced-type parameter, and expose it via `UNITTEST` for unit testing.
  • Add new unit test 1667 (plus test metadata/Makefile entries) to exercise many `ASN1tostr()` inputs and edge cases.

Notes: The intent appears to be more correct/consistent certinfo rendering, with regression protection via expanded unit tests.

🤖 Was this summary useful? React with 👍 or 👎

Copy link
Copy Markdown

@augmentcode augmentcode bot left a comment

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread lib/vtls/x509asn1.c
Comment thread tests/unit/unit1667.c
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit/unit1667.c Outdated
Comment thread lib/vtls/x509asn1.c
- move defines to header file
- make bit2str require < 8 unused bits
- make bool strings stricter
- make UTime2str show + or - for custom time zones
- removed unused 'type' argument to ASN1tostr() function
- fix int2str for negative values. All values below 10000 are now shown
  in decimal properly, also possibly negative values.

Add unit test 1667 to verify ASN1tostr
@bagder bagder force-pushed the bagder/ASN1tostr-test branch from 6cbb234 to e70c9be Compare March 20, 2026 10:55
@bagder bagder closed this in 14782b3 Mar 20, 2026
@bagder bagder deleted the bagder/ASN1tostr-test branch March 20, 2026 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants