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

Introduce properties for cipher suites #12120

Closed
sbordet opened this issue Aug 1, 2024 · 6 comments · Fixed by #12126
Closed

Introduce properties for cipher suites #12120

sbordet opened this issue Aug 1, 2024 · 6 comments · Fixed by #12126
Labels
Bug For general bugs on Jetty side

Comments

@sbordet
Copy link
Contributor

sbordet commented Aug 1, 2024

Jetty version(s)
12

Description
jetty-ssl-context.xml has some configuration for SslContextFactory, but it is notably lacking configuration for e.g. cipher suites.

We should use StringUtil.csvSplit() like we do in other Jetty module XML files (see for example jetty-cross-origin.xml).

In this way, people would be able to configure cipher suites directly from properties in $JETTY_BASE/start.d/jetty-ssl-context.ini

@joakime
Copy link
Contributor

joakime commented Aug 1, 2024

We used to document how to do this with XML using "tweak" XML files.

See https://github.com/jetty/jetty.project/blob/jetty-9.4.55.v20240627/jetty-documentation/src/main/asciidoc/configuring/connectors/configuring-ssl.adoc


To do this, first create a new ${jetty.base}/etc/tweak-ssl.xml file (this can be any name, just avoid prefixing it with "jetty-").

<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN"
          "https://jetty.org/configure_9_3.dtd">
<!-- Tweak SsslContextFactory Includes / Excludes -->
<Configure id="sslContextFactory" class="org.eclipse.jetty.util.ssl.SslContextFactory$Server">
  <!-- Mitigate SLOTH Attack -->
  <Call name="addExcludeCipherSuites">
    <Arg>
      <Array type="String">
        <Item>.*_RSA_.*SHA1$</Item>
        <Item>.*_RSA_.*SHA$</Item>
        <Item>.*_RSA_.*MD5$</Item>
      </Array>
    </Arg>
  </Call>
</Configure>

@joakime
Copy link
Contributor

joakime commented Aug 1, 2024

We will need different configurations for SET vs ADD vs REMOVE.
We tried using CSV split in the past, but found the delims to be a problem with regex.

@sbordet
Copy link
Contributor Author

sbordet commented Aug 1, 2024

@joakime but in particular with cipher suite names, a comma should not be present, so regex should not be a problem, no?

We can document the old solution, but perhaps most of the time a simpler config with a property should be enough.

@joakime
Copy link
Contributor

joakime commented Aug 1, 2024

@joakime but in particular with cipher suite names, a comma should not be present, so regex should not be a problem, no?

Unless you are using a JVM that isn't based on OpenJDK or follows the OpenJDK conventions for cipher suite names.
We've come across many flavors of cipher suite names already. (openssl based, ibm legacy based, ibm modern based, various embedded systems, etc), commas and pipe symbols appear in a surprising number of places.

@sbordet
Copy link
Contributor Author

sbordet commented Aug 1, 2024

@joakime sure, but none of them have a comma in the name!

@joakime
Copy link
Contributor

joakime commented Aug 1, 2024

Yes, some do.
We have encountered cipher suite names like ECDHE-ECDSA,CHACHA20-POLY1305,SHA256 in some non-OpenJDK JVMs.

sbordet added a commit that referenced this issue Aug 1, 2024
Added documentation for advanced TLS configuration.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet linked a pull request Aug 1, 2024 that will close this issue
sbordet added a commit that referenced this issue Aug 2, 2024
Added documentation for advanced TLS configuration.

Updated the javadoc-url attribute to the new javadocs URI.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants