fix: better error for setting bad quic policy

[lrstewart]

Resolved issues:

Related to #4594

Description of changes:

A customer ran into the situation where they attempted to set "default_fips" as their security policy while using s2n-quic. "default_fips" does not include any TLS1.3 cipher suites, so was not valid and led to runtime failures.

This change makes an (admittedly inadequate) attempt to prevent similar failures. My stance is that it's better than nothing-- see Call-outs below.

However, this change is still maybe a breaking change. Technically you could call s2n_config_enable_quic, s2n_config_set_cipher_preferences(TLS1.2), and then s2n_config_set_cipher_preferences(TLS1.3) before this change without issue, but that sequence of calls would now result in an error. I don't know if that is a realistic use case though, and it's not a use case that should obviously work. Once you've enabled quic, a TLS1.2-only policy is wrong.

Call-outs:

Could we do better? I couldn't think of an actually good solution.

The biggest problem use cases:

1. s2n_config_set_cipher_preferences(TLS1.2), s2n_config_enable_quic, s2n_config_set_cipher_preferences(TLS1.3): This is why we can't validate in s2n_config_enable_quic. The application is allowed to change the security policy later, and this sequence is valid.
2. s2n_connection_set_cipher_preferences(TLS1.2), s2n_connection_enable_quic, s2n_connection_set_cipher_preferences(TLS1.3): Basically the same as the previous case. This is why we can't validate in s2n_connection_enable_quic. The application is allowed to change the security policy later, and this sequence is valid.
3. s2n_config_set_cipher_preferences(TLS1.2), s2n_config_enable_quic, s2n_connection_set_config: Not a valid sequence. We could catch the error if we validated in s2n_connection_set_config, but the error would have nothing to do with the connection and be fairly unexpected. In Rust, it would not show up in the config::Builder.
4. s2n_config_set_cipher_preferences(TLS1.2), s2n_connection_set_config, s2n_connection_enable_quic: Not a valid sequence. We couldn't catch this error even if we validated in s2n_connection_set_config.
5. s2n_config_enable_quic, s2n_connection_set_config, s2n_connection_set_cipher_preferences(tls1.3): A valid sequence, but would be broken by any validation of the config in s2n_connection_set_config or config::Builder. The config isn't valid on its own, but is valid in combination with the connection.

Validation in s2n_connection_set_config or config::Builder couldn't catch cases 2 or 4. And because of case 5, I don't believe we can actually do any validation in s2n_connection_set_config or config::Builder without breaking valid use cases.

Since the only current quic integration is s2n-quic and therefore Rust, we could focus on our Rust bindings, specifically on config::Builder::build. Given the above constraints, the only "better" solutions I can think of are:

1. Add some sort of new validation method to call from config::Builder::build. This might be useful to C customers too, except that we couldn't assume that our C customers actually call it. So it'd probably go in s2n_internal.h. Not possible because of case 5.
2. Save the policy set on the builder and delay calling s2n_config_set_cipher_preferences until config::Builder::build. That's not free because the policy can be backed by a CString we'd need to clone because the builder only takes a reference to the policy. I suppose the config builder probably doesn't need to be crazy performant, but it does seem really wasteful :/
3. Add a s2n_config_get_cipher_preferences method to s2n_internal.h to call from config::Builder::build so that we can call s2n_config_set_cipher_preferences again from config::Builder::build. That just seems like a more hacky version of solution 1 though.

Testing:

Unit tests.

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

[camshaft]

Add some sort of new validation method to call from config::Builder::build. This might be useful to C customers too, except that we couldn't assume that our C customers actually call it. So it'd probably go in s2n_internal.h.

This would be my preference

[lrstewart]

Add some sort of new validation method to call from config::Builder::build. This might be useful to C customers too, except that we couldn't assume that our C customers actually call it. So it'd probably go in s2n_internal.h.

This would be my preference

I started on that solution and ran into a problem. The case "s2n_config_enable_quic, s2n_connection_set_config, s2n_connection_set_cipher_preferences(tls1.3)" is valid, but would be flagged as invalid by any check we do in s2n_connection_set_config / config::Builder::build. Validating in config::Builder::build might not be possible at all given the connection overrides.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.