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

Implement same client cert check feature #42

Merged
merged 1 commit into from
May 31, 2020

Conversation

mgusiew-guide
Copy link
Contributor

Implement same client cert check functionality as discussed in #37

Major changes:

  1. New flag --same-client-cert-enable that enables check set to false by default (backwards compatibility)
  2. Extend config validation to require TLS on both sides and client cert on Kafka connection when new flag is set
  3. Feature implementation in client that includes handshake (with timeout with same value as dial timeout), retrieving client certificate from proxy client connection and validating that it matches configured Kafka client cert
  4. Tests for config validation and flag handling (both success and failure scenarios)
  5. Include flag description and feature sample in readme file

@@ -63,3 +63,7 @@ Session.vim
*~
# Auto-generated tag files
tags

#IntelliJ

Choose a reason for hiding this comment

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

there is already existing IJ section at line 10. Merge your entries with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch ! Indeed this can be merged

README.md Show resolved Hide resolved
@@ -133,6 +133,7 @@ See:
--tls-client-key-password string Password to decrypt rsa private key
--tls-enable Whether or not to use TLS when connecting to the broker
--tls-insecure-skip-verify It controls whether a client verifies the server's certificate chain and host name
--same-client-cert-enable Use only when mutual TLS is enabled on proxy and broker. It controls whether a proxy validates if proxy client certificate matches brokers client cert (tls-client-cert-file)

Choose a reason for hiding this comment

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

should the flag start with "--tls"?

Choose a reason for hiding this comment

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

  • please change "matches" to "exactly matches" to avoid ambiguity.
  • reorder sentences:

Controls whether a proxy validates if proxy client certificate exactly matches brokers client cert (tls-client-cert-file). Use only when mutual TLS is enabled on proxy and broker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that, no problem

proxy/tls.go Show resolved Hide resolved
proxy/tls.go Show resolved Hide resolved
@mgusiew-guide
Copy link
Contributor Author

@mantkiewicz - thanks for comments, let's wait for @everesio review and then I will apply necessary changes in one go

@everesio
Copy link
Contributor

everesio commented May 28, 2020

thank you for the PR.
great job !

@everesio everesio merged commit 0f429f5 into grepplabs:master May 31, 2020
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.

None yet

3 participants