Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: fix test test-tls-dhe for OpenSSL32 #54903

Merged
merged 2 commits into from
Sep 14, 2024
Merged

test: fix test test-tls-dhe for OpenSSL32 #54903

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fcb989
test: fix test test-tls-dhe for OpenSSL32
mhdawson Sep 5, 2024
722f460
squash: fixup missed error
mhdawson Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions test/parallel/test-tls-dhe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,12 @@ const dheCipher = 'DHE-RSA-AES128-SHA256';
const ecdheCipher = 'ECDHE-RSA-AES128-SHA256';
const ciphers = `${dheCipher}:${ecdheCipher}`;

// Test will emit a warning because the DH parameter size is < 2048 bits
common.expectWarning('SecurityWarning',
'DH parameter is less than 2048 bits');
if (!common.hasOpenSSL(3, 2)) {
// Test will emit a warning because the DH parameter size is < 2048 bits
// when the test is run on versions lower than OpenSSL32
common.expectWarning('SecurityWarning',
'DH parameter is less than 2048 bits');
}

function loadDHParam(n) {
const keyname = `dh${n}.pem`;
Expand Down Expand Up @@ -104,7 +107,11 @@ function testCustomParam(keylen, expectedCipher) {
}, /DH parameter is less than 1024 bits/);

// Custom DHE parameters are supported (but discouraged).
await testCustomParam(1024, dheCipher);
if (!common.hasOpenSSL(3, 2)) {
await testCustomParam(1024, dheCipher);
} else {
await testCustomParam(3072, dheCipher);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to test 3072-bit keys? We're already testing 2048-bit keys on the line below. And if we do need to test 3072-bit keys, that could be with all version of OpenSSL?

Copy link
Member Author

Choose a reason for hiding this comment

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

We tested with two different keys before so I kept it that way, we could add 3072 for all versions but that might add to runtime. I think what is in the PR is my first choice but I don't feel to strongly one way or the other.

Copy link
Member

@lpinca lpinca Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think testing the 3072-bit key will only slow down the test, but I also have no strong opinion.

}
await testCustomParam(2048, dheCipher);

// Invalid DHE parameters are discarded. ECDHE remains enabled.
Expand Down
Loading