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.

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 httpd, tweak cipher list #14502

Closed
wants to merge 3 commits into from
Closed

Conversation

icing
Copy link
Contributor

@icing icing commented Aug 12, 2024

Configure the AES 256 instead of the AES 128 cipher in the test httpd to make scorecard testing between httpd and caddy more comparable.

Adapt test_17 expectations, now that AES 128 can no longer, but 256 can now be negotiated.

Configure the AES 256 instead of the AES 128 cipher in the test httpd
to make scorecard testing between httpd and caddy more comparable.

Adapt test_17 expectations, now that AES 128 can no longer, but 256
can now be negotiated.
@icing icing added the tests label Aug 12, 2024
@jan2000
Copy link
Contributor

jan2000 commented Aug 12, 2024

AES128 was intentionally chosen over AES256 to be kept enabled for testing as it should be faster (well, at least in theory). I am not to sure what scoreboard testing is, so I do not understand why AES256 and AES128 must be swapped?

But, if you absolutely must swap the two, please do it consistently. So do swap the TLS 1.2 ciphers as well. Otherwise the "AES 128 ciphers are disabled in the test server" comment is not true, and the test will be harder to understand.

Also, these two lines: [[0x1302, 0x1303], True], and [[0xC02C, 0xC030, 0xCCA9, 0xCCA8], True] are there to test [mis-match, match], this is no longer the case when AES is swapped. So these should be updated as well.

When all swapped out, the list should become:

        [[0x1301], False],
        [[0x1302], True],
        [[0x1303], True],
        [[0x1301, 0x1303], True],
        [[0xC02B, 0xC02F], False],
        [[0xC02C, 0xC030], True],
        [[0xCCA9, 0xCCA8], True],
        [[0xC02B, 0xC02F, 0xCCA9, 0xCCA8], True],

@icing
Copy link
Contributor Author

icing commented Aug 12, 2024

AES128 was intentionally chosen over AES256 to be kept enabled for testing as it should be faster (well, at least in theory). I am not to sure what scoreboard testing is, so I do not understand why AES256 and AES128 must be swapped?

When running tests/http/scorecard.py with GnuTLS, I get half the performance on downloads than with AES256 added.

But, if you absolutely must swap the two, please do it consistently. So do swap the TLS 1.2 ciphers as well. Otherwise the "AES 128 ciphers are disabled in the test server" comment is not true, and the test will be harder to understand.

You think it is important to swap the 1.2 ciphers? I can update the comment.

@jan2000
Copy link
Contributor

jan2000 commented Aug 12, 2024

When running tests/http/scorecard.py with GnuTLS, I get half the performance on downloads than with AES256 added.

Hmm, such a significant performance difference likely suggests that GnuTLS prefers the non hardware-accelerated CHACHA20 over AES128 and uses that instead. Out of curiosity... is this performance difference still there with the SSLHonorCipherOrder turned on, or when CHACHA20 is disabled?

You think it is important to swap the 1.2 ciphers? I can update the comment.

Yes, but for for consistency. It would be easier for someone looking at the test to understand it, when either AES128 or AES256 is completely disabled, than to have a mishmash between protocols and algorithms.

Also while we are at it... when I wrote test_17_07_ssl_ciphers it was the only way that I know to test the cipher suites option. However, I disliked that I had to change the test server configuration for this test, that influences all the other tests. Is there maybe a way the configuration of the test server can be changed for a test in isolation?

@icing
Copy link
Contributor Author

icing commented Aug 12, 2024

When running tests/http/scorecard.py with GnuTLS, I get half the performance on downloads than with AES256 added.

Hmm, such a significant performance difference likely suggests that GnuTLS prefers the non hardware-accelerated CHACHA20 over AES128 and uses that instead. Out of curiosity... is this performance difference still there with the SSLHonorCipherOrder turned on, or when CHACHA20 is disabled?

You think it is important to swap the 1.2 ciphers? I can update the comment.

Yes, but for for consistency. It would be easier for someone looking at the test to understand it, when either AES128 or AES256 is completely disabled, than to have a mishmash between protocols and algorithms.

The test is not for the faint of heart... =)

Also while we are at it... when I wrote test_17_07_ssl_ciphers it was the only way that I know to test the cipher suites option. However, I disliked that I had to change the test server configuration for this test, that influences all the other tests. Is there maybe a way the configuration of the test server can be changed for a test in isolation?

Yes, that is possible to allow an override in the config and reload the server in the test.

@jan2000
Copy link
Contributor

jan2000 commented Aug 12, 2024

Yes, that is possible to allow an override in the config and reload the server in the test.

Can you give me pointers how to that? Then I think it would be better to let the test server have the default cipher config, and let the test_17_07_ssl_ciphers function set it own config. That would also allow for more elaborate testing.

@icing
Copy link
Contributor Author

icing commented Aug 12, 2024

Yes, that is possible to allow an override in the config and reload the server in the test.

Can you give me pointers how to that? Then I think it would be better to let the test server have the default cipher config, and let the test_17_07_ssl_ciphers function set it own config. That would also allow for more elaborate testing.

I just added that to this PR. It is done in test_17_ssl_use.py when the fixture for httpd is created.

@jan2000
Copy link
Contributor

jan2000 commented Aug 12, 2024

Great! If only I had known it could be done like that in the first place.

@icing
Copy link
Contributor Author

icing commented Aug 12, 2024

Should have thought of that. Ah, well.

@bagder bagder closed this in 68dad8c Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants