Skip to content

Commit

Permalink
Remove count + change limit messaging
Browse files Browse the repository at this point in the history
  • Loading branch information
@lrstewart
lrstewart committed Apr 29, 2024
1 parent aadd636 commit fb33549
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 41 deletions.
6 changes: 4 additions & 2 deletions api/s2n.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3776,14 +3776,16 @@ S2N_API int s2n_connection_deserialize(struct s2n_connection *conn, uint8_t *buf
* a trust store that includes the default system certificates. That means that
* s2n_config_new_minimal or s2n_config_wipe_trust_store should be used.
*
* s2n-tls limits the total certificate authorities size to 10k bytes.
* s2n-tls currently limits the total certificate authorities size to 10k bytes.
* This method will fail if the certificate authorities retrieved from the trust
* store exceed that limit.
*
* @param config A pointer to the s2n_config object.
* @param count The number of certificate authorities loaded from the trust store.
* Can be used for logging or to sanity check the trust store configuration.
* @returns S2N_SUCCESS on success. S2N_FAILURE on failure.
*/
S2N_API int s2n_config_set_cert_authorities_from_trust_store(struct s2n_config *config, uint16_t *count);
S2N_API int s2n_config_set_cert_authorities_from_trust_store(struct s2n_config *config);

#ifdef __cplusplus
}
Expand Down
37 changes: 8 additions & 29 deletions tests/unit/s2n_cert_authorities_test.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,8 @@ int main(int argc, char **argv)
{
/* Test: Safety */
{
uint16_t count = 0;
EXPECT_FAILURE_WITH_ERRNO(
s2n_config_set_cert_authorities_from_trust_store(NULL, &count),
S2N_ERR_NULL);

DEFER_CLEANUP(struct s2n_config *config = s2n_config_new_minimal(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
EXPECT_FAILURE_WITH_ERRNO(
s2n_config_set_cert_authorities_from_trust_store(config, NULL),
s2n_config_set_cert_authorities_from_trust_store(NULL),
S2N_ERR_NULL);
};

Expand All @@ -66,14 +59,12 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config,
S2N_ECDSA_P512_CERT_CHAIN, NULL));

uint16_t count = 0;
if (s2n_cert_authorities_supported_from_trust_store()) {
EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config, &count));
EXPECT_EQUAL(count, 1);
EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config));
EXPECT_NOT_EQUAL(config->cert_authorities.size, 0);
} else {
EXPECT_FAILURE_WITH_ERRNO(
s2n_config_set_cert_authorities_from_trust_store(config, &count),
s2n_config_set_cert_authorities_from_trust_store(config),
S2N_ERR_INTERNAL_LIBCRYPTO_ERROR);
EXPECT_EQUAL(config->cert_authorities.size, 0);
}
Expand All @@ -86,26 +77,22 @@ int main(int argc, char **argv)
EXPECT_NOT_NULL(config);

/* Fails with default system trust store */
uint16_t count = 0;
EXPECT_FAILURE_WITH_ERRNO(
s2n_config_set_cert_authorities_from_trust_store(config, &count),
s2n_config_set_cert_authorities_from_trust_store(config),
S2N_ERR_INVALID_STATE);
EXPECT_EQUAL(config->cert_authorities.size, 0);

/* Succeeds again after wiping trust store */
EXPECT_SUCCESS(s2n_config_wipe_trust_store(config));
EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config, &count));
EXPECT_EQUAL(count, 0);
EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config));
EXPECT_EQUAL(config->cert_authorities.size, 0);
};

/* Test: empty trust store */
{
DEFER_CLEANUP(struct s2n_config *config = s2n_config_new_minimal(), s2n_config_ptr_free);
EXPECT_NOT_NULL(config);
uint16_t count = 0;
EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config, &count));
EXPECT_EQUAL(count, 0);
EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config));
EXPECT_EQUAL(config->cert_authorities.size, 0);
};

Expand All @@ -116,9 +103,8 @@ int main(int argc, char **argv)
/* This is just a copy of the default trust store from an Amazon Linux instance */
EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config, S2N_TEST_TRUST_STORE, NULL));

uint16_t count = 0;
EXPECT_FAILURE_WITH_ERRNO(
s2n_config_set_cert_authorities_from_trust_store(config, &count),
s2n_config_set_cert_authorities_from_trust_store(config),
S2N_ERR_TOO_MANY_CAS);
EXPECT_EQUAL(config->cert_authorities.size, 0);
};
Expand Down Expand Up @@ -303,13 +289,11 @@ int main(int argc, char **argv)
/* clang-format off */
const struct {
const char *cert_name;
uint8_t expected_count;
uint8_t expected_bytes_size;
uint8_t expected_bytes[1000];
} test_cases[] = {
{
.cert_name = S2N_RSA_PSS_2048_SHA256_LEAF_CERT,
.expected_count = 1,
.expected_bytes_size = 32,
.expected_bytes = {
0x00, 0x2f, 0x00, 0x1c, 0x00, 0x1a, 0x00, 0x18,
Expand All @@ -320,7 +304,6 @@ int main(int argc, char **argv)
},
{
.cert_name = S2N_ECDSA_P512_CERT_CHAIN,
.expected_count = 1,
.expected_bytes_size = 107,
.expected_bytes = {
0x00, 0x2f, 0x00, 0x67, 0x00, 0x65, 0x00, 0x63,
Expand All @@ -341,7 +324,6 @@ int main(int argc, char **argv)
},
{
.cert_name = S2N_RSA_2048_SHA256_URI_SANS_CERT,
.expected_count = 2,
.expected_bytes_size = 192,
.expected_bytes = {
0x00, 0x2f, 0x00, 0xbc, 0x00, 0xba, 0x00, 0x53,
Expand Down Expand Up @@ -372,7 +354,6 @@ int main(int argc, char **argv)
},
{
.cert_name = S2N_RSA_2048_PKCS1_CERT_CHAIN,
.expected_count = 3,
.expected_bytes_size = 94,
.expected_bytes = {
0x00, 0x2f, 0x00, 0x5a, 0x00, 0x58, 0x00, 0x1a,
Expand All @@ -398,9 +379,7 @@ int main(int argc, char **argv)
EXPECT_SUCCESS(s2n_config_set_verification_ca_location(config,
test_cases[i].cert_name, NULL));

uint16_t count = 0;
EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config, &count));
EXPECT_EQUAL(count, test_cases[i].expected_count);
EXPECT_SUCCESS(s2n_config_set_cert_authorities_from_trust_store(config));

DEFER_CLEANUP(struct s2n_connection *conn = s2n_connection_new(S2N_SERVER),
s2n_connection_ptr_free);
Expand Down
9 changes: 3 additions & 6 deletions tls/extensions/s2n_cert_authorities.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,9 @@ bool s2n_cert_authorities_supported_from_trust_store()
#endif
}

static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *config, uint16_t *count)
static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *config)
{
RESULT_ENSURE_REF(config);
RESULT_ENSURE_REF(count);
*count = 0;

if (!config->trust_store.trust_store) {
return S2N_RESULT_OK;
Expand Down Expand Up @@ -69,7 +67,6 @@ static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *c
RESULT_GUARD_POSIX(s2n_stuffer_write_bytes(&output, name_bytes, name_size));
RESULT_ENSURE(s2n_stuffer_data_available(&output) <= S2N_CERT_AUTHORITIES_MAX_SIZE,
S2N_ERR_TOO_MANY_CAS);
(*count)++;
}

RESULT_GUARD_POSIX(s2n_stuffer_extract_blob(&output, &config->cert_authorities));
Expand All @@ -79,11 +76,11 @@ static S2N_RESULT s2n_cert_authorities_set_from_trust_store(struct s2n_config *c
#endif
}

int s2n_config_set_cert_authorities_from_trust_store(struct s2n_config *config, uint16_t *count)
int s2n_config_set_cert_authorities_from_trust_store(struct s2n_config *config)
{
POSIX_ENSURE_REF(config);
POSIX_ENSURE(!config->trust_store.loaded_system_certs, S2N_ERR_INVALID_STATE);
POSIX_GUARD_RESULT(s2n_cert_authorities_set_from_trust_store(config, count));
POSIX_GUARD_RESULT(s2n_cert_authorities_set_from_trust_store(config));
return S2N_SUCCESS;
}

Expand Down
10 changes: 6 additions & 4 deletions tls/extensions/s2n_cert_authorities.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
#include "tls/extensions/s2n_extension_type.h"
#include "tls/s2n_connection.h"

/* The only fixed bound on the size of the certificate_authorities is the maximum
* size of an extension (UINT16_MAX), but we need to ensure enough space for other
* extensions as well.
/* The only defined bound on the size of the certificate_authorities is the maximum
* size of an extension, UINT16_MAX. However, the full extensions list is also
* limited to UINT16_MAX, so all the extensions on a message combined cannot exceed
* UINT16_MAX. Other extensions could therefore limit the maximum size of the
* certificate_authorities extension.
*
* To keep the limit predictable and avoid surprise errors during negotiation,
* set a reasonable limit.
* set a reasonable fixed limit.
*/
#define S2N_CERT_AUTHORITIES_MAX_SIZE (10000)

Expand Down

0 comments on commit fb33549

Please sign in to comment.