search

saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
14.15k stars 5.48k forks source link

Add ability to configure SSL ciphers in salt-api with cherrypy #52981

Open jyriatntt opened 5 years ago

jyriatntt commented 5 years ago

Description of Issue/Question

No way to configure ssl version used in cherrypy / master config doesn't have an effect resulting insecure API channel, there is no documentation of this so I consider it security bug.

Setup

Ubuntu 18.04. LTS salt-api 2019.2.0+ds-1 salt-common 2019.2.0+ds-1 salt-master 2019.2.0+ds-1 python-cherrypy3 8.9.1-2 also tested with the newest CherryPy installed with pip

Steps to Reproduce Issue

Configure master to only support TLS v1.2

ssl:
    ssl_version: PROTOCOL_TLSv1_2

configure cherrypy salt-api.conf:

rest_cherrypy:
  port: 8000
  debug: false
  ssl_crt: /etc/pki/tls/certs/saltmaster.crt
  ssl_key: /etc/pki/tls/private/saltmaster.key
  webhook_disable_auth: false
  thread_pool: 100
  socket_queue_size: 30
  expire_responses: false
  max_request_body_size: 1048576
  collect_stats: false
  root_prefix: /
  rest_timeout: 7200

Restart master and salt-api and try if the new setting has effect:

$ systemctl restart salt-master
$ systemctl restart salt-api
$ curl -XGET --insecure -i --tlsv1.0 https://192.168.50.10:8000
HTTP/1.1 200 OK
Content-Length: 146
Access-Control-Expose-Headers: GET, POST
Vary: Accept-Encoding
Server: CherryPy/17.4.1
Allow: GET, HEAD, POST
Access-Control-Allow-Credentials: true
Date: Mon, 13 May 2019 12:58:32 GMT
Access-Control-Allow-Origin: *
Content-Type: application/json

{"clients": ["local", "local_async", "local_batch", "local_subset", "runner", "runner_async", "ssh", "wheel", "wheel_async"], "return": "Welcome"}

CherryPy happily answers to TLS v1.0 request

Versions Report

~$ salt --versions-report
Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: unknown
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: 2.0.3
      gitpython: 2.1.8
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: 1.0.7
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.15rc1 (default, Nov 12 2018, 14:31:15)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.3
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: Ubuntu 18.04 bionic
         locale: UTF-8
        machine: x86_64
        release: 4.15.0-29-generic
         system: Linux
        version: Ubuntu 18.04 bionic

Work to be done

garethgreenaway commented 5 years ago

@saltstack/team-core Thoughts?

waynew commented 5 years ago

Is this a problem with CherryPy, or are we just not setting some value?

jyriatntt commented 5 years ago

Not setting value, CherryPy uses external libraries for SSL support:

import cherrypy import OpenSSL.SSL as ssl context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2)

and so on.

jyriatntt commented 5 years ago

Unfortunately it wasn't as simple as just setting it up in salt/netapi/restcherrypy/__init_\.py in def start():

        if 'ssl_version'in apiopts.keys():
            cherrypy.server.ssl_version = 'ssl.' + apiopts['ssl_version']

I'm out of my depth in here. Anyone, ideas?

garethgreenaway commented 5 years ago

I'm still poking around at it but it appears what is needed is to create an SSL context and then add that context to the cherrypy server using cherrypy.server.ssl_context.

bennodepenno commented 4 years ago

Found similar behaviour on Debian 9.11 with salt 2019.2.2 version and python-cherrypy3 3.5.0-2 version.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

bennodepenno commented 4 years ago

Is there any idea out there how to solve this problem?

stale[bot] commented 4 years ago

Thank you for updating this issue. It is no longer marked as stale.

waynew commented 4 years ago

This should actually be much easier than that:

https://github.com/saltstack/salt/blob/master/salt/netapi/rest_cherrypy/__init__.py#L94

Right after that line add

cherrypy.server.ssl_ciphers = 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256'

That should give you the most advanced TLS ciphers.

At least, that seems to work with this sample script:

import cherrypy

class HeyDude:
    @cherrypy.expose
    def index(self):
        return "Salt Rocks!"

cherrypy.server.ssl_module = 'builtin'
cherrypy.server.ssl_certificate= '/Users/wwerner/snakeoil.crt'
cherrypy.server.ssl_private_key= '/Users/wwerner/snakeoil.key'
cherrypy.server.ssl_ciphers = 'TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256'
cherrypy.quickstart(HeyDude())

We do require tests on PRs, but in theory it should be pretty simple to add a test of this nature to the existing netapi tests.

I'd probably write the code fix like this:

if 'ssl_ciphers' in apiopts:
    cherrypy.server.ssl_ciphers = apiopts['ssl_ciphers']

Then the test would be written to verify that the enabled ciphers are, in fact, enabled, and that disabled ciphers are not available.

PaulWBrassard commented 4 years ago

Any chance we could get an ETA on this? We have some vulnerability scans that pick this up and opens new tickets each time so I'm hoping to lay out the timeline for our security team.

jyriatntt commented 4 years ago

Hi @waynew I don't think that that works as you think that it will work: ubuntu:~$ curl -XGET --insecure -i --tlsv1.0 https://localhost:8080 HTTP/1.1 200 OK Date: Mon, 10 Feb 2020 15:12:25 GMT Content-Length: 11 Content-Type: text/html;charset=utf-8 Server: CherryPy/8.9.1

Salt Rocks!

waynew commented 4 years ago

@jyriatntt thanks for the help! I couldn't find that invocation when I was searching for how to force curl to use TLS. I think the only thing I found specified ciphers to use? Anyway, I just did a bit of searching and this may be a limitation with CherryPy? At least I haven't found any instructions on how to make CherryPy disable tlsv1.0 - apparently you can specify the ciphers but maybe it doesn't care?

Oh... wait, no, I think what you suggested doesn't actually do what you thought it did?

* TCP_NODELAY set
* Connection failed
* connect to ::1 port 8080 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8080 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES256-GCM-SHA384
* ALPN, server did not agree to a protocol
* Server certificate:
*  subject: C=US; ST=AR; L=Maumelle; O=SaltStack open test; CN=localhost; emailAddress=wwerner@saltstack.com
*  start date: Jan 17 18:43:44 2020 GMT
*  expire date: Jan 16 18:43:44 2021 GMT
*  issuer: C=US; ST=AR; L=Maumelle; O=SaltStack open test; CN=localhost; emailAddress=wwerner@saltstack.com
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
> GET / HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.64.1
> Accept: */*
>
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Content-Type: text/html;charset=utf-8
Content-Type: text/html;charset=utf-8
< Server: CherryPy/18.5.0
Server: CherryPy/18.5.0
< Date: Thu, 27 Feb 2020 22:43:16 GMT
Date: Thu, 27 Feb 2020 22:43:16 GMT
< Content-Length: 11
Content-Length: 11

According to curl that doesn't look like it's actually making a tlsv1.0 connection?

jyriatntt commented 4 years ago

@waynew Hmm, what was the incantation you used with curl? As you can see from my test I got an answer with v1.0 using your sample script and modifications to the code.

waynew commented 4 years ago

curl -XGET --insecure -i --tlsv1.0 -v https://localhost:8080 produced the above output.

I believe I was using curl -k -v --ciphers NULL-MD5 https://localhost:8080, given my shell history.

Looks like I probably got that from http://openssl.cs.utah.edu/docs/apps/ciphers.html#tls_v1_0_cipher_suites_

waynew commented 4 years ago

Looking further, the cipher reported in my curl -v was absolutely listed in TLS 1.2, not TLS 1.0

jyriatntt commented 4 years ago

@waynew Yep;


# curl -v -i --insecure --tlsv1.0 https://127.0.0.1:8080
* Rebuilt URL to: https://127.0.0.1:8080/
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8080 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/certs/ca-certificates.crt
  CApath: /etc/ssl/certs
* TLSv1.0 (OUT), TLS handshake, Client hello (1):
* TLSv1.0 (IN), TLS handshake, Server hello (2):
* TLSv1.0 (IN), TLS handshake, Certificate (11):
* TLSv1.0 (IN), TLS handshake, Server key exchange (12):
* TLSv1.0 (IN), TLS handshake, Server finished (14):
* TLSv1.0 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.0 (OUT), TLS change cipher, Client hello (1):
* TLSv1.0 (OUT), TLS handshake, Finished (20):
* TLSv1.0 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.0 / ECDHE-RSA-AES256-SHA
* ALPN, server did not agree to a protocol
* Server certificate:
*  subject: C=ES; ST=Some-State; L=BCN; O=NTT; OU=Tech; CN=cherrrypy.test
*  start date: Feb  7 11:56:53 2020 GMT
*  expire date: Feb  6 11:56:53 2021 GMT
*  issuer: C=ES; ST=Some-State; L=BCN; O=NTT; OU=Tech; CN=cherrrypy.test
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
127.0.0.1 - - [06/Mar/2020:10:23:59] "GET / HTTP/1.1" 200 12 "" "curl/7.58.0"
> GET / HTTP/1.1
> Host: 127.0.0.1:8080
> User-Agent: curl/7.58.0
> Accept: */*
>
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< Date: Fri, 06 Mar 2020 09:23:59 GMT
Date: Fri, 06 Mar 2020 09:23:59 GMT
< Content-Length: 12
Content-Length: 12
< Content-Type: text/html;charset=utf-8
Content-Type: text/html;charset=utf-8
< Server: CherryPy/8.9.1
Server: CherryPy/8.9.1

<
Salt Rocks!
* Connection #0 to host 127.0.0.1 left intact

As it is I think you should revert the subject of the issue to the original :)

waynew commented 4 years ago

@jyriatntt is that with the patched code, or the existing cherrypy server?

jyriatntt commented 4 years ago

@waynew that is with your patch.

waynew commented 4 years ago

@jyriatntt can you try this Dockerfile?

FROM python:3.6-slim

RUN apt-get update && apt-get install -y curl
RUN python -m pip install cherrypy
RUN openssl req -new -newkey rsa:2048 -days 365 -nodes -x509 -keyout snakeoil.key -out snakeoil.crt -subj "/C=US/O=testing/CN=example.com"
RUN echo "import cherrypy\n\nclass HeyDude:\n @cherrypy.expose\n def index(self):\n  return 'Salt Rocks!'\n\ncherrypy.server.ssl_module = 'builtin'\ncherrypy.server.ssl_certificate='snakeoil.crt'\ncherrypy.server.ssl_private_key='snakeoil.key'\ncherrypy.server.ssl_ciphers='TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256:TLS_AES_128_GCM_SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256'\ncherrypy.server.socket_host='0.0.0.0'\ncherrypy.quickstart(HeyDude())" > /test.py
ENTRYPOINT ["python3", "/test.py"]

If you build the image with: docker build -t test . and then start it:

docker run --rm -it -p 8080:8080 test

Then in another terminal:

docker exec test curl -v -i --insecure --tlsv1.0 https://127.0.0.1:8080

This, for me, reports TLS v1.3. My host system reports TLS v1.2

What about for you?

oeuftete commented 4 years ago

ZD-5156.

waynew commented 4 years ago

As extra flavor, here's another curl that fails for me:

curl  --insecure -i --tlsv1.0 -v https://localhost:8000 --tls-max 1.0 
* About to connect() to localhost port 8000 (#0)
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8000 (#0)
* Initializing NSS with certpath: sql:/etc/pki/nssdb
* NSS error -12286 (SSL_ERROR_NO_CYPHER_OVERLAP)
* Cannot communicate securely with peer: no common encryption algorithm(s).
* Closing connection 0
curl: (35) Cannot communicate securely with peer: no common encryption algorithm(s).
oeuftete commented 4 years ago

@waynew's one-line fix in https://github.com/saltstack/salt/issues/52981#issuecomment-575768120 does work, but the catch is that support for the ssl_ciphers options was added in cherrypy in https://github.com/cherrypy/cherrypy/commit/0aa622f8ec930c95e45455acae810e23f3c47105, which is in version 10.2.2 and above.

@jyriatntt Your Server version in https://github.com/saltstack/salt/issues/52981#issuecomment-595680678 is CherryPy/8.9.1, which explains why the patch didn't work for you in blocking TLSv1.

waynew commented 4 years ago

What we need to do to resolve this issue, then, is to add support, probably api_ssl_ciphers as an option to the Salt config. I'm guessing that we can do something like map tlsv1.x to the appropriate cipher sets, or just use the ciphers as provided.

We should also document, probably in the default config, as well as in the Salt-API docs, that it's only supported in CherryPy>=10.2.2.

Did I miss anything?

jyriatntt commented 4 years ago

@oeuftete Thanks, that explains it. I did not upgrade to the latest CherryPy in my last test.

@waynew I think that it should be enough, though, it has to be clear to the end user that this cipher set will give you tlsv1.x and this will give you tlsv1.n, so maybe mapping would probably be the best.

waynew commented 4 years ago

Yeah - I'm thinking that documenting the mapping, but allowing people to override and specify whatever ciphers they'd like, is the most sensible approach. There are a number of weird dependencies out there that people have, and while I think we should provide safe/easy/reasonable defaults, if someone has some weird set or subset of ciphers that they require, who am I to argue? 🤷 All they would really have to do is set it up behind a reverse proxy to get a different behavior, so it's not like we would be protecting them against anything.

jlrcontegix commented 1 year ago

Has there been any movement on this? We are still getting dinged on scans for TLSv1 being accepted.