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

Add TLS options for http server #114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add TLS options for http server #114

wants to merge 1 commit into from

Conversation

gtowey
Copy link
Contributor

@gtowey gtowey commented Dec 16, 2019

This is a first attempt at adding TLS options to freno.

I've looked at the way SSL is initialized in Orchestrator here https://github.com/github/orchestrator/blob/master/go/app/http.go#L149-L161 and tried to follow that example but also do a bit of simplification as well.

I've added a second sample config file in conf to enable the tls options for testing so you can invoke freno with: ./freno -http -config conf/freno.conf.ssl.json -verbose

That also requires some self-signed certificates to be generated which is straight forward with:

openssl req -x509 -nodes -newkey rsa:2048 -keyout server.rsa.key -out server.rsa.crt -days 3650
ln -sf server.rsa.key server.key
ln -sf server.rsa.crt server.crt

There are a few questions I had about this change.

  1. I didn't include an option to set a certificate authority as part of the configuration -- what's the use case in Orchestrator and do you think it's necessary to include that option here?
  2. The code in Orchestrator takes some extra manual steps to load, parse and verify some of the certificate files, but I don't see many examples of other TLS configuration setups that take these extra steps. Are they needed here?

Also I think this PR isn't complete until it has additional documentation to explain how to configure the server for SSL operation as well.

/cc @shlomi-noach
/cc #112

@gtowey gtowey self-assigned this Dec 16, 2019
@shlomi-noach
Copy link

Apologies for the late review. This was submitted just as I was taking off for the Holidays and then just got buried below other notifications.

For a simplified TLS code, I think gh-ost would be a good example. orchestrator's code is somewhat more complex. The reasoning is its wide usage in the community, with everyone pulling to make it work on their own setup. The vast majority of TLS code in orchestrator was contributed by the community. I did refactor it here and there.

For now, I think we don't need an authority. We meanwhile use unsigned certificates. I do realize we will move to only allow signed certificates in MySQL, so entirely at your discretion whether to do everything now or take a simpler first step.

Copy link

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

If I read this correctly, this PR supports TLS/SSL in freno's HTTP API. This is great. I think our immediate need is to support TLS in the MySQL connections. We're moving towards encrypted mysql connections only, and freno is one of the tools in our infra to connect to MySQL without encryption. I suggest this is where should start with, and sincerely apologize for the confusion.

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

2 participants