scylladb / scylla-cqlsh

A fork of the cqlsh code
Apache License 2.0
11 stars 29 forks source link

Feature request: to allow turning off SSL server certificate hostname validation not turning off the certificate verification #77

Closed mark-bb closed 2 months ago

mark-bb commented 3 months ago

SSL server certificate hostname validation was turned off in previous releases, because ssl.PROTOCOL_TLS or ssl.PROTOCOL_TLSv1_2 were used previously, and there were no attempts to set the ssl_context.check_hostname property.
https://docs.python.org/3/library/ssl.html#ssl.SSLContext.check_hostname

The PROTOCOL_TLS_CLIENT protocol enables hostname checking by default. With other protocols, hostname checking must be enabled explicitly.

According to the link above (and test python program results) this means that ssl_context.check_hostname was turned off.
But the ssl.PROTOCOL_TLS_CLIENT is used in cqlsh shipped with 5.4, and ssl_context.check_hostname is set according to the validate option setting in the [ssl] section of cqlshrc.

This may break previous functionality, if I have a valid server certificate without an appropriate SAN section, and I don't want to turn off its verification with the validate=false cqlshrc setting.

The proposed solution is to introduce a new cqlshrc option check_hostname in the [ssl] section with an ability to use it as follows, for example:

[ssl]
...
validate = true
check_hostname = false

and make the following changes to /opt/scylladb/share/cassandra/libexec/../pylib/cqlshlib/sslhandling.py commenting out the 1-st original line and inserting the rest instead:

    # ssl_context.check_hostname = ssl_validate  # original line

    check_hostname_str = get_option('ssl', 'check_hostname')
    # Turn it off by default as in previous releases
    # check_hostname = check_hostname_str is not None and check_hostname_str.lower() != 'false'
    # Or set it to ssl_validate as currently by default
    check_hostname = ssl_validate if check_hostname_str is None else check_hostname_str.lower() != 'false'
    if check_hostname and not ssl_validate:
        sys.exit("SSL certificate hostname checking "
                 "(check_hostname in the [ssl] section) must be turned off "
                 "if certificate verification is turned off.")
    ssl_context.check_hostname = check_hostname
fruch commented 3 months ago

I've did in https://github.com/scylladb/scylla-cqlsh/pull/76/commits/7a1c84ed0aa8f9dcdad06abeb52eff1b06c31f90

I think it's kind of covers it.

mark-bb commented 3 months ago

@fruch I'm afraid, that it's a dangerous approach. We get "ValueError: Cannot set verify_mode to CERT_NONE when check_hostname is enabled." with just validate=false client setting only, which is allowed.

    ssl_check_hostname = ssl_check_hostname is None or ssl_check_hostname.lower() != 'false'

ssl_check_hostname gets True in case of absent check_hostname...

fruch commented 3 months ago

@fruch I'm afraid, that it's a dangerous approach. We get "ValueError: Cannot set verify_mode to CERT_NONE when check_hostname is enabled." with just validate=false client setting only, which is allowed.

    ssl_check_hostname = ssl_check_hostname is None or ssl_check_hostname.lower() != 'false'

ssl_check_hostname gets True in case of absent check_hostname...

yeah you have a point there, I've forgot they are connected (and that's why I've put them on the same flag)

mark-bb commented 3 months ago

@fruch

Is it possible to turn off the hostname verification by default as in previous releases? That is:

    # ssl_context.check_hostname = ssl_validate  # original line

    check_hostname_str = get_option('ssl', 'check_hostname')
    # Turn it off by default as in previous releases
    check_hostname = check_hostname_str is not None and check_hostname_str.lower() != 'false'
    # Or set it to ssl_validate as currently by default
    # check_hostname = ssl_validate if check_hostname_str is None else check_hostname_str.lower() != 'false'
    if check_hostname and not ssl_validate:
        sys.exit("SSL certificate hostname checking "
                 "(check_hostname in the [ssl] section) must be turned off "
                 "if certificate verification is turned off.")
    ssl_context.check_hostname = check_hostname
fruch commented 3 months ago

@fruch

Is it possible to turn off the hostname verification by default as in previous releases? That is:

    # ssl_context.check_hostname = ssl_validate  # original line

    check_hostname_str = get_option('ssl', 'check_hostname')
    # Turn it off by default as in previous releases
    check_hostname = check_hostname_str is not None and check_hostname_str.lower() != 'false'
    # Or set it to ssl_validate as currently by default
    # check_hostname = ssl_validate if check_hostname_str is None else check_hostname_str.lower() != 'false'
    if check_hostname and not ssl_validate:
        sys.exit("SSL certificate hostname checking "
                 "(check_hostname in the [ssl] section) must be turned off "
                 "if certificate verification is turned off.")
    ssl_context.check_hostname = check_hostname

We can consider it

mark-bb commented 3 months ago

We can consider it

It would be a preferable way of implementation. Thanks in advance!

mark-bb commented 3 months ago

@fruch Hello, is there any chance that resolution of this issue and #75 will be included in, say, 5.4.6 release?

mark-bb commented 3 months ago

@fruch Hello, is there any news on this? Thanks in advance.