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 ClientHello key share ext no curveSM2 when enable TLS1.3 + SM strictly #525

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Conversation

dongbeiouba
Copy link
Member

Fixed #522

Re-order curveSM2 to the first supported group when enable_sm_tls13_strict is set, so that the key_share extension will include a KeyShareEntry for the "curveSM2" group because only one KeyShareEntry is sent now.

Checklist
  • https://yuque.com/tsdoc 增加或更新了必要的文档
  • 增加或更新了必要的测试用例
  • 对于重要修改,更新了CHANGES文件
  • 当前修改存在对已有API参数或返回值的改变
  • 当前修改存在对旧版本功能的兼容性改变(如网络协议或密码算法)

@dongbeiouba dongbeiouba added bug Something isn't working master labels Nov 24, 2023
@FlyGoat
Copy link

FlyGoat commented Nov 24, 2023

IMO this is really hacky.
Clients without enable_sm_tls13_strict set won't be able to handshake with server that have enable_sm_tls13_strict set.
It creates a major interoperability barrier.
A proper implementation should send multiple KeyShareEntry at ClientHello.

@dongbeiouba
Copy link
Member Author

IMO this is really hacky. Clients without enable_sm_tls13_strict set won't be able to handshake with server that have enable_sm_tls13_strict set. It creates a major interoperability barrier. A proper implementation should send multiple KeyShareEntry at ClientHello.

  1. Clients without enable_sm_tls13_strict set won't be able to handshake with server that have enable_sm_tls13_strict set.

The test cases already cover this situation, Clients (enable_sm_tls13_strict = off) is able to handshake with server(enable_sm_tls13_strict = on). If you find any problems, please paste a test report.

2.It creates a major interoperability barrier.

As required by RFC8998: For the key_share extension, a KeyShareEntry for the "curveSM2" group MUST be included. So clients set enable_sm_tls13_strict include "curveSM2" both in supported_groups and key_share extensions.

Can you describe the interoperability problem in detail?

@zzl360
Copy link

zzl360 commented Nov 25, 2023

like https://rfc8998.badgmssl.com,If client support rfc8998 and send curveSM2 in key_share extension,the server will handshake using tls1.3+rfc8998.and if client doesn’t support rfc8998,the server will handshake with client using tls1.3 without rfc8998.
Another case,Image that client doesn't know whether server support rfc8998,If client only support one curve in key_share extension, there will no chance for client that send CurveSM2 handshak with server which don't support rfc8998 using tls1.3.

@InfoHunter
Copy link
Member

CIs are still failing

…+ SM strictly

Fixed #522

Re-order curveSM2 to the first supported group when enable_sm_tls13_strict is set,
so that the key_share extension will include a KeyShareEntry for the "curveSM2"
group because only one KeyShareEntry is sent now.
@dongbeiouba
Copy link
Member Author

CIs are still failing

Fixed.

@InfoHunter InfoHunter merged commit 8559a4c into Tongsuo-Project:master Nov 27, 2023
77 checks passed
@dongbeiouba dongbeiouba requested review from zzl360 and a team December 12, 2023 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC 8998 Violation: Client KeyShareEntry
5 participants