refactor: generalize cert sig preference handling #4379

jmayclin · 2024-01-26T01:05:15Z

This commit rearranges our logic for handling certificate signature preferences. This work is being done to simplify applying certificate signature preferences to the local side of a connection.

Resolved issues:

Related to #4339

Description of changes:

To apply cert signatures both to peer certs and local certs, we need to be able to work with X509* and s2n_cert structures. The s2n_cert_description struct is used to abstract out the information that is used for the validation. It also provides an easy was to validate against certificate key preferences, which will be added in a later PR.

Call-outs:

Having both s2n_security_policy_validate_certificate and s2n_security_policy_validate_sig_scheme_supported probably seems a little overwrought right now, but the separation makes it much easier to add a s2n_security_policy_validate_sig_key_supported method in the future.

Formatting was done with

git clang-format --style="{BasedOnStyle: InheritParentConfig, ColumnLimit: 100}" --commit HEAD~1

Testing:

New unit tests are added for new functionality. Additionally, all existing CI should pass.

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

This commit rearranges our logic for handling certificate signature preferences. This work is being done to simplify applying certificate signature preferences to the local side of a connection.

- add const qualifiers to test descriptions - add deference to pointer construction

- accidentally commited my CMake changes :(

tls/s2n_x509_validator.h

tls/s2n_security_policies.c

crypto/s2n_certificate.h

crypto/s2n_certificate.c

tls/s2n_x509_validator.c

- assert not null arguments - preserve comments - remove unused pkey - result guard rather than result ensure - add comment for sigalgs - rename s2n_cert_description -> s2n_cert_info - remove s2n_cert_info comment

- finish cert description rename - move expect_error to also assert on errno

tls/s2n_x509_validator.c

tls/s2n_security_policies.c

tests/unit/s2n_cert_info_test.c

- reword comment for SHA1 check

- add comments to test - remove extra scope in test

tests/unit/s2n_cert_info_test.c

tls/s2n_security_policies.c

- remove print statement - manually format test structures

- move test description down to for loop - remove validate-cert method

- remove unused variable - add in SHA1 TLS version unit tests

crypto/s2n_certificate.c

crypto/s2n_certificate.h

goatgoose · 2024-02-05T01:16:30Z

tests/unit/s2n_cert_info_test.c

+        DEFER_CLEANUP(X509 *intermediate = NULL, X509_free_pointer);
+        DEFER_CLEANUP(X509 *root = NULL, X509_free_pointer);
+
+        /* read in cert chain */


Just wondering if it would be simpler to use s2n_test_cert_permutation_load_server_chain() and read each x509 with s2n_cert_get_der().

I think it'd definitely be simpler, but this felt a bit "cleaner" in that I am very sure there isn't any s2n_cert parsing side-effect things going on. But happy to change it if you think that makes sense!

tls/s2n_x509_validator.c

tls/s2n_security_policies.h

tests/unit/s2n_security_policy_cert_preferences_test.c

goatgoose · 2024-02-05T03:49:46Z

tests/unit/s2n_x509_validator_certificate_signatures_test.c

    };

-    /* s2n_is_certificate_sig_scheme_supported() */


Shouldn't all of these tests work with s2n_validator_check_cert_preferences()? I see you've added new tests for s2n_security_policy_validate_sig_scheme_supported(), but having tests for s2n_validator_check_cert_preferences() with real certs could be useful, unless you don't think they're valuable anymore.

Shouldn't all of these tests work with s2n_validator_check_cert_preferences()

Yes

but having tests for s2n_validator_check_cert_preferences() with real certs could be useful, unless you don't think they're valuable anymore.

I think it seems a little redundant? E.g. We'd be copy-pasting the same tests cases for two different functions. While I do agree that having tests cases using only higher level functions can be useful, I'd actually prefer to just make those integration tests and e.g. do a handshake using only public APIs and check that the client failed with the appropriate error.

It can be dangerous to remove tests during a refactor. You might miss a behavior change.
If the setup is repetitive, can you fix that without removing the test?

I preserved all test cases, but let me know if I it looks like I missed any functionality.

Certificate signature algorithm is in test certificate signature preferences list

Certificate signature algorithm is not in test certificate signature preferences list

Certificates signed with an RSA PSS signature can be validated

were all moved to s2n_security_policy_cert_preferences_test.c since the functionality of s2n_validate_sig_scheme_supported now lives in s2n_security_policy_validate_sig_scheme_supported

Certificate signature algorithm is in the test certificate signature preferences list but signature is SHA-1 and TLS 1.3 has been negotiated.

Certificate signature algorithm is in the test certificate signature preferences list and signature is SHA-1 and TLS 1.2 has been negotiated.

have both been added as new tests cases for s2n_validator_check_cert_preferences. Previously s2n_validate_sig_scheme_supported/s2n_security_policy_validate_sig_scheme_supported was responsible for handling the TLS1.3/SHA-1 restriction, but to make things more general that validator specific logic has been moved out of s2n_security_policy_validate_sig_scheme_supported and into s2n_validator_check_cert_preferences

tests/unit/s2n_x509_validator_certificate_signatures_test.c

- correct spelling mistake - move s2n_cert_get_info to openssl and rename - move s2n_cert_info_test to the openssl namespace - manually indent clang-formatted code - use s2n_read_test_pem_and_len over alternative - remove left over header definition - switch argument ordering for s2n_security_policy - NULL initialize pointer - prefer result ensure over result bail

tests/testlib/s2n_testlib.h

tests/unit/s2n_openssl_x509_test.c

tls/s2n_security_policies.c

tls/s2n_x509_validator.c

lrstewart · 2024-02-06T01:19:43Z

tests/unit/s2n_x509_validator_certificate_signatures_test.c

    };

-    /* s2n_is_certificate_sig_scheme_supported() */


It can be dangerous to remove tests during a refactor. You might miss a behavior change.
If the setup is repetitive, can you fix that without removing the test?

- switch test method to S2N_RESULT - add trailing commas for test struct - rename s2n_security_policy_ -> s2n_signature_preferences_ - rename validator method

- change arguments of cert validator - change the name again - spearate out the checks for the self-signed

tls/s2n_security_policies.h

tls/s2n_x509_validator.c

- change the name again

- switch to result_ensure_ref bcause of cppcheck

jmayclin requested a review from lrstewart January 26, 2024 01:05

github-actions bot added the s2n-core team label Jan 26, 2024

jmayclin requested a review from maddeleine January 26, 2024 01:05

jmayclin force-pushed the cert-sig-refactor branch from a9b978b to ead852b Compare January 26, 2024 17:40

refactor: generalize cert sig preference handling

cc4ce15

This commit rearranges our logic for handling certificate signature preferences. This work is being done to simplify applying certificate signature preferences to the local side of a connection.

jmayclin force-pushed the cert-sig-refactor branch from ead852b to cc4ce15 Compare January 26, 2024 18:18

jmayclin marked this pull request as ready for review January 26, 2024 22:25

jmayclin added 3 commits January 26, 2024 22:45

address CI errors

25d8af9

- add const qualifiers to test descriptions - add deference to pointer construction

address CI failures

848bf2c

- accidentally commited my CMake changes :(

revert accidental test deletion

98cb0a0

maddeleine reviewed Jan 30, 2024

View reviewed changes

jmayclin added 3 commits January 31, 2024 01:43

address pr feedback

78065c9

- assert not null arguments - preserve comments - remove unused pkey - result guard rather than result ensure - add comment for sigalgs - rename s2n_cert_description -> s2n_cert_info - remove s2n_cert_info comment

address pr feedback

5a5fe0b

- finish cert description rename - move expect_error to also assert on errno

clang format

96dc453

jmayclin requested a review from maddeleine January 31, 2024 01:57

maddeleine reviewed Jan 31, 2024

View reviewed changes

jmayclin added 3 commits January 31, 2024 23:07

address pr feedback

1a67b28

- reword comment for SHA1 check

address pr feedback

f7c60c8

- add comments to test - remove extra scope in test

correct print comment

0d072e4

maddeleine reviewed Feb 1, 2024

View reviewed changes

tests/unit/s2n_cert_info_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_cert_info_test.c Outdated Show resolved Hide resolved

tests/unit/s2n_cert_info_test.c Outdated Show resolved Hide resolved

tls/s2n_security_policies.c Outdated Show resolved Hide resolved

jmayclin added 2 commits February 1, 2024 20:18

address pr feedback

de49c1b

- remove print statement - manually format test structures

address pr feedback

f580a06

- move test description down to for loop - remove validate-cert method

jmayclin requested a review from maddeleine February 1, 2024 21:24

address pr & ci feedback

32a9170

- remove unused variable - add in SHA1 TLS version unit tests

jmayclin requested a review from goatgoose February 2, 2024 18:29

maddeleine approved these changes Feb 2, 2024

View reviewed changes

goatgoose reviewed Feb 5, 2024

View reviewed changes

jmayclin requested a review from goatgoose February 5, 2024 22:51

Merge branch 'main' into cert-sig-refactor

274ed4d

lrstewart reviewed Feb 6, 2024

View reviewed changes

jmayclin added 2 commits February 6, 2024 03:04

address pr feedback

0539943

- switch test method to S2N_RESULT - add trailing commas for test struct - rename s2n_security_policy_ -> s2n_signature_preferences_ - rename validator method

address pr feedback

1753e8d

- change arguments of cert validator - change the name again - spearate out the checks for the self-signed

lrstewart approved these changes Feb 6, 2024

View reviewed changes

tls/s2n_security_policies.h Outdated Show resolved Hide resolved

tls/s2n_x509_validator.c Show resolved Hide resolved

jmayclin added 2 commits February 6, 2024 08:12

address pr feedback

eea8cd2

- change the name again

try upgrading cppcheck

f6b0619

jmayclin requested a review from dougch as a code owner February 6, 2024 08:54

jmayclin added 3 commits February 6, 2024 08:57

change cache key

7540b37

address ci failures

62761b1

- switch to result_ensure_ref bcause of cppcheck

Merge branch 'main' into cert-sig-refactor

7217ddb

jmayclin enabled auto-merge (squash) February 6, 2024 09:29

jmayclin merged commit 5e3fc01 into aws:main Feb 6, 2024
31 checks passed

BrewTestBot mentioned this pull request Feb 14, 2024

s2n 1.4.4 Homebrew/homebrew-core#162729

Merged

jmayclin deleted the cert-sig-refactor branch June 15, 2024 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: generalize cert sig preference handling #4379

refactor: generalize cert sig preference handling #4379

jmayclin commented Jan 26, 2024

goatgoose Feb 5, 2024

jmayclin Feb 5, 2024

goatgoose Feb 5, 2024

jmayclin Feb 5, 2024 •

edited

Loading

lrstewart Feb 6, 2024

jmayclin Feb 6, 2024

lrstewart Feb 6, 2024

refactor: generalize cert sig preference handling #4379

refactor: generalize cert sig preference handling #4379

Conversation

jmayclin commented Jan 26, 2024

Resolved issues:

Description of changes:

Call-outs:

Testing:

goatgoose Feb 5, 2024

Choose a reason for hiding this comment

jmayclin Feb 5, 2024

Choose a reason for hiding this comment

goatgoose Feb 5, 2024

Choose a reason for hiding this comment

jmayclin Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

lrstewart Feb 6, 2024

Choose a reason for hiding this comment

jmayclin Feb 6, 2024

Choose a reason for hiding this comment

lrstewart Feb 6, 2024

Choose a reason for hiding this comment

jmayclin Feb 5, 2024 •

edited

Loading