feat: add basic support for certificate_authorities #4506
Conversation
| uint16_t extension_type; | ||
| struct s2n_blob extension; | ||
| uint16_t extension_type; |
There was a problem hiding this comment.
Adding a new extension adds a new element to the client_hello parsed extension list, which adds ~40 bytes to the connection. That triggered the s2n_connection_size_test to yell at me for increasing the size of the connection. To avoid that, I swapped these two fields.
In C, the fields of a struct have to meet alignment requirements. If they don't, the compiler inserts empty space so that they do. By swapping extension and extension_type, I fixed the field alignment and avoided that empty space. See http://www.catb.org/esr/structure-packing/
Basically, this is an easy memory win that reduces the size of the parsed extensions array enough to offset the new element I added to it.
There was a problem hiding this comment.
See here: https://godbolt.org/z/f9EE56rP3
| /* RFC 5246 7.4.4 - If the certificate_authorities list is empty, then the | ||
| * client MAY send any certificate of the appropriate ClientCertificateType */ | ||
| uint16_t acceptable_cert_authorities_len = 0; | ||
| POSIX_GUARD(s2n_stuffer_write_uint16(out, acceptable_cert_authorities_len)); |
There was a problem hiding this comment.
To me this reads that for TLS1.2 if certificate_authorities list is empty we can send any cert. Why then write 0 instead of writing nothing?
More confusing is that now we call s2n_cert_authorities_send, which writes the size (0) and presumably an empty cert_authorities blob. So the behavior matches the existing behavior... but we also have a s2n_extension_send_if_tls13_connection which makes it seem like we shouldn't be sending anything for TLS1.2??
Seems like the call to s2n_cert_authorities_send is achieving what we want but it might end up being confusing since we technically do send when not tls13 connection. Am I missing something?
There was a problem hiding this comment.
I think you're getting the two mechanisms mixed up.
- In TLS1.3, we send the extension.
- In TLS1.2, we don't send the extension.
s2n_extension_send_if_tls13_connectionis irrelevant because there's no extension involved. Instead, certificate_authorities is part of the actual CertificateRequest message. That's also why we were writing "0" before-- that's a zero-length certificate_authorities. You can't just write nothing because then you're missing a field and the message is malformed.
There was a problem hiding this comment.
I was missing the fact that s2n_cert_req_send is not called from the tls13 state machine.
Resolved issues:
Splitting #4502 into more reviewable chunks.
Description of changes:
I add support for the certificate_authorities extension in TLS1.3, and the certificate_authorities field of the CertificateRequest message in TLS1.2.
Call-outs:
Currently, there's no mechanism to actually set config->cert_authorities and trigger the extension to be sent. That will be the next PR, where we will load CAs from the trust store. For now, I manually set the field in the tests.
Testing:
New unit tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.