Make S2N_CERT_AUTH_OPTIONAL the default for clients

[lrstewart]

Resolved issues:

resolves #4382

Description of changes:

Currently, the default client behavior is to consider a CertificateRequest an unexpected message / error, since client auth is completely disabled by default. That seems wrong, since a server can request a certificate without actually requiring the client to provide a certificate, and a client can't necessarily predict the server behavior. That was apparently the use case behind the original work to make clients support S2N_AUTH_TYPE_OPTIONAL. I talked to alexw91, who did the original S2N_AUTH_TYPE_OPTIONAL work for clients, and he doesn't remember a specific reason for not changing the default behavior. He pointed me towards this conversation.

This PR changes the default client behavior so that a client will instead respond to a CertificateRequest with an empty Certificate message. The old behavior is still possible if explicitly configured.

Call-outs:

I have two concerns with this change:

This is a behavior change. If a customer wasn't calling set_client_auth_type because they specifically wanted their clients to error on receiving a CertificateRequest, we have now broken that customer. That sounds like a strange use case to me, but I can't say for sure that no customer is doing that. I'm torn on whether this change is worth the risk.

s2n_config_get_client_auth_type is now somewhat misleading. It reports the client_cert_auth_type even if no override was set and the client_cert_auth_type will therefore be ignored. I think the more correct behavior would be an error, but that would likely break a customer who calls s2n_config_get_client_auth_type for all connections (like maybe for metrics). I think we have to leave the odd behavior as-is.

Testing:

Unit tests

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

[lrstewart]

I was having some real trouble with the SAW proofs. I think something strange is going on with bitfield support, which to be fair is experimental.

I initially tried to use bitfields for both the config and connection, but:

• I couldn't use "crucible_alloc_readonly" with llvm_points_to_bitfield. I didn't record that error.
• I couldn't use bitflags instead of uint8_ts for the new flags: previous CI run example.
• This field reorder is sufficient to cause errors related to other bitflags: https://gist.github.com/lrstewart/19fe915de575f4c6c5077e12a74e5648

[camshaft]

looks good! I don't follow the saw changes enough to review that but the rest looks correct.

[pennyannn]

I was having some real trouble with the SAW proofs. I think something strange is going on with bitfield support, which to be fair is experimental.

I initially tried to use bitfields for both the config and connection, but:

• I couldn't use "crucible_alloc_readonly" with llvm_points_to_bitfield. I didn't record that error.
• I couldn't use bitflags instead of uint8_ts for the new flags: previous CI run example.
• This field reorder is sufficient to cause errors related to other bitflags: https://gist.github.com/lrstewart/19fe915de575f4c6c5077e12a74e5648

@lrstewart , I have tried an experiment where I changed client_cert_auth_type_overridden to be bitfield, and I also get errors. It seems like the bitfield feature is indeed flaky. Maybe newer versions of SAW could help but I'm not sure.

[lrstewart]

@lrstewart , I have tried an experiment where I changed client_cert_auth_type_overridden to be bitfield, and I also get errors. It seems like the bitfield feature is indeed flaky. Maybe newer versions of SAW could help but I'm not sure.

I just tried with 1.1. Same error :(