-
Notifications
You must be signed in to change notification settings - Fork 490
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
base: main
Are you sure you want to change the base?
INF-13413 #1712
Conversation
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? |
We have put this forward as we want to
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. To enable and test, run 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 To connect, Note that the certificates provided are only for demonstration purposes. |
Hi, Is there any feedback on this? thanks |
There was a problem hiding this 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cert, key, mtlsCert, err := conf.TLSCertFiles() | |
cert, key, caCert, err := conf.TLSCertFiles() |
It is not really an MTLS cert, but the CA cert.
} else { | ||
srv = &http.Server{ | ||
Addr: conf.Port, | ||
Handler: handler, | ||
} |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (c *Config) TLSCertFiles() (cert, key string, cacert string, err error) { | |
func (c *Config) TLSCertFiles() (cert, key, cacert string, err error) { |
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