Skip to content

Conversation

IngelaAndin
Copy link
Contributor

closes #9632

Copy link
Contributor

github-actions bot commented Jul 3, 2025

CT Test Results

  2 files   17 suites   4m 38s ⏱️
286 tests 282 ✅ 4 💤 0 ❌
302 runs  298 ✅ 4 💤 0 ❌

Results for commit 3c1957e.

♻️ 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

@IngelaAndin IngelaAndin self-assigned this Jul 3, 2025
@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Jul 3, 2025
@IngelaAndin IngelaAndin force-pushed the ingela/public_key/pss-params-sig-alg/OTP-19699 branch 2 times, most recently from 4534fd1 to 23493b2 Compare July 7, 2025 14:53
@IngelaAndin IngelaAndin requested a review from Copilot July 7, 2025 17:56
Copilot

This comment was marked as outdated.

@IngelaAndin IngelaAndin force-pushed the ingela/public_key/pss-params-sig-alg/OTP-19699 branch from 23493b2 to 3cea4cd Compare July 8, 2025 05:55
@IngelaAndin IngelaAndin requested a review from Copilot July 8, 2025 06:35
Copy link

@Copilot 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 adds handling for RSA-PSS key parameters when they are only present in the certificate’s signature algorithm (not in the SubjectPublicKeyInfo), and adds corresponding tests.

  • Introduces a new key_params/2 helper to extract PSS parameters from the signature algorithm if missing from the key info.
  • Updates validate_signature/6 to use key_params/2 before verifying.
  • Adds pkix_pss_params_in_signalg tests in public_key_SUITE.erl and cleans up an EDDSA test case.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/public_key/src/pubkey_cert.erl Added key_params/2, updated validate_signature/6 callsite.
lib/public_key/test/public_key_SUITE.erl Added PSS‐params tests and minor EDDSA test cleanup.
Comments suppressed due to low confidence (3)

lib/public_key/src/pubkey_cert.erl:2200

  • [nitpick] Consider adding a docstring above key_params/2 to explain when and why it extracts parameters from the certificate's signature algorithm, improving maintainability and clarity.
key_params(#'OTPCertificate'{tbsCertificate = TBSCert}, Params) when Params == asn1_NOVALUE;

lib/public_key/test/public_key_SUITE.erl:1466

  • Add a test case covering the scenario where key parameters are explicitly 'NULL' in the SubjectPublicKeyInfo to ensure key_params/2 correctly handles both asn1_NOVALUE and 'NULL' inputs.
pkix_pss_params_in_signalg() ->

lib/public_key/src/pubkey_cert.erl:261

  • [nitpick] The parameter name KeyParams0 is not descriptive; consider renaming it to something like RawKeyParams or InitialParams to clarify its purpose before transformation.
validate_signature(Cert, DerCert, Key, KeyParams0,

@IngelaAndin IngelaAndin force-pushed the ingela/public_key/pss-params-sig-alg/OTP-19699 branch from 3cea4cd to 3071429 Compare July 8, 2025 09:06
@IngelaAndin IngelaAndin requested a review from u3s July 8, 2025 09:15
@IngelaAndin IngelaAndin force-pushed the ingela/public_key/pss-params-sig-alg/OTP-19699 branch from 3ff492d to c894e8e Compare July 8, 2025 16:43
@IngelaAndin IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Jul 9, 2025
Copy link
Contributor

@u3s u3s left a comment

Choose a reason for hiding this comment

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

is this anyhow covered in RFCs?

} = TBSCert,
%% Sometimes parameters may be missing in issuer's "SubjectPublicKeyInfo" but included in
%% the certs "SignatureAlgorithm" for RSA PSS signatures.
case Algo of
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this case is not needed?

  • if you match on ?'id-RSASSA-PSS' in function head
  • 2nd case clause would be handled by existing 2nd function clause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not due avoiding long lines, but I split it differently so as to be able to match in function head which I normally prefer.

@IngelaAndin IngelaAndin force-pushed the ingela/public_key/pss-params-sig-alg/OTP-19699 branch from c894e8e to 986d49e Compare July 10, 2025 06:14
@IngelaAndin
Copy link
Contributor Author

IngelaAndin commented Jul 10, 2025

is this anyhow covered in RFCs?

Not explicitly. I think the ASN.1 specs makes it quite possible and it makes sense for RSA keys as you can then upgrade an existing intermediat RSA cert to an RSA-PSS-PSS cert without changing the intermediate cert. And OpenSSL will verify this chain.

@IngelaAndin IngelaAndin added full-build-and-check Run a more thorough testing of this PR and removed full-build-and-check Run a more thorough testing of this PR labels Jul 10, 2025
@IngelaAndin IngelaAndin force-pushed the ingela/public_key/pss-params-sig-alg/OTP-19699 branch from 986d49e to 972991c Compare July 10, 2025 06:27
@IngelaAndin IngelaAndin force-pushed the ingela/public_key/pss-params-sig-alg/OTP-19699 branch from 972991c to 3c1957e Compare July 10, 2025 06:34
@IngelaAndin IngelaAndin merged commit 072b521 into erlang:maint Jul 10, 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