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

fix: add back configs for setting TLS protocols and cipher suites #6558

Merged
merged 3 commits into from
Nov 4, 2020

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Nov 3, 2020

Description

Fixes #6555

Functionality of the configs ssl.enabled.protocols and ssl.cipher.suites were lost in the Jetty -> Vert.x migration. This PR adds back the functionality.

There were also some other configs that weren't migrated:

  • ssl.endpoint.identification.algorithm: I'm not entirely clear on the expected behavior of this config, particularly because it's a client config, not a server config. If configured, ksqlDB properly passes the config to outgoing clients such as Kafka admin clients, producers, consumers, etc. This PR adds support for the config in the ksqlDB CLI, for use when connecting to ksqlDB. Is the config also meant to apply to ksqlDB intra-node clients? Currently, hostname verification is configured for these intra-node requests if (and only if) intra-node authentication is enabled and TLS mutual auth is requested.
  • ssl.protocol, ssl.provider, ssl.keymanager.algorithm and ssl.trustmanager.algorithm: I don't see equivalents for these configs in Vert.x.

Open questions:

  • What should the default value for ssl.enabled.protocols be? Vert.x defaults to allowing {"TLSv1", "TLSv1.1", "TLSv1.2"}, which differs from Jetty which does not allow TLSv1 and TLSv1.1 by default. If we choose to go with the Jetty defaults, are we OK with this breaking change on a patch release (either 6.0.1 or 6.0.2, pending release schedule)? Also, how will we keep our default up to date as the Jetty default changes?
  • Relatedly, I don't think it makes sense to try to mirror the Jetty default for ssl.cipher.suites. I think the Vert.x default of delegating to the JVM default makes more sense.
  • What's the expected behavior of the ssl.endpoint.identification.algorithm config, discussed above?
  • Do we know of use cases for the configs mentioned above, that have yet to be migrated?

TODOs:

  • Docs update. This will be a separate PR so it is easier to cherry-pick across branches.
  • Backport this change to 6.0.x.

Testing done

Manual + integration test.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@rnpridgeon
Copy link

What should the default value for ssl.enabled.protocols be? Should we change the default for 6.0.1 and 6.02?

We should be defaulting to TLS v1.2 (RestUtils should change this default as well). I do not think its worth making a breaking change here though its worth noting we should be using TLSv1.2 in cloud already. If we aren't changing this should be less impactful than it was with Kafka but still warrants customer outreach.

https://github.com/confluentinc/cc-spec-kafka/blob/master/plugins/kafka/templates/serverConfig.tmpl#L69-L70

Relatedly, I don't think it makes sense to try to mirror the Jetty default for ssl.cipher.suites. I think the Vert.x default of delegating to the JVM default makes more sense.

That's seems reasonable as long as its configurable. You will want to be able to whitelist cipher suites for compliance reasons (FIPS).

What's the expected behavior of the ssl.endpoint.identification.algorithm config, discussed above?

The config ssl.endpoint.identification.algorithm=https tells the client to verify the certificate hostname of the target against the provided certificate. The client will first check for the hostname in the certificates subjectAltName extension and if necessary fallback to the CN(deprecated). This is relevant with or without mTLS so it should be performed even when mTLS is not enabled.

Do we know of use cases for the configs mentioned above, that have yet to be migrated?

Yes we need to be able to configure the keystore/truststore provider in order to support FIPS compliant JSSE providers; i.e. BCJSSE .

@AlanConfluent
Copy link
Member

  • What should the default value for ssl.enabled.protocols be? Vert.x defaults to allowing {"TLSv1", "TLSv1.1", "TLSv1.2"}, which differs from Jetty which does not allow TLSv1 and TLSv1.1 by default. If we choose to go with the Jetty defaults, are we OK with this breaking change on a patch release (either 6.0.1 or 6.0.2, pending release schedule)? Also, how will we keep our default up to date as the Jetty default changes?

I think that TLS 1 and 1.1 are not considered secure anymore. Just check out a Google search: https://www.google.com/search?q=tls+1+vs+1.1+security

In some sense, allowing them was a regression we discovered, so it's unclear if it's a breaking change or unbreaking change. Clearly it has the possibility of breaking people who relied on it in this window, though I think the number of clients not supporting 1.2 is small. It's hard to know the existing client breakdown, though there are many pages similar to https://help.okta.com/en/prod/Content/Topics/Miscellaneous/okta-ends-browser-support-for-TLS-1.1.htm where companies have disabled support for 1.1.

I think it wouldn't be bad to revert support for TLS 1.0 and 1.1.

@colinhicks
Copy link
Contributor

Should we consider pulling the default from org.apache.common.config.SslConfigs.DEFAULT_SSL_ENABLED_PROTOCOLS?

https://github.com/apache/kafka/blob/2.6/clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java#L76-L87

    public static final String DEFAULT_SSL_ENABLED_PROTOCOLS;

    static {
        if (Java.IS_JAVA11_COMPATIBLE) {
            DEFAULT_SSL_PROTOCOL = "TLSv1.3";
            DEFAULT_SSL_ENABLED_PROTOCOLS = "TLSv1.2,TLSv1.3";
        } else {
            DEFAULT_SSL_PROTOCOL = "TLSv1.2";
            DEFAULT_SSL_ENABLED_PROTOCOLS = "TLSv1.2";
        }
    }

@vcrfxia
Copy link
Contributor Author

vcrfxia commented Nov 4, 2020

Hey @rnpridgeon @AlanConfluent @colinhicks , thanks for the input!

For default TLS protocols, I think Colin's suggestion of using the Apache Kafka defaults starting in 6.1.x makes sense -- this way we don't have to worry about keeping up to date with Jetty, yet we still get reasonable consistency with the rest of the platform. I've updated this PR accordingly. For 6.0.x, I'm inclined to side with Ryan and say let's not making a breaking change on a point release -- with the patch in #6559, users with requirements to disable older versions can configure it themselves if desired. I disagree with the claim "In some sense, allowing them was a regression we discovered" since we never expressed desire to disallow older TLS versions starting in 6.0. (We discussed this internally but never took steps towards implementation, nor made any such public announcement AFAIA.)

The config ssl.endpoint.identification.algorithm=https tells the client to verify the certificate hostname of the target against the provided certificate.

This makes sense to me but I'm unclear which clients the config is meant to affect. Does it affect clients used to communicate with Kafka? With Schema Registry? With other dependencies? Clients for intra-node ksqlDB requests? All of the above?

Yes we need to be able to configure the keystore/truststore provider in order to support FIPS compliant JSSE providers; i.e. BCJSSE .

Sorry, I'm not quite following here. Which config(s) are necessary for doing so? If you could point me to example usage for other CP components, that'd be super helpful. Thanks.

Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vcrfxia vcrfxia mentioned this pull request Nov 4, 2020
2 tasks
@vcrfxia
Copy link
Contributor Author

vcrfxia commented Nov 4, 2020

Hey @rnpridgeon I'm going to merge this PR for now to unblock the ksqlDB 0.14 release cut, and will open a follow-up PR to address the usage of the ssl.endpoint.identification.algorithm config and any other missing configs we need to add functionality for once discussions there resolve. Thanks.

@vcrfxia vcrfxia merged commit cf7de69 into confluentinc:6.1.x Nov 4, 2020
@vcrfxia vcrfxia deleted the ssl-configs-6.1.x branch November 4, 2020 19:16
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.

5 participants