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

feat: extended TLS configuration #259

Merged
merged 6 commits into from
Sep 23, 2019
Merged

feat: extended TLS configuration #259

merged 6 commits into from
Sep 23, 2019

Conversation

trutx
Copy link
Contributor

@trutx trutx commented Sep 5, 2019

This PR extends the server's TLS configuration. While the default TLS settings should be enough for most use cases, if you get a bit paranoid about security you'd probably want to tweak things a bit. This PR allows for that.

The motivation in my case is passing the FedRAMP certification, which required some TLS vulnerabilities to be addressed. Here's a FedRAMP compliant (as of Sep 5th, 2019) configuration example:

server:
  addr: :5001
  certificate: /path/to/server.pem
  key: /path/to/server.key
  hsts: true
  tls_min_version: 0x0303
  tls_curve_preferences:
    - 25
    - 24
    - 23
  tls_cipher_suites:
    - 0x009c
    - 0x009d
    - 0xc02f
    - 0xc02b
    - 0xc030
    - 0xc02c
    - 0xcca8
    - 0xcca9

If you're curious I used this tool to test both the current server and the patched one with the settings above, and the differences are there.

@rojer
Copy link
Collaborator

rojer commented Sep 9, 2019

no objections in general, but for readability, can we use string names for the ciphers and curve names instead?

@rojer
Copy link
Collaborator

rojer commented Sep 9, 2019

TLS_RSA_WITH_AES_128_GCM_SHA256 is much more readable than 0x009c
if there is no existing method to look up cipher suite number by name, you could use reflect to look up symbol by name in the tls package.

@trutx
Copy link
Contributor Author

trutx commented Sep 13, 2019

I'm not sure I understand what you mean by using reflect to lookup constants from a remote package. If it can be done I'd definitely love to learn how.

My goal was to keep the code simple and to avoid hardcoding (I guess that was the goal in https://github.com/cesanta/docker_auth/pull/232/files too). I see no way around either a hardcoded map to "translate" user provided strings to actual tls.CipherSuites or some sort of pre-build go generate that does a similar thing, but again I'd love to learn a new solution. I totally agree that the actual ciphersuite name is much more readable and user friendly than a hex value.

@rojer
Copy link
Collaborator

rojer commented Sep 13, 2019

ok, scratch that, you cannot use reflect to lookup package constants.

so, how about we support string representation for existing list of ciphers as map[string]uint16 but also allow numeric values.

that way:

tls_cipher_suites:
  - TLS_RSA_WITH_RC4_128_SHA
  - 0x0005

will both work. all you need to do is make that list of strings, look up value in the hard-coded map and then fall back to strconv.Atoi.

same applies to curve names.

@trutx
Copy link
Contributor Author

trutx commented Sep 20, 2019

That should do it. It was slightly trickier than strconv.Atoi because we're dealing with uint16 here. Now you can pass in either a string or an hex value to configure the TLS min version, the TLS Curves and the TLS CipherSuites.

Copy link
Collaborator

@rojer rojer left a comment

Choose a reason for hiding this comment

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

getting there, some minor comments

auth_server/main.go Outdated Show resolved Hide resolved
auth_server/server/config.go Outdated Show resolved Hide resolved
auth_server/server/config.go Outdated Show resolved Hide resolved
auth_server/main.go Outdated Show resolved Hide resolved
@trutx trutx requested a review from rojer September 23, 2019 14:10
@trutx
Copy link
Contributor Author

trutx commented Sep 23, 2019

Some validation logic could be added in config.go to check that all the passed values exist in the value maps, though this would mean doing the conversion dance twice -- one in server.validate() and another one in ServeOnce(). Not sure if it's worth it.

@rojer rojer merged commit 3fb13f1 into cesanta:master Sep 23, 2019
@rojer
Copy link
Collaborator

rojer commented Sep 23, 2019

merged, thank you!

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.

2 participants