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

Add unit test for setting server cipher preferences in the CH callback #4550

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

phymod0
Copy link

@phymod0 phymod0 commented May 10, 2024

Description of changes:

This adds a new unit test for changing server cipher preferences in the ClientHello callback

From the documentation of s2n_client_hello_fn:

The callback function takes a s2n-tls connection as input, which receives the ClientHello and the context previously provided in s2n_config_set_client_hello_cb. The callback can access any ClientHello information from the connection and use the s2n_connection_set_config call to change the config of the connection.

Within this callback, are calls to s2n_connection_set_cipher_preferences also valid? I found no documentation or unit test that confirms this, so I'm contributing one under the premise that this is the case.

Testing

I confirmed that removing the s2n_connection_set_cipher_preferences() call from the test callback fails the new unit test.

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

@dougch
Copy link
Contributor

dougch commented May 13, 2024

Thanks for the PR ! We'll have a look.

Copy link
Contributor

@goatgoose goatgoose left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Within this callback, are calls to s2n_connection_set_cipher_preferences also valid?

Yes, I believe this should be valid, since the client hello callback is invoked before a cipher suite is negotiated.

tests/unit/s2n_self_talk_client_hello_cb_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_self_talk_client_hello_cb_test.c Outdated Show resolved Hide resolved
@@ -104,6 +105,22 @@ int mock_client(struct s2n_test_io_pair *io_pair, int expect_failure, int expect
exit(result);
}

bool security_policy_contains_cipher(const char* security_policy_name, uint8_t* cipher_iana) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, it looks like this is working with the FIPS test now. The only failing test is for clang-format:

Suggested change
bool security_policy_contains_cipher(const char* security_policy_name, uint8_t* cipher_iana) {
bool security_policy_contains_cipher(const char *security_policy_name, uint8_t *cipher_iana)
{

Comment on lines +25 to 26
#include "tls/s2n_security_policies.h"
#include "tls/s2n_internal.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

clang-format:

Suggested change
#include "tls/s2n_security_policies.h"
#include "tls/s2n_internal.h"
#include "tls/s2n_internal.h"
#include "tls/s2n_security_policies.h"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants