search

myENA / consul-backinator

Command line Consul backup and restore utility supporting KVs, ACLs and Queries
Mozilla Public License 2.0
226 stars 22 forks source link

Add TLS Options for customizing connections to Consul #16

Closed KAllan357 closed 8 years ago

KAllan357 commented 8 years ago

Hello. I found this library yesterday and thought it would be really useful. Great work!

I have a need for supporting some Consul instances with self-signed certificates. I see that this issue was also recently opened today with the same need - https://github.com/myENA/consul-backinator/issues/15.

I've gone ahead and added the code but it remains untested. For now, I'd love to hear your feedback about my general approach and style choices (I attempted to mirror the style you've used throughout the code). I'll add a note / update this summary once I've tested to ensure this works as I expect.

Edit: I've tested these changes now locally using a Consul configured with some self-signed certs. See below.

Thanks!

aaronhurt commented 8 years ago

Thanks for the contribution. The request in #15 can be resolved using the existing CONSUL_HTTP_SSL_VERIFY environment variable. However, I do completely understand the point of being able to actually ensure self-signed certificates are enforced. I'll wait for your test results before merging. Thanks again.

KAllan357 commented 8 years ago

@leprechau I test this code this morning. I'm using a self-signed CA / certs with my Consul instance configured to load my certs.

The behavior is working as I'd expect.

For reference, I've copied the results of my tests here: https://gist.github.com/KAllan357/9f99421575b38fd787f22818bf90fa76.

aaronhurt commented 8 years ago

@KAllan357 Thank you!

aaronhurt commented 8 years ago

@KAllan357 I just added another commit to master that's using the 0.7.0 API as you pointed out in your comments. Would you mind testing to see if this still works for you?

KAllan357 commented 8 years ago

Sure, I think I could reduce some of the duplicated code using what's in the 0.7.0 API. Let me open another PR and test against it when I find some time.

aaronhurt commented 8 years ago

I made an attempt ... check 49feddf0f945afe7f7e87a07dbb2fe64a50a8ca1 and let me know if that works for you.

KAllan357 commented 8 years ago

Oh awesome! Let me pull and test this then. Be back in a few.

aaronhurt commented 8 years ago

Hrm. might hold off ... think I made a pretty big goof. Just got a nil pointer here when testing without the options.

aaronhurt commented 8 years ago

There ... that's better.

KAllan357 commented 8 years ago

I've tested the code in master and I can confirm that my tests no longer work like they used to. I will see if I can't figure out why.