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

Record padding integration test #3715

Merged
merged 32 commits into from
Jan 18, 2023
Merged

Record padding integration test #3715

merged 32 commits into from
Jan 18, 2023

Conversation

franklee26
Copy link
Contributor

@franklee26 franklee26 commented Dec 15, 2022

Resolved issues:

Description of changes:

  • New integration test verifies s2nd can process various-sized padded payloads (happy path)
  • For the remaining negative cases specified in rfc 8446 section 5.4, we would need to either directly manipulate the handshake protocol or the content type field in order to mock these failure scenarios. Instead, we already have unit tests covering these cases but the rfc comments were missing. The aforementioned requirements that are annotated in this pr are:
  1. "Application Data records may contain a zero-length TLSInnerPlaintext.content if the sender desires."
  2. "Implementations MUST NOT send Handshake and Alert records that have a zero-length TLSInnerPlaintext.content; if such a message is received, the receiving implementation MUST terminate the connection with an "unexpected_message" alert."

Below are the rest of the rfc requirements:

  1. "The presence of padding does not change the overall record size limitations: the full encoded TLSInnerPlaintext MUST NOT exceed 2^14 + 1 octets." Already annotated here: https://github.com/aws/s2n-tls/blob/main/tls/s2n_record_read.c#L176
  2. "Padding is a string of zero-valued bytes appended to the ContentType field before encryption. Implementations MUST set the padding octets to all zeros before encrypting." As far as I know, s2n doesn't provide an API for specifying padding block sizes. If we later support this feature, then we need to ensure that padding is simply a stream of zeroed-bytes.

Call-outs:

  • This test uses openssl11 as the client. I choose openssl because it was already setup on my dev desktop and it has a simple api to enable padding to some block size (-record_padding <val>)
  • I'm not sure how reliable or important it it to verify that openssl actually sent a padded payload. I added this check as a sanity measure but I'm open to other ideas.
  • I'm not sure what's the best way to verify that s2n handled the padded record correctly. This test just checks that the process completed successfully and that the data sent is in the output.
  • TODO? Add padding support for s2nc?

Testing:

Manual codebuild run (commit e73a449): https://us-west-2.console.aws.amazon.com/codesuite/codebuild/024603541914/projects/S2nIntegrationV2Batch2/batch/S2nIntegrationV2Batch2%3A51d63535-78ae-4dcc-af06-7ff5af7ea86d?region=us-west-2


from configuration import available_ports, ALL_TEST_CURVES, MINIMAL_TEST_CERTS
from common import Ciphers, ProviderOptions, Protocols, data_bytes
from fixtures import managed_process # lgtm [py/unused-import]

Check notice

Code scanning / CodeQL

Unused import

Import of 'managed_process' is not used.
@franklee26 franklee26 marked this pull request as ready for review December 15, 2022 22:53
@franklee26 franklee26 changed the title [IGNORE] Record padding integration test Record padding integration test Dec 15, 2022
@franklee26 franklee26 changed the title Record padding integration test [IGNORE] Record padding integration test Dec 16, 2022
@franklee26 franklee26 changed the title [IGNORE] Record padding integration test Record padding integration test Jan 4, 2023
@zaherd zaherd requested a review from maddeleine January 5, 2023 17:39
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
tests/integrationv2/test_record_padding.py Outdated Show resolved Hide resolved
Comment on lines 153 to 154
timeout=5, send_marker=str(client_random_bytes)[2:-1])

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you ignoring the first two characters of your random data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The string output of the bytes have the form b'<string>'. Skipping the first two chars is for skipping the literal b'.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest looking at how other tests that don't need to do [2:-1] handle the bytes / str conversions. I remember it's a bit of a mess, but doable. Maybe look at utils::to_string? If you can't figure it out, you need a comment and possible a little wrapper method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I played around with this a little.

You don't need to do this for the close_marker. You do need to do something like this for the send_marker when using openssl (but not s2n). We've gotten around this in different tests in different ways, like my solution from the renegotiation test. But we should fix this at the framework level rather than repeatedly fighting with it in individual tests. Can you open an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'll open an issue

Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

You'll need maddeleine to reapprove, since there have been nontrivial changes.

Also make sure to comment a link to a successful test run before you merge.

@franklee26 franklee26 enabled auto-merge (squash) January 18, 2023 21:30
@franklee26 franklee26 merged commit 0888a96 into main Jan 18, 2023
@franklee26 franklee26 deleted the frank_padding_test branch January 18, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants