Skip to content

Conversation

@dgud
Copy link
Contributor

@dgud dgud commented Nov 25, 2025

Extensions was not looked up correctly they are split depending on extension structure, currently three different functions.

Clean up code in public_key to lookup the extensions before enc/decoding them so that all supported extensions will be available.

Also remove the type that where extensions and couldn't be called directly, SubjectAltName and FreshestCRL.

Test encode/decode all them.

Fixes #10404

@dgud dgud self-assigned this Nov 25, 2025
@dgud dgud added the team:PS Assigned to OTP team PS label Nov 25, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 25, 2025

CT Test Results

  2 files   17 suites   6m 53s ⏱️
292 tests 286 ✅ 6 💤 0 ❌
308 runs  302 ✅ 6 💤 0 ❌

Results for commit 0de5a61.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@dgud dgud added the testing currently being tested, tag is used by OTP internal CI label Nov 25, 2025
@dgud dgud requested a review from IngelaAndin November 25, 2025 18:27
@dgud dgud requested a review from Copilot November 26, 2025 08:56
Copilot finished reviewing on behalf of dgud November 26, 2025 09:00
Copy link

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 refactors the certificate extension encoding and decoding logic in the public_key module to fix issues where extensions were not being looked up correctly. The changes unify extension handling through a cleaner lookup mechanism and add comprehensive test coverage for all supported extension types.

Key Changes

  • Refactored der_encode/2 and der_decode/2 to use a two-tier lookup: first checking direct ASN.1 module mappings via get_asn1_module/1, then falling back to extension-specific handling via ext_oid/1
  • Enhanced extension_id/1 to return a 4-tuple {Module, DecodeFunc, EncodeFunc, ExtensionName} containing all metadata needed for extension processing, enabling proper routing to the correct ASN.1 decoder/encoder functions
  • Removed direct ASN.1 module mappings for SubjectAltName and FreshestCRL from get_asn1_module/1, allowing them to be handled exclusively through the extension infrastructure
  • Added comprehensive test ext_encoding/1 that validates encode/decode round-trips for 22 different extension types

Reviewed changes

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

File Description
lib/public_key/test/public_key_SUITE.erl Added comprehensive test ext_encoding/1 covering 22 extension types and error handling for unknown types
lib/public_key/src/public_key.erl Refactored der_encode/2 and der_decode/2 to use unified extension lookup, removed hardcoded extension type guards, added catch-all clause to get_asn1_module/1, and improved error reporting
lib/public_key/src/pubkey_cert_records.erl Refactored extension_id/1 to return 4-tuple with module and function metadata, updated decode_extensions/2 and encode_extensions/1 to use new tuple structure

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

Extensions was not looked up correctly they are split depending
on extension structure, currently three different functions.

Clean up code in public_key to lookup the extensions before
enc/decoding them so that all supported extensions will be available.

Also remove the type that where extensions and couldn't be called
directly, SubjectAltName and FreshestCRL.

Test encode/decode all them.
@dgud dgud force-pushed the dgud/public_key/ext_coding/GH-10404/OTP-19869 branch from cdda767 to 0de5a61 Compare November 26, 2025 12:35
@dgud dgud requested a review from IngelaAndin November 26, 2025 12:37
@dgud dgud merged commit bb462d3 into erlang:maint Nov 27, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants