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.

Sign up for GitHub

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

Test TLS 1.2 builds with each encryption type #6374

Merged
merged 23 commits into from
Oct 12, 2022
Merged

Test TLS 1.2 builds with each encryption type #6374

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
11c362a
Add component to build and test default config with stream cipher only
mprse Sep 27, 2022
89ad623
Fix guards for mbedtls_ct_size_mask() and mbedtls_ct_memcpy_if_eq()
mprse Sep 27, 2022
a82290b
Fix guards for mbedtls_ssl_ticket_write() and mbedtls_ssl_ticket_pars…
mprse Sep 27, 2022
6f29a6c
test_suite_cipher.function: always include aes.h
mprse Sep 27, 2022
4c49927
Fix unused variables warnings in default + stream cipher only build
mprse Sep 27, 2022
d582a01
Make MBEDTLS_SSL_CONTEXT_SERIALIZATION dependent on AEAD
mprse Sep 28, 2022
e31ba83
Use basic symbols instead MBEDTLS_CIPHER_MODE_AEAD in check config
mprse Sep 28, 2022
9550c05
Add component to build and test full config with stream cipher only
mprse Sep 28, 2022
b0de1c0
Add components to build and test default/full config with legacy-ccm …
mprse Sep 28, 2022
0cc3466
Change testing strategy to default + one cypher only (psa/no psa)
mprse Sep 28, 2022
68db0d2
Optimize one cipher only components and adapt nemes
mprse Sep 29, 2022
a891a09
test_suite_cmac.data: fix bug: use cipher type instead cipher id
mprse Sep 29, 2022
8d4b241
Remove redundant indirect dependencies after optimizing setup for one…
mprse Sep 29, 2022
ce5b68c
Revert "Fix guards for mbedtls_ssl_ticket_write() and mbedtls_ssl_tic…
mprse Sep 29, 2022
48a6a66
Add ssl-opt tls 1.2 tests for single cipher builds
mprse Sep 29, 2022
460192e
Fix and sync configuration file and configuration verifiation
mprse Oct 3, 2022
6a5cc74
Fix typos and comments
mprse Oct 3, 2022
0957e7b
Rmove MBEDTLS_NIST_KW_C dependency from MBEDTLS_SSL_TICKET_C
mprse Oct 3, 2022
e32cd44
Add changelog entry: tls 1.2 builds with single encryption type
mprse Oct 5, 2022
52a428b
Fix MBEDTLS_SSL_TICKET_C, MBEDTLS_SSL_SESSION_TICKETS dependencies
mprse Oct 10, 2022
68a01a6
Fix session tickets related build flags in fuzz_server and ssl_server2
mprse Oct 10, 2022
1f02c6c
Reword change log entry
mprse Oct 10, 2022
d61a4d3
Fix missing guard and double-space
mprse Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/mbedtls/check_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,6 +962,9 @@
#error "MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH defined, but not all prerequisites"
#endif

#if defined(MBEDTLS_SSL_SESSION_TICKETS) && !( defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C) )
#error "MBEDTLS_SSL_SESSION_TICKETS defined, but not all prerequisites"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, sorry, can you remind me why SSL_SESSION_TICKETS, as opposed to SSL_TICKET_C, has a dependency on AEAD? I'm afraid we might have done the wrong thing here (and I'm afraid it was based on a suggestion from me, sorry if that's the case.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the commit message:

Both functions are calling mbedtls_cipher_auth_[encrypt/decrypt]ext() functions. These functions are guarded with MBEDTLS_CIPHER_MODE_AEAD || MBEDTLS_NIST_KW_C flags - make it consistent.
As a result ssl_server2 won't build now with MBEDTLS_SSL_SESSION_TICKETS enabled (mbedtls_cipher_auth[encrypt/decrypt]_ext() functions not available).
Mark MBEDTLS_SSL_SESSION_TICKETS as dependent on MBEDTLS_CIPHER_MODE_AEAD || MBEDTLS_NIST_KW_C and disable MBEDTLS_SSL_SESSION_TICKETS in stream cipher only build.

Then MBEDTLS_CIPHER_MODE_AEAD was switched to defined(MBEDTLS_GCM_C) || defined(MBEDTLS_CCM_C) || defined(MBEDTLS_CHACHAPOLY_C) as it couldn't be used in check_config.h.

Then MBEDTLS_NIST_KW_C was removed.

Copy link
Contributor Author

@mprse mprse Oct 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • mbedtls_cipher_auth_[encrypt/decrypt]ext() functions depend on AEAD (MBEDTLS_GCM_C || MBEDTLS_CCM_C || MBEDTLS_CHACHAPOLY_C)
  • These functions are used in ssl_msg.c (guarded with MBEDTLS_GCM_C || MBEDTLS_CCM_C || MBEDTLS_CHACHAPOLY_C) and ssl_ticket.c.
  • As you mentioned here SSL_SESSION_TICKETS should depend on AEAD (GCM || CCM || ChachaPoly), but you refered to SSL_TICKET_C (not SSL_SESSION_TICKETS).
  • We can't build with MBEDTLS_SSL_SESSION_TICKETS and without SSL_TICKET_C. So MBEDTLS_SSL_SESSION_TICKETS also depends on GCM || CCM || ChachaPoly.

Summary:
For me it should be fixed as follows:

  • SSL_TICKET_C depends on GCM || CCM || ChachaPoly.
  • MBEDTLS_SSL_SESSION_TICKETS depends on SSL_TICKET_C.

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach seems right to me.

Copy link
Contributor

@mpg mpg Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, thanks for looking into this. I agree with the first point: SSL_TICKET_C depends on GCM || CCM || ChachaPoly but not with the second: the library should build and be usable with SSL_SESSION_TICKETS enabled and SSL_TICKET_C disabled. If that's not the case, then this is a build and should be fixed.

The thing is, SESSION_TICKETS controls support for the RFC 5077 in the protocol: that is, the new Hello extensions, the new handshake message NewSessionTicket and the new logic to decide when to resume a session. That's all that's needed client-side. On the server side though, one more thing is needed: how to generate and unwrap session tickets, and handle the keys that are used to do so. The RFC doesn't say how this should be done, and there are multiple reasonable strategies for doing it, depending on your kind of deployment (do you use a single server or a pack that need to share keys?), so we leave that to the user, who has to provide callbacks with mbedtls_ssl_conf_session_tickets_cb() (which depends only on SESSION_TICKETS && SRV_C). We could leave it there, but that would force all users to write code for that even for the simplest of use cases, so instead we provide example callbacks that do the job for simple deployments, and that's ssl_ticket.c / SSL_TICKET_C. So, if you're not in the simple use case and took the time to write your own callbacks, you don't want check_config.h to force SSL_TICKET_C on you because you really don't need it.

From what you wrote, I think things are fine in the library, but there are issues with ssl_client2, ssl_server2 and/or tests that use them. I think the goal is as follows:

  • ssl_client2 should support ticket-based resumption with just SESSION_TICKETS and that's it;
  • ssl_server2 can rely on SSL_TICKET_C for ticket-based resumption, because it needs an implementation of the callbacks, and it would be a bit pointless to write a second one here while the one in the library is perfectly suitable - so, perhaps a few guards there need to be changed from SESSION_TICKETS to SESSION_TICKETS && SSL_TICKET_C (edited) (server and client are not symmetrical here);
  • tests in ssl-opt.sh should have dependencies that reflect what they use: if they do ticket-based resumption between ssl_client2 and openssl s_server for example, then SESSION_TICKETS is enough; if they do it between anything and ssl_server2 then SESSION_TICKETS && TICKET_C (edited) is needed.

Final note: there are two kinds of session resumption in TLS 1.2: the "basic" one using a cache, which used to be the only one, and it always enabled as it's part of the protocol, but on the server side requires the application to either use SSL_CACHE_C or write its own code (for example if the cache is to be shared between multiple servers, quite similarly to SSL_TICKET_C), and the "new" one using tickets. All of the above is only about the new one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok but in this case ssl_server2.c should use MBEDTLS_SSL_SESSION_TICKETS && SSL_TICKET_C guards. Linker undefined references:

ssl_server2.c:(.text.startup+0x17b): undefined reference to `mbedtls_ssl_ticket_init'
/usr/bin/ld: ssl_server2.c:(.text.startup+0x93d): undefined reference to `mbedtls_ssl_ticket_free'
/usr/bin/ld: ssl_server2.c:(.text.startup+0x1f1c): undefined reference to `mbedtls_ssl_ticket_setup'
/usr/bin/ld: ssl_server2.c:(.text.startup+0x1f32): undefined reference to `mbedtls_ssl_ticket_parse'
/usr/bin/ld: ssl_server2.c:(.text.startup+0x1f39): undefined reference to `mbedtls_ssl_ticket_write'
/usr/bin/ld: ssl_server2.c:(.text.startup+0x1fcd): undefined reference to `mbedtls_ssl_ticket_rotate'

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, indeed I think the current guards are wrong and MBEDTLS_SSL_SESSION_TICKETS && SSL_TICKET_C would be correct.

#endif


/* Reject attempts to enable options that have been removed and that could
Expand Down
7 changes: 6 additions & 1 deletion library/ssl_ticket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ static int ssl_ticket_gen_key( mbedtls_ssl_ticket_context *ctx,
/*
* Rotate/generate keys if necessary
*/
#if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C)
mpg marked this conversation as resolved.
Show resolved Hide resolved
MBEDTLS_CHECK_RETURN_CRITICAL
static int ssl_ticket_update_keys( mbedtls_ssl_ticket_context *ctx )
{
Expand Down Expand Up @@ -150,6 +151,7 @@ static int ssl_ticket_update_keys( mbedtls_ssl_ticket_context *ctx )
#endif /* MBEDTLS_HAVE_TIME */
return( 0 );
}
#endif

/*
* Rotate active session ticket encryption key
Expand Down Expand Up @@ -293,7 +295,7 @@ int mbedtls_ssl_ticket_setup( mbedtls_ssl_ticket_context *ctx,
* The key_name, iv, and length of encrypted_state are the additional
* authenticated data.
*/

#if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C)
int mbedtls_ssl_ticket_write( void *p_ticket,
const mbedtls_ssl_session *session,
unsigned char *start,
Expand Down Expand Up @@ -390,7 +392,9 @@ int mbedtls_ssl_ticket_write( void *p_ticket,

return( ret );
}
#endif

#if defined(MBEDTLS_CIPHER_MODE_AEAD) || defined(MBEDTLS_NIST_KW_C)
/*
* Select key based on name
*/
Expand Down Expand Up @@ -517,6 +521,7 @@ int mbedtls_ssl_ticket_parse( void *p_ticket,

return( ret );
}
#endif

/*
* Free context
Expand Down
1 change: 1 addition & 0 deletions tests/scripts/all.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,6 +1296,7 @@ component_test_crypto_default_stream_cipher_only () {
scripts/config.py unset MBEDTLS_CTR_DRBG_C
scripts/config.py unset MBEDTLS_CMAC_C
scripts/config.py unset MBEDTLS_NIST_KW_C
scripts/config.py unset MBEDTLS_SSL_SESSION_TICKETS

# Enable stream(null) cipher only
scripts/config.py set MBEDTLS_CIPHER_NULL_CIPHER
Expand Down