Skip to content

test_ssl.rb: Refactor test_pqc_sigalg - #1048

Open
junaruga wants to merge 1 commit into
ruby:masterfrom
junaruga:wip/ssl-test_pqc_sigalg
Open

junaruga wants to merge 1 commit into
ruby:masterfrom
junaruga:wip/ssl-test_pqc_sigalg

Conversation

@junaruga

@junaruga junaruga commented May 4, 2026

Copy link
Copy Markdown
Member

I noticed that the rsa-1 is used for certificate (CA) and server in the test/openssl/test_ssl.rb test_pqc_sigalg test, as @ca_key and @ca_cert are created by rsa-1 according to the following logic:

test/openssl/test_ssl.rb

  def setup
...
    @ca_key  = Fixtures.pkey("rsa-1")
    @svr_key = Fixtures.pkey("rsa-2")
    @cli_key = Fixtures.pkey("rsa-3")
...
    @ca_cert  = issue_cert(@ca, @ca_key, 1, @ca_exts, nil, nil)
    @svr_cert = issue_cert(@svr, @svr_key, 2, @ee_exts, @ca_cert, @ca_key)
    @cli_cert = issue_cert(@cli, @cli_key, 3, @ee_exts, @ca_cert, @ca_key)
...
  end

I assume that this may not be intentional. So, I changed the key numbers


  • Align key numbers with the pattern in test/openssl/utils.rb setup: rsa-1 for
    certificate (CA), rsa-2 for server, rsa-3 for client.
  • Use mldsa65-1 for certificate and mldsa65-2 for server.
  • Use rsa-2 for server, because rsa-1 is already used for certificate (@ca_key,
    @ca_cert).

Assisted-by: Claude Code

@rhenium

rhenium commented Oct 16, 2026

Copy link
Copy Markdown
Member

Sorry, I've missed this PR.

I don't really see what this changes, though. It seems we're checking things as expected with or without this diff?

@junaruga
junaruga force-pushed the wip/ssl-test_pqc_sigalg branch from 76abbde to fbe16b3 Compare October 26, 2026 17:29
@junaruga

junaruga commented Oct 26, 2026 •

Copy link
Copy Markdown
Member Author

I don't really see what this changes, though. It seems we're checking things as expected with or without this diff?

There are 2 issues:

  • In test_pqc_sigalg, @ca_key is created by rsa-1 by @ca_key = Fixtures.pkey("rsa-1") in setup in test/openssl/utils.rb. So, on the previous code, the testing key rsa = Fixtures.pkey("rsa-1") in test_pqc_sigalg uses the same key with the testing CA key @ca_key. I added the code comment # rsa-1 is used for @ca_key to clarify my intention. I think that rsa-1 is used for both CA and key is an issue.
    @ca_key = Fixtures.pkey("rsa-1")
    @svr_key = Fixtures.pkey("rsa-2")
    @cli_key = Fixtures.pkey("rsa-3")
  • In setup in utils.rb, the rsa-1 is used for CA key, the rsa-2 is used for server key, and rsa-3 is used for client key. So, there is a pattern foo-1 is for CA key, foo-2 is for server key and foo-3 is for client key. This PR's change is to align with this pattern, mldsa65-1 is used for CA key, the mldsa65-2 is used for a key. The point is the CA key should be created first from foo-1, because other (server/client/a) key is created based on foo-2. This helps me read a code easily.
    @ca_key  = Fixtures.pkey("rsa-1")
    @svr_key = Fixtures.pkey("rsa-2")
    @cli_key = Fixtures.pkey("rsa-3")
    

Does it make sense?

@junaruga
junaruga force-pushed the wip/ssl-test_pqc_sigalg branch 3 times, most recently from 9ac99e6 to 4fa97ae Compare October 26, 2026 17:53
* Align key numbers with the pattern in test/openssl/utils.rb setup: rsa-1 for
  CA, rsa-2 for server, rsa-3 for client.
* Use rsa-2 for server, because rsa-1 is already used for CA (@ca_key,
  @ca_cert).
* Use mldsa65-1 for CA and mldsa65-2 for server.

Assisted-by: Claude:Opus 4.6
@junaruga
junaruga force-pushed the wip/ssl-test_pqc_sigalg branch from 4fa97ae to 22b8d7b Compare October 26, 2026 22:19
@junaruga junaruga changed the title test_ssl.rb: Fix key numbering in test_pqc_sigalg test_ssl.rb: Refactor test_pqc_sigalg Oct 26, 2026
@junaruga

Copy link
Copy Markdown
Member Author

I don't really see what this changes, though. It seems we're checking things as expected with or without this diff?

The current test is not wrong, but this PR is about refactoring the test to make it better.

This branch has not been deployed

No deployments
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.

2 participants