Skip to content

docs: tighten TLS 1.3 + PQ guidance and align proto/docs layering#44950

Closed
Copilot wants to merge 4 commits into
mainfrom
copilot/update-tls-documentation-44944
Closed

docs: tighten TLS 1.3 + PQ guidance and align proto/docs layering#44950
Copilot wants to merge 4 commits into
mainfrom
copilot/update-tls-documentation-44944

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 8, 2026

This updates PR #44944 per review by removing repetitive/internals-heavy wording and keeping TLS behavior guidance at the right level: concise in proto comments, detailed in architecture docs. The result preserves the original scope (TLS defaults, TLS 1.3 cipher-suite handling, PQ guidance) without changing runtime behavior.

  • TLS 1.3 wording in TlsParameters.cipher_suites (proto)

    • Removed the repeated TLS 1.3 explanation under cipher_suites.
    • Kept proto comment brief and added a cross-reference to the higher-level TLS defaults section.
  • PQ guidance made operator-facing

    • Removed user-facing references to BoringSSL API functions (e.g. SSL_CTX_set1_curves_list).
    • Removed detailed causal/internal explanation for why PQ hybrid is off by default.
    • Kept only behavior/config guidance:
      • PQ hybrid key exchange is not enabled by default.
      • Enable via explicit ecdh_curves.
      • Observe negotiated group via %DOWNSTREAM_TLS_GROUP% / %UPSTREAM_TLS_GROUP%.
  • Detailed TLS 1.3 content kept in ssl.rst

    • Retained the fuller TLS 1.3 explanation and negotiable TLS 1.3 suite list in docs/root/intro/arch_overview/security/ssl.rst.
    • Simplified PQ section language to focus on configuration and observability.
  • Changelog/docs wording alignment

    • Updated docs changelog text to match the new operator-level framing and avoid implementation-internal phrasing.
    • Minor punctuation/style normalization in v2 TLS note text.
ecdh_curves:
  - X25519MLKEM768
  - X25519
  - P-256

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • mirror.bazel.build
    • Triggering command: /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/bin/java bazel(envoy) --add-opens=java.base/java.lang=ALL-UNNAMED -Xverify:none -Djava.util.logging.config.file=/build/bazel_root/base/javalog.properties -Dcom.google.devtools.build.lib.util.LogHandlerQuerier.class=com.google.devtools.build.lib.util.SimpleLogHandler$HandlerQuerier -XX:-MaxFDLimit -Djava.library.path=/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/embedded_tools/jdk/lib/server:/build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/ -Dfile.encoding=ISO-8859-1 -Duser.country= -Duser.language= -Duser.variant= -Xmx3g -DBAZEL_TRACK_SOURCE_DIRECTORIES=1 -Djavax.net.ssl.trustStore=/tmp/custom-cacerts -Djavax.net.ssl.trustStorePassword=changeit -jar /build/bazel_root/install/fb2a7f6d344d2f4e335882534df59296/A-server.jar --max_idle_secs=10800 --noshutdown_on_low_sys_mem --connect_timeout_secs=30 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

Update pull request #44944 in repository envoyproxy/envoy based on review feedback.

Context:

Requested changes from review:

  1. In api/envoy/extensions/transport_sockets/tls/v3/common.proto, the new TLS 1.3 note under cipher_suites is considered repetitive because the preceding line already says the field has no effect for TLS 1.3. Reduce or remove the repeated wording. Prefer moving the fuller explanation to the higher-level docs and, if useful, add only a brief cross-reference from the proto comment.
  2. Remove user-facing references to BoringSSL API functions such as SSL_CTX_set1_curves_list.
  3. Remove detailed causal explanations about why post-quantum hybrid key exchange is not enabled by default. The docs should stay at the operator/user level.
  4. Keep the documentation focused on behavior and configuration guidance:
    • PQ hybrid key exchange is not enabled by default.
    • Users can enable it by explicitly configuring ecdh_curves.
    • Include an example configuration.
    • Mention %DOWNSTREAM_TLS_GROUP% / %UPSTREAM_TLS_GROUP% as the way to observe the negotiated group.
  5. In docs/root/intro/arch_overview/security/ssl.rst, keep the fuller TLS 1.3 explanation and list of negotiable TLS 1.3 cipher suites, since that is the better location for the detail than the proto field comment.
  6. Update any changelog/docs wording as needed so it remains accurate after the above edits.

Implementation guidance:

  • Modify the existing PR branch rather than creating a new unrelated change.
  • Make the resulting text concise, user-facing, and aligned with the review comments.
  • Preserve the overall purpose of the PR: documenting TLS defaults, TLS 1.3 cipher suite handling, and PQ key exchange without changing runtime behavior.
  • Ensure the PR reflects the review feedback clearly so the blocking review can be resolved.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: we need to update thiks pr based on feedback

the one bit of pushback im wondering - where it says - this is repeated on the line above - they are separate fields - but then perhaps that is an argument to moving it up (and perhaps linking)

Assistant: Yes — I think your instinct is right: “they are separate fields” is true, but the reviewer’s point still stands.

Direct recommendation: move the fuller TLS 1.3 explanation to the higher-level ssl.rst section and keep the proto field comment minimal, with a cross-reference.

What the feedback is really saying:

  • In cipher_suites, you already say “this setting has no effect when negotiating TLS 1.3.”
  • The added note immediately below repeats that same idea in different words.
  • Even if the new note adds useful detail (“BoringSSL selects from its built-in set” and lists the suites), that detail may be better placed in the overview docs rather than inline in the field comment.

So I’d update the PR like this:

  1. api/.../v3/common.proto

    • Remove the new .. note:: block under cipher_suites.
    • Optionally replace it with a single short sentence or ref, e.g.:
      • “For TLS 1.3 cipher suite behavior, see :ref:arch_overview_ssl_tls_defaults.”
    • Keep the field focused on what cipher_suites controls.
  2. docs/root/intro/arch_overview/security/ssl.rst

    • Keep the fuller TLS 1.3 section there.
    • That’s the right place for:
      • TLS 1.3 suites not being configurable via cipher_suites
      • the list of negotiable TLS 1.3 suites
  3. PQ text

    • Reviewer feedback is stronger here: remove API-function details and “why”.
    • Rephrase toward operator guidance only:
      • not enabled by default
      • configure ecdh_curves explicitly to enable it
      • here is an example
      • use %DOWNSTREAM_TLS_GROUP% / %UPSTREAM_TLS_GROUP% to observe it
    • Same cleanup in both proto/docs, especially ssl.rst.

Based on the review, the concrete asks are:

  • Remove the repeated TLS 1.3 wording in cipher_suites
  • Remove references to SSL_CTX_set1_curves_list
  • Remove the causal/internal explanation about overriding BoringSSL defaults
  • Keep only user-facing behavior and config guidance

A good compromise for the proto comment is:

// This setting applies when negotiating TLS 1.0-1.2.
// For TLS 1.3 cipher suite behavior, see :ref:`arch_overview_ssl_tls_defaults`.

And for PQ something more like:

// .. warning::
//
//   Post-quantum hybrid key exchange is not enabled by default.
//   To enable it for TLS 1.3 connections, explicitly configure ``ecdh_curves``, for example:
//
//   .. code-block:: yaml
//
//     ecdh_curves:
//       - X25519MLKEM768
//       - X25519
//       - P-256
//
//   You can observe the negotiated group via ``%DOWNSTREAM_TLS_GROUP%`` and
//   ``%UPSTREAM_T...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created from Copilot chat.*
>

@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #44950 was opened by Copilot.

see: more, trace.

Copilot AI changed the title [WIP] Update TLS defaults and cipher suites documentation based on review feedback docs: tighten TLS 1.3 + PQ guidance and align proto/docs layering May 8, 2026
Copilot finished work on behalf of phlax May 8, 2026 16:27
Copilot AI requested a review from phlax May 8, 2026 16:27
@phlax phlax closed this May 8, 2026
@phlax phlax deleted the copilot/update-tls-documentation-44944 branch May 12, 2026 23:09
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.

2 participants