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: add sslv2 client hello test w/ jvm #5019

Merged
merged 11 commits into from
Jan 16, 2025
Merged

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Jan 10, 2025

Description of changes:

Based on feedback from #5011 , this PR instead adds an SSLv2 integration using the integrationv2 style.

Testing:

I examined the ClientHello with wireshark and added a debug statement to confirm that the ClientHello that the java client was sending was ClientHello SSLv2.

ubuntu@ip-1-1-1-1:~/workspace/s2n-tls/tests/integrationv2$ uv run pytest --provider-version openssl-3 --best-effort-NOT-FOR-CI -x -rpfs test_sslv2_client_hello.py
============================= test session starts ==============================
platform linux -- Python 3.12.3, pytest-8.3.4, pluggy-1.5.0
rootdir: /home/ubuntu/workspace/s2n-tls/tests/integrationv2
configfile: pyproject.toml
plugins: xdist-3.6.1, rerunfailures-15.0
collected 1 item                                                               

test_sslv2_client_hello.py .                                             [100%]

=========================== short test summary info ============================
PASSED test_sslv2_client_hello.py::test_s2n_server_sslv2_client_hello
============================== 1 passed in 0.62s ===============================

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jan 10, 2025
@jmayclin jmayclin marked this pull request as ready for review January 14, 2025 00:04
@jmayclin jmayclin requested a review from dougch as a code owner January 14, 2025 00:04
Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

Corretto 17 doesn't like this...

Command line: java -classpath bin SSLSocketClient 8002 ../pems/rsa_2048_sha256_client_cert.pem TLS_RSA_WITH_AES_256_CBC_SHA256 TLS1.2 S S L v 2 H e l l o
Exit code: 0
Stdout: 
Stderr: java.lang.IllegalArgumentException: Unsupported protocolS
        at java.base/sun.security.ssl.ProtocolVersion.namesOf(ProtocolVersion.java:292)
        at java.base/sun.security.ssl.SSLSocketImpl.setEnabledProtocols(SSLSocketImpl.java:361)
        at SSLSocketClient.main(SSLSocketClient.java:38)

Which jdk are you using/expecting ? and do we need a way to account for max(java --version) ?

tests/integrationv2/bin/SSLSocketClient.java Outdated Show resolved Hide resolved
@jmayclin jmayclin changed the title Integv2 sslv2 test: Add sslv2 client hello test w/ Java Jan 15, 2025
@jmayclin jmayclin changed the title test: Add sslv2 client hello test w/ Java test: add sslv2 client hello test w/ jvm Jan 15, 2025
* string needed to be byte string
* extra flags neeed to be list
Perhaps in CI autopep8 has a different config that it reads from? But
whenever I try and run it it changes a whole bunch of unrelated lines.
@jmayclin jmayclin requested review from lrstewart and dougch January 15, 2025 01:53
Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

passes locally on my arm nix host 👍

bin/s2nd.c Outdated Show resolved Hide resolved
tests/integrationv2/bin/SSLSocketClient.java Outdated Show resolved Hide resolved
tests/integrationv2/bin/SSLSocketClient.java Show resolved Hide resolved
* rely on existing ClientHello debugging message
* stricter parsing of protocol version
* document protocol assumptions
@jmayclin jmayclin requested a review from lrstewart January 15, 2025 23:24
tests/integrationv2/bin/SSLSocketClient.java Outdated Show resolved Hide resolved
@jmayclin jmayclin enabled auto-merge January 16, 2025 01:19
@jmayclin jmayclin added this pull request to the merge queue Jan 16, 2025
Merged via the queue into aws:main with commit 9fa5f6e Jan 16, 2025
42 checks passed
@jmayclin jmayclin deleted the integv2-sslv2 branch January 16, 2025 01:54
@jmayclin jmayclin mentioned this pull request Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants