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

Fix compile errors without 0-RTT, MPS and compatible mode #359

Open
wants to merge 17 commits into
base: tls13-prototype
Choose a base branch
from

Conversation

yuhaoth
Copy link
Collaborator

@yuhaoth yuhaoth commented Aug 13, 2021

When unset 0-RTT,MPS and TLS13_COMPATIBLE, it reports
compile fail.

That's prepare for Mbed-TLS#4417 . Try to figure out the minimal sets for handshake.

@yuhaoth yuhaoth added the needs work Some requests need addressing label Aug 13, 2021
Copy link
Collaborator

@hanno-becker hanno-becker left a comment

Choose a reason for hiding this comment

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

Just had a quick look: The introduction of MBEDTLS_SSL_EARLY_DATA was deliberately not made dependent on MBEDTLS_ZERO_RTT to keep the state machine logic as static as possible. Instead, it's the 0-RTT handler that skips the state it if it's not enabled.

Either way works - I personally prefer less #ifdefs in the code, and dummy functions instead, so would rather keep the code as is. Other opinions? @lhuang04 @zhihan?

@lhuang04
Copy link
Collaborator

Just had a quick look: The introduction of MBEDTLS_SSL_EARLY_DATA was deliberately not made dependent on MBEDTLS_ZERO_RTT to keep the state machine logic as static as possible. Instead, it's the 0-RTT handler that skips the state it if it's not enabled.

Either way works - I personally prefer less #ifdefs in the code, and dummy functions instead, so would rather keep the code as is. Other opinions? @lhuang04 @zhihan?

I'm fine either way. As we will always have MBEDTLS_ZERO_RTT defined. This change should be a no-op for us.

@yuhaoth
Copy link
Collaborator Author

yuhaoth commented Aug 13, 2021

Just had a quick look: The introduction of MBEDTLS_SSL_EARLY_DATA was deliberately not made dependent on MBEDTLS_ZERO_RTT .

I try to wrapper early data codes with MBEDTLS_SSL_EARLY_DATA_SUPPORT , but not success.
I'd like introduce MBEDTLS_SSL_EARLY_DATA_SUPPORT in future.

to keep the state machine logic as static as possible. Instead, it's the 0-RTT handler that skips the state it if it's not enabled.

That's due to maintenance reason, right?
But it will increase the code size if MBEDTLS_ZERO_RTT is not defined.

@hanno-becker
Copy link
Collaborator

@yuhaoth A lot of overhead will be removed by the compiler because it's happening in a single compilation unit - but some might remain, so you have a point regarding code-size. I don't feel strongly here, and it's not unlikely the same thing will be flagged upstream - so let's do what you suggest.

@yuhaoth yuhaoth force-pushed the pr/fix-compile-errors-without-0rtt-mps branch from e6d7861 to 807f284 Compare August 16, 2021 06:04
@yuhaoth
Copy link
Collaborator Author

yuhaoth commented Aug 16, 2021

updated

@yuhaoth yuhaoth force-pushed the pr/fix-compile-errors-without-0rtt-mps branch 3 times, most recently from ace5f86 to b52af03 Compare September 4, 2021 12:12
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: Ibba188d13b15eb5cf1d5ecc7e9032b9044a98f22
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: Iec27c917a35a375adb16e82236e8abaaee00517a
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
change the state machine if early data is not
enabled

Change-Id: Iede5ab0dee6158110ac33976536117681d1d4a71
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: I015ae35a141a8b99352f5b90218fe919d7bbdc55
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: I6de197161d012960c391503db3e26db9b1342ff1
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: Ic8dd4967c9b11de41d2357a78303d9f6b03684a4
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
if all extensions are disabled, it reports
unused variable or parameter error

CustomizedGitHooks: yes
Change-Id: I74cffec7b0bb9d4e11d33414d27ad90758e630cf
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: I8fe177515d462f6b70e1c824ea75ffc4b5987a68
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
Move support groups, signature algorithm and key share
up. To make consistent with `development`

Change-Id: I44583b08d70e86a4af51c5f486c0f3617fc970c2
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: Ib002ea7cc783a649d44a26e9ff56e6dbd5280e73
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: Ic028e292504cff6f3621288bfd0043b18556ea77
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
Change-Id: I81e378d43a4a52aaa8d3c7bd7712b52b8249da01
CustomizedGitHooks: yes
Change-Id: I3c4bf28f23d596edb2d435174888b71254961bd7
CustomizedGitHooks: yes
Signed-off-by: Jerry Yu <[email protected]>
Signed-off-by: Jerry Yu <[email protected]>
@yuhaoth yuhaoth force-pushed the pr/fix-compile-errors-without-0rtt-mps branch from b52af03 to 29604d5 Compare September 4, 2021 13:49
@yuhaoth
Copy link
Collaborator Author

yuhaoth commented Sep 4, 2021

  • ssl-opt.sh test pass
  • And test with below script pass
./scripts/config.py unset MBEDTLS_SSL_USE_MPS
./scripts/config.py unset MBEDTLS_SSL_TLS13_COMPATIBILITY_MODE
./scripts/config.py unset MBEDTLS_ZERO_RTT
./scripts/config.py unset MBEDTLS_TLS13_EARLY_DATA
./scripts/config.py unset MBEDTLS_SSL_NEW_SESSION_TICKET
./scripts/config.py unset MBEDTLS_SSL_SERVER_NAME_INDICATION
./scripts/config.py unset MBEDTLS_KEY_EXCHANGE_PSK_ENABLED
./scripts/config.py unset MBEDTLS_KEY_EXCHANGE_RSA_PSK_ENABLED
./scripts/config.py unset MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED
./scripts/config.py unset MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED
./scripts/config.py unset MBEDTLS_SSL_COOKIE_C
./scripts/config.py unset MBEDTLS_HAVE_TIME
./scripts/config.py unset MBEDTLS_HAVE_TIME_DATE
make clean 
make -j20 CFLAGS="-g -Werror" 
./programs/ssl/ssl_server2 debug_level=2  nbio=0 | tee $1.server.log &
openssl s_server -www -cert data_files/server5.crt -key data_files/server5.key -tls1_3 -no_middlebox -num_tickets 0 -no_resume_ephemeral -no_cache -msg -accept 16624 &
gnutls-serv --x509certfile data_files/server5.crt --x509keyfile data_files/server5.key --priority=NORMAL:-VERS-ALL:+VERS-TLS1.3:+CIPHER-ALL:%NO_TICKETS:%DISABLE_TLS13_COMPAT_MODE --disable-client-cert -d 4 &
sleep 3 
./programs/ssl/ssl_client2  force_version=tls1_3  debug_level=3 nbio=0 | tee $1.client.log
killall ssl_server2

./programs/ssl/ssl_client2  force_version=tls1_3  debug_level=2 nbio=0 server_port=16624 | tee $1.openssl.client.log
./programs/ssl/ssl_client2  force_version=tls1_3  debug_level=2 nbio=0 server_port=5556 | tee $1.gnutls.client.log
killall openssl
killall gnutls-serv

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs work Some requests need addressing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants