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

DONTMERGE [common] Update ciphersuite used for securing pipe/UDS connections #1911

Closed
wants to merge 1 commit into from

Conversation

dimakuv
Copy link
Contributor

@dimakuv dimakuv commented Jun 17, 2024

Description of the changes

Previously, Gramine-SGX used MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256 ciphersuite when creating TLS-secured pipes and Unix Domain Sockets. This ciphersuite is considered deprecated, thus we switch to MBEDTLS_TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA256 (with Elliptic Curve DHE key exchange). This is recommended for Forward Security.

Unfortunately mbedTLS v3.6.0 doesn't provide an ECDHE PSK based ciphersuite that has AES-GCM, but only the one with AES-CBC. For the lack of options, we use the AES-CBC one.

DONTMERGE:

  • This hampers performance of applications that use pipes/UDSes a lot.
  • Serialization of TLS sessions (during fork) is broken because our patch only works for GCM mode (only checkpoints CTR counter).

There is an alternative PR that uses DHE: #1918

More info see also in Mbed-TLS/mbedtls#8170

How to test this PR?

CI is enough.

CI currently fails on forking apps, because serialization of TLS sessions doesn't work correctly. That's because of our minimalistic patch.


This change is Reviewable

…ections

Previously, Gramine-SGX used MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256
ciphersuite when creating TLS-secured pipes and Unix Domain Sockets.
This ciphersuite is considered deprecated, thus we switch to
MBEDTLS_TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA256 (with Elliptic Curve DHE
key exchange). This is recommended for Forward Security.

Unfortunately mbedTLS v3.6.0 doesn't provide an ECDHE PSK based
ciphersuite that has AES-GCM, but only the one with AES-CBC. For the
lack of options, we use the AES-CBC one.

DONTMERGE:
- This hampers performance of applications that use pipes/UDSes a lot.
- Serialization of TLS sessions (during fork) is broken because our
  patch only works for GCM mode (only checkpoints CTR counter).

Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "DONTMERGE" found in commit messages' one-liners

a discussion (no related file):
Also see this discussion: Mbed-TLS/mbedtls#8170


Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "DONTMERGE" found in commit messages' one-liners (waiting on @dimakuv)

a discussion (no related file):
A crypto expert from Intel tells me that mbedTLS seems to follow these two RFCs exactly (which also don't have PSK + ECDHE + GCM combination defined):

Since there's no ECDHE + GCM combination, the crypto expert recommends to use TLS_DHE_PSK_WITH_AES_128_GCM_SHA256. I.e. not ECDHE but DHE. This ciphersuite is available in mbedTLS and should be performant.


@dimakuv
Copy link
Contributor Author

dimakuv commented Jun 28, 2024

As explained by @mpg in #1918 (comment), there is no deprecation of the currently-used MBEDTLS_TLS_PSK_WITH_AES_128_GCM_SHA256. Closing this PR.

@dimakuv dimakuv closed this Jun 28, 2024
@dimakuv dimakuv deleted the dimakuv/change-mbedtls-key-exchange-to-ecdhe branch June 28, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant