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

fix: add coverage for all ticket formats #4997

Merged

Conversation

CarolYeh910
Copy link
Contributor

Release Summary:

Resolved issues:

resolves #4965

Description of changes:

There are two changes in this PR:

  1. Remove the manual setting of session ticket formats in s2n_deserialize_resumption_state_test.c. We no longer make assumptions about the possible format versions

  2. Update the path to coverage reports in Readme

Call-outs:

N/A

Testing:

Generated fuzz coverage reports locally and confirmed that S2N_SERIALIZED_FORMAT_TLS12_V3 is covered

Fuzz_coverage

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 4, 2025
@CarolYeh910 CarolYeh910 requested review from jmayclin and jouho January 4, 2025 01:08
@lrstewart lrstewart requested review from lrstewart and removed request for jouho January 4, 2025 04:52
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.

Nice! From your new coverage report, are there any concerns that we're only covering the server code path and not the client code path?

@@ -44,7 +44,6 @@ int s2n_fuzz_test(const uint8_t *buf, size_t len)
/* There are only a few valid formats for session tickets; this ensures the
* format version is at or below S2N_SERIALIZED_FORMAT_TLS12_V3, which will
* keep the test checking mostly valid paths. */
randval = randval % S2N_SERIALIZED_FORMAT_TLS12_V3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this!

Since we're removing this line, let's go ahead and clean up the comment/setup that is no longer used.

@CarolYeh910
Copy link
Contributor Author

Nice! From your new coverage report, are there any concerns that we're only covering the server code path and not the client code path?

The result seems to make sense because the connection mode is set to S2N_SERVER in the test file. If we want to cover the client code path, we may need to add additional tests to this file.

DEFER_CLEANUP(struct s2n_connection *server_conn = s2n_connection_new(S2N_SERVER), s2n_connection_ptr_free);

@CarolYeh910 CarolYeh910 requested a review from jmayclin January 6, 2025 20:49
@CarolYeh910 CarolYeh910 added this pull request to the merge queue Jan 6, 2025
Merged via the queue into aws:main with commit 8c50fc7 Jan 6, 2025
40 checks passed
@CarolYeh910 CarolYeh910 deleted the fuzz-coverage-for-all-ticket-formats branch January 6, 2025 23:46
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.

add fuzz coverage for all session ticket formats
3 participants