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

INF-13413 #1712

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

INF-13413 #1712

wants to merge 5 commits into from

Conversation

nickhyoti
Copy link

Extended to support mTLS for connection between a reverse proxy and Athens

Enabled by adding TLSCACERTFILE to the configuration file.

What is the problem I am trying to address?

Enabling mTLS to harden network connectivity between a reverse proxy eg. nginx and Athens.

How is the fix applied?

Code as been extended adding a configuration option TLSCACERTFILE. If present, the http server will be configured to require the certificate from the reverse proxy on connection requestion

What GitHub issue(s) does this PR fix or close?

N/A

Extended to support mTLS for connection between a reverse proxy and Athens

Enabled by adding TLSCACERTFILE to the configuration file.
@nickhyoti nickhyoti requested a review from a team as a code owner April 20, 2021 13:34
@arschles
Copy link
Member

hey @nickhyoti - I took a glance at this and it looks reasonable. do you have a use case for it in your environment where you run Athens though?

Added additional environment to configure test version of mTLS support
Turned off proxy_ssl_verify pending certificate cleanup
@nickhyoti
Copy link
Author

We have put this forward as we want to

  1. Reduce the attack surface of the service. By mandating a connection through nginx reverse proxy (our test config), connections to Athens are restricted.
  2. Ensure connectivity is encrypted within the interconnecting components
  3. Eliminate the risk of incorrect interconnection of components

To support demonstration/testing, I have created a directory mtls which contains a docker-compose override to create a nginx container dependent on Athens dev container and also some test certificate directories.
The frontend directory certificates are deployed on the reverse proxy, the backend are used by the proxied connection to Athens.
Configuration files are provided to support this in the nginx/conf.d directory.

To enable and test, run
docker-compose -f docker-compose.yml -f mtls/mtls.yaml up --build

docker ps will show that the athens_dev_1 and nginx_1 containers are running,listening on ports 80 & 443 for nginx, and port 3000 for athens

Testing connections using curl to port 3000 on Athens will return
curl: (56) OpenSSL SSL_read: error:14094412:SSL routines:ssl3_read_bytes:sslv3 alert bad certificate, errno 0

To connect,
curl -k --cert web.cert.pem --key web.key.pem https://localhost:3000

Note that the certificates provided are only for demonstration purposes.

@nickhyoti
Copy link
Author

Hi,

Is there any feedback on this?

thanks

Copy link
Contributor

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

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

Hi. I have added some comments. In addition the mtls folder does not really belong in the project. I would be better added in the PR comments for testing purposes.

@@ -38,15 +41,37 @@ func main() {
log.Fatal(err)
}

cert, key, err := conf.TLSCertFiles()
cert, key, mtlsCert, err := conf.TLSCertFiles()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cert, key, mtlsCert, err := conf.TLSCertFiles()
cert, key, caCert, err := conf.TLSCertFiles()

It is not really an MTLS cert, but the CA cert.

Comment on lines +68 to +72
} else {
srv = &http.Server{
Addr: conf.Port,
Handler: handler,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This else is not needed if you initialise the server with these properties set. It also means that L63 can just be setting the server TLSConfig.

Addr: conf.Port,
Handler: handler,
if mtlsCert != "" {
clientCACert, err := ioutil.ReadFile(conf.TLSCACertFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

ioutil was depreciated in Go 1.16 and is not safe to use. Use os.ReadFile instead.

@@ -213,26 +214,36 @@ func (c *Config) BasicAuth() (user, pass string, ok bool) {

// TLSCertFiles returns certificate and key files and an error if
// both files doesn't exist and have approperiate file permissions
func (c *Config) TLSCertFiles() (cert, key string, err error) {
func (c *Config) TLSCertFiles() (cert, key string, cacert string, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (c *Config) TLSCertFiles() (cert, key string, cacert string, err error) {
func (c *Config) TLSCertFiles() (cert, key, cacert string, err error) {

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