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

Insecure cipher suites was found #6558

Closed
mitchell258 opened this issue Jun 10, 2022 · 4 comments · Fixed by #6562
Closed

Insecure cipher suites was found #6558

mitchell258 opened this issue Jun 10, 2022 · 4 comments · Fixed by #6562
Labels
needs info Needs further information from the user

Comments

@mitchell258
Copy link

Environment:

  • Dask version: 2022.2
  • Python version:3.7
  • Operating System: Linux
  • Install method (conda, pip, source): pip

What happened:
When tls_min_version is specified TLSv1_3, the list of cipher suites provided by Client Hello contains insecure cipher suites, such as TLS_RSA_W_AES_256_CBC_SHA256 .

Client Hello provides the following cipher suites:

            Cipher Suites (31 suites)
                Cipher Suite: TLS_AES_256_GCM_SHA384 (0x1302)
                Cipher Suite: TLS_CHACHA20_POLY1305_SHA256 (0x1303)
                Cipher Suite: TLS_AES_128_GCM_SHA256 (0x1301)
                Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 (0xc02c)
                Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030)
                Cipher Suite: TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 (0x009f)
                Cipher Suite: TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 (0xcca9)
                Cipher Suite: TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (0xcca8)
                Cipher Suite: TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (0xccaa)
                Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (0xc02b)
                Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
                Cipher Suite: TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 (0x009e)
                Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 (0xc024)
                Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (0xc028)
                Cipher Suite: TLS_DHE_RSA_WITH_AES_256_CBC_SHA256 (0x006b)
                Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 (0xc023)
                Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027)
                Cipher Suite: TLS_DHE_RSA_WITH_AES_128_CBC_SHA256 (0x0067)
                Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA (0xc00a)
                Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014)
                Cipher Suite: TLS_DHE_RSA_WITH_AES_256_CBC_SHA (0x0039)
                Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA (0xc009)
                Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013)
                Cipher Suite: TLS_DHE_RSA_WITH_AES_128_CBC_SHA (0x0033)
                Cipher Suite: TLS_RSA_WITH_AES_256_GCM_SHA384 (0x009d)
                Cipher Suite: TLS_RSA_WITH_AES_128_GCM_SHA256 (0x009c)
                Cipher Suite: TLS_RSA_WITH_AES_256_CBC_SHA256 (0x003d)
                Cipher Suite: TLS_RSA_WITH_AES_128_CBC_SHA256 (0x003c)
                Cipher Suite: TLS_RSA_WITH_AES_256_CBC_SHA (0x0035)
                Cipher Suite: TLS_RSA_WITH_AES_128_CBC_SHA (0x002f)
                Cipher Suite: TLS_EMPTY_RENEGOTIATION_INFO_SCSV (0x00ff)

What you expected to happen:
Leaving only the cipher suite supported by tls 1.3.

            Cipher Suites (4 suites)
                Cipher Suite: TLS_AES_256_GCM_SHA384 (0x1302)
                Cipher Suite: TLS_CHACHA20_POLY1305_SHA256 (0x1303)
                Cipher Suite: TLS_AES_128_GCM_SHA256 (0x1301)
                Cipher Suite: TLS_EMPTY_RENEGOTIATION_INFO_SCSV (0x00ff)

Anything else we need to know?:
Maybe we can disable insecure cipher suites by adding judgment statements?
Add a judgment to the next line of line 255.

ctx.minimum_version = self.tls_min_version

Such as,

            if self.tls_min_version is not None:
                ctx.minimum_version = self.tls_min_version

                if self.tls_min_version == ssl.TLSVersion.TLSv1_3:
                    ctx.options |= ssl.OP_NO_TLSv1_2
                
            if self.tls_max_version is not None:
                ctx.maximum_version = self.
@fjetter
Copy link
Member

fjetter commented Jun 10, 2022

Thanks for your report. Is there any way for us to reproduce this?

I was trying to reproduce this with the code below

import dask
from distributed.security import Security
from distributed.utils_test import get_cert
ca_file = get_cert("tls-ca-cert.pem")
cert1 = get_cert("tls-cert.pem")
key1 = get_cert("tls-key.pem")
keycert1 = get_cert("tls-key-cert.pem")

c = {
    "distributed.comm.tls.ca-file": ca_file,
    "distributed.comm.tls.scheduler.key": key1,
    "distributed.comm.tls.scheduler.cert": cert1,
    "distributed.comm.tls.worker.cert": keycert1,
    "distributed.comm.tls.min-version": 1.3,
}
dask.config.set(c)
sec = Security()
ciphers = sec.get_connection_args("scheduler")['ssl_context'].get_ciphers()
print({d['name'] for d in ciphers})

but did not get any insecure ciphers

cc @jcrist can you reproduce this or help out? xref #5594

@fjetter
Copy link
Member

fjetter commented Jun 10, 2022

I just noticed that you're running on python3.7 and an older version 2022.2. Can you try reproducing this on a more recent version? We're no longer supporting python3.7 and I would like to confirm this is not linked to the python version

@fjetter fjetter added needs info Needs further information from the user security labels Jun 10, 2022
graingert added a commit to graingert/distributed that referenced this issue Jun 10, 2022
graingert added a commit to graingert/distributed that referenced this issue Jun 10, 2022
graingert added a commit to graingert/distributed that referenced this issue Jun 13, 2022
graingert added a commit to graingert/distributed that referenced this issue Jun 13, 2022
graingert added a commit to graingert/distributed that referenced this issue Jun 14, 2022
@mitchell258
Copy link
Author

mitchell258 commented Jun 20, 2022

Thanks for your report. Is there any way for us to reproduce this?

I was trying to reproduce this with the code below

import dask
from distributed.security import Security
from distributed.utils_test import get_cert
ca_file = get_cert("tls-ca-cert.pem")
cert1 = get_cert("tls-cert.pem")
key1 = get_cert("tls-key.pem")
keycert1 = get_cert("tls-key-cert.pem")

c = {
    "distributed.comm.tls.ca-file": ca_file,
    "distributed.comm.tls.scheduler.key": key1,
    "distributed.comm.tls.scheduler.cert": cert1,
    "distributed.comm.tls.worker.cert": keycert1,
    "distributed.comm.tls.min-version": 1.3,
}
dask.config.set(c)
sec = Security()
ciphers = sec.get_connection_args("scheduler")['ssl_context'].get_ciphers()
print({d['name'] for d in ciphers})

but did not get any insecure ciphers

cc @jcrist can you reproduce this or help out? xref #5594

@fjetter Thank you for your reply!
I have found out the reason why insecure protocols are used. When I start the cluster using the command line, the configuration in the distributed.yaml file is read instead of the content configured for the client object.

Then when I changed comm.tls.min-version to 1.3 in distributed.yaml, the problem was solved...

Is there an option to specify the minimum version of the TLS protocol when starting with the command line?

tls:
  ciphers: null # Allowed ciphers, specified as an OpenSSL cipher string.
  min-version: 1.3 # 1.2 # The minimum TLS version supported.
  max-version: null # The maximum TLS version supported.

@fjetter
Copy link
Member

fjetter commented Jun 20, 2022

Well, thanks for your report anyhow. We discovered another problem with the minimal version.

Is there an option to specify the minimum version of the TLS protocol when starting with the command line?

There are a couple of ways to set this parameter. For instance, we're accepting multiple dask config files in multiple positions, e.g. you could put a config in your home dir with this option always set.

Have a look at https://docs.dask.org/en/stable/configuration.html for the various possibilities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs info Needs further information from the user
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants