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

Encrypt communication using self-signed certificate #169

Merged
merged 6 commits into from Aug 22, 2017

Conversation

tulios
Copy link
Contributor

@tulios tulios commented May 2, 2017

This PR fixes issue #168

It adds the ssl.ca option:

Kafka.Producer({
  connectionString: 'kafka://<something>'
  ssl: {
    ca: '/path/to/my-cert.crt' // or fs.readFileSync('my-cert.crt')
  }
});

It also accepts process.env.KAFKA_CLIENT_CA

@tulios
Copy link
Contributor Author

tulios commented May 2, 2017

There is one test failing in one of the builds which is not related to this PR

1) Compression sync should send/receive with Snappy compression (<32kb):
      AssertionError: expected 'p02' to deeply equal 'p00'
      + expected - actual
      -p02
      +p00

I don't have the permissions to re-run it so I can confirm that it was a random thing.

@oleksiyk
Copy link
Owner

oleksiyk commented May 3, 2017

I'm not sure what before and after blocks in the test do.
Also, why you are loading client.crt as the CA in the test?

@tulios
Copy link
Contributor Author

tulios commented May 3, 2017

The hooks are ensuring that KAFKA_CLIENT_CERT and KAFKA_CLIENT_CERT_KEY are not set since they are defined in the .travis.yml. Maybe they are not needed since you have builds without the configurations but it was to ensure that the tests always run with the correct setup. I can remove if you want.

About the client.crt I wasn't planning to test the tls module so I just needed something that would pass the /^-----BEGIN CERTIFICATE-----/ check. I can create a proper certificate if you want, kafka won't use it in the tests as this requires more configuration.

@oleksiyk
Copy link
Owner

oleksiyk commented May 3, 2017

The hooks are ensuring that KAFKA_CLIENT_CERT and KAFKA_CLIENT_CERT_KEY are not set

But you are not clearing them (the environment variables), you just save them in additional variables.

I can create a proper certificate if you want, kafka won't use it in the tests as this requires more configuration.

We should have the proper test here, Kafka server should send a certificate signed by self-signed CA (I guess it does this already) and the no-kafka test should have rejectUnauthorized set to true in order to validate server's certificate.

I can look into this and make a test a bit later.

@tulios
Copy link
Contributor Author

tulios commented May 3, 2017

But you are not clearing them

@oleksiyk right! I missed the cleanup while creating the PR, I wrote the tests without it and checked the travis.yml later. Sorry about that, I can fix it now.

I can look into this and make a test a bit later.

Great! I can take a look and see if I can setup this.

@tulios
Copy link
Contributor Author

tulios commented May 3, 2017

I tested this using docker before and it worked. Today I deployed to our staging environment and it is working fine.

@tulios
Copy link
Contributor Author

tulios commented Jun 7, 2017

@oleksiyk how can I help you to get this merged?

@oleksiyk
Copy link
Owner

oleksiyk commented Jun 9, 2017

Can you please update README.md to mention new SSL ca option and I'll merge it then.

@cdambo
Copy link

cdambo commented Aug 21, 2017

@tulios what's blocking us from merging this?

@tulios
Copy link
Contributor Author

tulios commented Aug 22, 2017

Hi, it's missing an update in the README. I completely forgot about this, I will do it today and see if we can get this merged. Thanks for the reminder 😃

@tulios
Copy link
Contributor Author

tulios commented Aug 22, 2017

@oleksiyk Can you review the PR again, I've updated the readme. Thanks

@oleksiyk oleksiyk merged commit 7e954f1 into oleksiyk:master Aug 22, 2017
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