Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ktls: support aes256 #4227

Merged
merged 4 commits into from
Oct 5, 2023
Merged

ktls: support aes256 #4227

merged 4 commits into from
Oct 5, 2023

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Sep 28, 2023

Description of changes:

Adds support for AES256 GCM, as well as refactors how ciphers are supported to make future ciphers easier to add.

After this change, support for new ciphers can be added by appending the new crypto_info to the union in s2n_ktls_crypto_info and implementing the cipher set_ktls_info function. Theoretically the new union member in s2n_ktls_crypto_info is optional: we could allocate the memory for the crypto_info on the heap instead, but the union is an easy way to avoid that.

Call-outs:

To avoid the conditional compilation leaking into more files in /crypto, I made versions of the symbols that ktls needs for crypto_info available even without ktls support.

Testing:

Rewrote the unit tests to use the new cipher method, + added tests of each cipher to the self-talk test.

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

@github-actions github-actions bot added the s2n-core team label Sep 28, 2023
@lrstewart lrstewart force-pushed the ktls_cipher branch 2 times, most recently from ffe5619 to e115762 Compare September 28, 2023 11:20
@lrstewart lrstewart marked this pull request as ready for review September 28, 2023 16:28
@lrstewart lrstewart requested review from camshaft, maddeleine and goatgoose and removed request for maddeleine September 28, 2023 16:28
@lrstewart lrstewart force-pushed the ktls_cipher branch 2 times, most recently from 7085c6d to 5a28c27 Compare October 3, 2023 05:57
@lrstewart lrstewart requested a review from goatgoose October 3, 2023 06:54
uint8_t (*is_available)(void);
int (*init)(struct s2n_session_key *key);
int (*set_decryption_key)(struct s2n_session_key *key, struct s2n_blob *in);
int (*set_encryption_key)(struct s2n_session_key *key, struct s2n_blob *in);
int (*destroy_key)(struct s2n_session_key *key);
S2N_RESULT (*set_ktls_info)(struct s2n_ktls_crypto_info_inputs *inputs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great idea!

@lrstewart lrstewart enabled auto-merge (squash) October 5, 2023 04:22
@lrstewart lrstewart requested a review from goatgoose October 5, 2023 19:40
@lrstewart lrstewart merged commit 2a6ead7 into aws:main Oct 5, 2023
@lrstewart lrstewart deleted the ktls_cipher branch October 5, 2023 21:06
@justinr1234
Copy link

@lrstewart Any help with this would be greatly appreciated!

Starting with this commit I can't build anymore in a clean docker env on linux (arm64 and amd64 both):

[  0%] Building C object CMakeFiles/s2n.dir/crypto/s2n_aead_cipher_aes_gcm.c.o
In file included from /opt/s2n-tls/crypto/s2n_cipher.h:26,
                 from /opt/s2n-tls/crypto/s2n_aead_cipher_aes_gcm.c:19:
/opt/s2n-tls/crypto/s2n_ktls_crypto.h:55:48: error: field 'aes_gcm_256' has incomplete type
         s2n_ktls_crypto_info_tls12_aes_gcm_256 aes_gcm_256;
                                                ^~~~~~~~~~~
/opt/s2n-tls/crypto/s2n_aead_cipher_aes_gcm.c: In function 's2n_aead_cipher_aes256_gcm_set_ktls_info':
/opt/s2n-tls/crypto/s2n_aead_cipher_aes_gcm.c:421:16: error: dereferencing pointer to incomplete type 's2n_ktls_crypto_info_tls12_aes_gcm_256' {aka 'struct tls12_crypto_info_aes_gcm_256'}
     crypto_info->info.version = TLS_1_2_VERSION;
                ^~
/opt/s2n-tls/crypto/s2n_aead_cipher_aes_gcm.c:422:37: error: 'TLS_CIPHER_AES_GCM_256' undeclared (first use in this function); did you mean 'TLS_CIPHER_AES_GCM_128'?
     crypto_info->info.cipher_type = TLS_CIPHER_AES_GCM_256;
                                     ^~~~~~~~~~~~~~~~~~~~~~
                                     TLS_CIPHER_AES_GCM_128
/opt/s2n-tls/crypto/s2n_aead_cipher_aes_gcm.c:422:37: note: each undeclared identifier is reported only once for each function it appears in
In file included from /opt/s2n-tls/error/s2n_errno.h:23,
                 from /opt/s2n-tls/utils/s2n_safety.h:23,
                 from /opt/s2n-tls/crypto/s2n_ecc_evp.h:24,
                 from /opt/s2n-tls/crypto/s2n_ecdsa.h:22,
                 from /opt/s2n-tls/crypto/s2n_pkey.h:20,
                 from /opt/s2n-tls/crypto/s2n_certificate.h:22,
                 from /opt/s2n-tls/tls/s2n_crypto.h:18,
                 from /opt/s2n-tls/crypto/s2n_aead_cipher_aes_gcm.c:21:
/opt/s2n-tls/crypto/s2n_aead_cipher_aes_gcm.c:444:20: error: invalid application of 'sizeof' to incomplete type 's2n_ktls_crypto_info_tls12_aes_gcm_256' {aka 'struct tls12_crypto_info_aes_gcm_256'}
             sizeof(s2n_ktls_crypto_info_tls12_aes_gcm_256)));
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/s2n-tls/utils/s2n_ensure.h:35:15: note: in definition of macro '__S2N_ENSURE'
         if (!(cond)) {             \
               ^~~~
/opt/s2n-tls/crypto/s2n_aead_cipher_aes_gcm.c:443:5: note: in expansion of macro 'RESULT_GUARD_POSIX'
     RESULT_GUARD_POSIX(s2n_blob_init(&out->value, (uint8_t *) (void *) crypto_info,
     ^~~~~~~~~~~~~~~~~~
make[2]: *** [CMakeFiles/s2n.dir/build.make:63: CMakeFiles/s2n.dir/crypto/s2n_aead_cipher_aes_gcm.c.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:8665: CMakeFiles/s2n.dir/all] Error 2
make: *** [Makefile:141: all] Error 2
The command '/bin/sh -c S2N_DONT_MLOCK=1 make' returned a non-zero code: 2

@lrstewart
Copy link
Contributor Author

Try this patch: https://gist.github.com/lrstewart/42e91a507cfed98263b646520930dfb3 Let me know if that works and I'll put out a PR

@camshaft
Copy link
Contributor

this should be addressed in #4295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants