KAFKA-9460: Enable TLSv1.2 by default and disable all others protocol versions by nizhikov · Pull Request #7998 · apache/kafka

nizhikov · 2020-01-22T17:30:45Z

This PR by default disable all SSL protocols except TLSv1.2.
Changes discussed in KIP-553.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

… versions

nizhikov · 2020-01-22T17:32:36Z

Hello @rajinisivaram.
Can you, please, review my changes regarding KIP-553.

rondagostino · 2020-01-22T18:31:15Z

+            + "Allowed values in recent JVMs are TLS, TLSv1.1, TLSv1.2 and TLSv1.3. SSL, SSLv2 and SSLv3 "
            + "may be supported in older JVMs, but their usage is discouraged due to known security vulnerabilities.";

    public static final String DEFAULT_SSL_PROTOCOL = "TLS";


Does this need to change as well? Or does the enabled protocol list constrain it?

It seems, doc inconsistent for now.
We do support TLVv1.3.

see e275742

rajinisivaram

@nizhikov Thanks for the PR, left a couple of comments. Can we also add a test to verify the change?

nizhikov · 2020-01-22T19:38:56Z

@rajinisivaram

Can we also add a test to verify the change?

Can you, please, clarify, what should this new test check?
Just assert the value of the default SslConfig? Or something more complicated?

rajinisivaram · 2020-01-23T13:10:37Z

@nizhikov I think it is perhaps useful to add a test that uses the actual configs. Something similar to SslTransportLayerTest.testUnsupportedTLSVersion(), but where the protocol configs are the defaults. You could use TestSecurityConfig to ensure we are using defaults, since this is already used in SslTransportLayerTest for server-side configs.

nizhikov · 2020-01-27T13:52:50Z

@rajinisivaram

SslTransportLayerTest#testTLSDefaults added.
Please, take a look.

nizhikov · 2020-01-27T17:38:02Z

retest this please

rajinisivaram

@nizhikov Thanks for adding the test. Left a few comments.

nizhikov · 2020-01-28T13:58:57Z

@rajinisivaram Thanks for the feedback.

PR updated according to your comments.
Please, take a look.

rajinisivaram

@nizhikov Thanks for the updates, left a few more minor comments in the test. Apart from those, I think this is ready to go.

rajinisivaram · 2020-01-28T14:26:19Z


    /**
-     * Tests that connections cannot be made with unsupported TLS versions
+     * Tests that connection sucess with the default TLS version.


nit: typo success
Also mention that it tests that insecure protocols are not enabled by default.

nizhikov · 2020-01-28T14:38:38Z

@rajinisivaram PR updated according to your comments.

rajinisivaram

@nizhikov Thanks for the updates, LGTM. Will merge if the PR builds pass.

nizhikov · 2020-01-28T18:39:19Z

@rajinisivaram One of the build passed and other failed. It seems fail doesn't relate to these changes.

rajinisivaram · 2020-01-28T18:54:40Z

@nizhikov Yes, the test failure is unrelated (ConsumerBounceTest). Merging to trunk.

nizhikov · 2020-01-28T20:15:35Z

@rajinisivaram Thank you so much for the review and merge!

Conflicts and/or compiler errors due to the fact that we temporarily reverted the commit that removes Scala 2.11 support: * SslAdminIntegrationTest: keep using JAdminClient, take upstream changes otherwise. * ReassignPartitionsClusterTest: keep using JAdminClient, take upstream changes otherwise. * KafkaApis: use `asScala.foreach` instead of `forEach`. # By Ismael Juma (3) and others # Via GitHub * apache-github/trunk: (22 commits) KAFKA-9437; Make the Kafka Protocol Friendlier with L7 Proxies [KIP-559] (apache#7994) KAFKA-9375: Add names to all Connect threads (apache#7901) MINOR: Introduce 2.5-IV0 IBP (apache#8010) KAFKA-8503; Add default api timeout to AdminClient (KIP-533) (apache#8011) Add retries to release.py script (apache#8021) KAFKA-8162: IBM JDK Class not found error when handling SASL (apache#6524) MINOR: Add explicit result type in public defs/vals (apache#7993) KAFKA-9408: Use StandardCharsets.UTF-8 instead of "UTF-8" (apache#7940) KAFKA-9474: Adds 'float64' to the RPC protocol types (apache#8012) KAFKA-9360: Allow disabling MM2 heartbeat and checkpoint emissions (apache#7887) KAFKA-7658: Add KStream#toTable to the Streams DSL (apache#7985) KAFKA-9445: Allow adding changes to allow serving from a specific partition (apache#7984) KAFKA-9422: Track the set of topics a connector is using (KIP-558) (apache#8017) KAFKA-9040; Add --all option to config command (apache#7607) KAFKA-4203: Align broker default for max.message.bytes with Java producer default (apache#4154) KAFKA-9426: Use switch instead of chained if/else in OffsetsForLeaderEpochClient (apache#7959) KAFKA-9405: Use Map.computeIfAbsent where applicable (apache#7937) KAFKA-9026: Use automatic RPC generation in DescribeAcls (apache#7560) MINOR: Remove unused fields in StreamsMetricsImpl (apache#7992) KAFKA-9460: Enable only TLSv1.2 by default and disable other TLS protocol versions (KIP-553) (apache#7998) ...

…TLS protocol versions (KIP-553) (apache#7998)" This reverts commit 172409c

…able other TLS protocol versions (KIP-553) (apache#7998)" (#275) This reverts commit 172409c TLSv1.0 and TLSv1.1 will be disabled in CP 6.0 across the whole platform.

KAFKA-9460: Enable TLSv1.2 by default and disable all others protocol…

b2a4fac

… versions

rondagostino reviewed Jan 22, 2020

View reviewed changes

rajinisivaram reviewed Jan 22, 2020

View reviewed changes

Comment thread clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java Outdated

Comment thread clients/src/main/java/org/apache/kafka/common/config/SslConfigs.java Outdated

KAFKA-9460: Code review fixes.

491f8a3

nizhikov added 4 commits January 27, 2020 13:16

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-9460

35868d2

KAFKA-9460: Code review fixes.

db602b3

KAFKA-9460: Code review fixes.

a44b3fb

KAFKA-9460: Code review fixes.

3253500

rajinisivaram reviewed Jan 28, 2020

View reviewed changes

KAFKA-9460: Code review fixes.

f90f725

rajinisivaram reviewed Jan 28, 2020

View reviewed changes

KAFKA-9460: Code review fixes.

f2784b7

rajinisivaram reviewed Jan 28, 2020

View reviewed changes

Comment thread clients/src/test/java/org/apache/kafka/common/network/SslTransportLayerTest.java

KAFKA-9460: Code review fixes.

6c2051b

rajinisivaram approved these changes Jan 28, 2020

View reviewed changes

rajinisivaram merged commit 172409c into apache:trunk Jan 28, 2020

rajinisivaram added a commit to confluentinc/kafka that referenced this pull request Mar 2, 2020

Revert "KAFKA-9460: Enable only TLSv1.2 by default and disable other …

1b0d02c

…TLS protocol versions (KIP-553) (apache#7998)" This reverts commit 172409c

Conversation

nizhikov commented Jan 22, 2020

Committer Checklist (excluded from commit message)

Uh oh!

nizhikov commented Jan 22, 2020

Uh oh!

rondagostino Jan 22, 2020

Choose a reason for hiding this comment

Uh oh!

nizhikov Jan 22, 2020

Choose a reason for hiding this comment

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nizhikov commented Jan 22, 2020

Uh oh!

rajinisivaram commented Jan 23, 2020

Uh oh!

nizhikov commented Jan 27, 2020

Uh oh!

nizhikov commented Jan 27, 2020

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nizhikov commented Jan 28, 2020

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

rajinisivaram Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

nizhikov Jan 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nizhikov commented Jan 28, 2020

Uh oh!

Uh oh!

rajinisivaram left a comment

Choose a reason for hiding this comment

Uh oh!

nizhikov commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajinisivaram commented Jan 28, 2020

Uh oh!

nizhikov commented Jan 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nizhikov commented Jan 28, 2020 •

edited

Loading