Add acceptor support for receiving pre-loaded certificates #284
Conversation
Pull Request Test Coverage Report for Build 4862396700
💛 - Coveralls |
| keyFile = certs[1] | ||
| cert, err := tls.LoadX509KeyPair(certs[0], certs[1]) | ||
| if err != nil { | ||
| panic(constants.ErrInvalidCertificates) |
There was a problem hiding this comment.
It might be good to add a test validating this behavior.
There was a problem hiding this comment.
There was a problem hiding this comment.
These current tests only seem to assert the case when it panics when len(certs) is 0 or different than 2. I believe we need to add a new entry to the test table with two invalid entries in certs so it tries to load the certificate and then panics.
| @@ -117,13 +121,13 @@ func (a *TCPAcceptor) Stop() { | |||
| } | |||
|
|
|||
| func (a *TCPAcceptor) hasTLSCertificates() bool { | |||
| return a.certFile != "" && a.keyFile != "" | |||
| return len(a.certs) > 0 | |||
There was a problem hiding this comment.
Shouldn't this be == 2 ? Otherwise it will return true if it only has 1 cert or if it has 10
There was a problem hiding this comment.
If I recall correctly, I think that the TLS API supports multiple certificates. So in order to make the "new" tcp acceptor API fully functional, I left it as a more generic checking.
The constraint for 2 files is only related to the "legacy" API that loads the files itself. I think we can remove this altogether in the future, making the code much more generic.
What do you think?
acceptor/tcp_acceptor.go
Outdated
| return NewTCP(addr, certificates...) | ||
| } | ||
|
|
||
| func NewTCP(addr string, certs ...tls.Certificate) *TCPAcceptor { |
There was a problem hiding this comment.
This name seems fairly ambiguous, could we name it something like NewTLSAcceptor or something like that?
There was a problem hiding this comment.
That's a good question, I always struggle with naming in Golang.
I defined it as NewTCP so that it doesn't stutter when we have it on code like acceptor.NewTCP (instead of acceptor.NewTCPAcceptor). The more verbose option would also conflict with the previous existing definition.
Another option could be NewTCPWithCertificates.
What do you think?
There was a problem hiding this comment.
Given the current naming standards, I agree with Rafa on changing to NewTLSAcceptor
|
@felippeduran can you fix the conflicts? |
commit f363e48 Author: Felipe Cavalcanti <fjfcavalcanti@gmail.com> Date: Tue May 9 19:35:53 2023 -0300 fix for issues topfreegames#304 and topfreegames#303 commit c89065f Merge: 8c6c7bc a318fda Author: Reinaldo Oliveira <reinaldo.oliveira@wildlifestudios.com> Date: Tue May 2 13:47:00 2023 -0300 Merge pull request topfreegames#284 from topfreegames/feature/tls-certificates Add acceptor support for receiving pre-loaded certificates commit a318fda Author: Reinaldo Oliveira <reinaldo.oliveira@wildlifestudios.com> Date: Tue May 2 11:29:40 2023 -0300 Rename NewTCP to NewTLSAcceptor commit 1cc28ec Author: Reinaldo Oliveira <reinaldo.oliveira@wildlifestudios.com> Date: Thu Mar 23 15:04:35 2023 -0300 Add a testcase to TCP Acceptor function
commit f363e48 Author: Felipe Cavalcanti <fjfcavalcanti@gmail.com> Date: Tue May 9 19:35:53 2023 -0300 fix for issues topfreegames#304 and topfreegames#303 commit c89065f Merge: 8c6c7bc a318fda Author: Reinaldo Oliveira <reinaldo.oliveira@wildlifestudios.com> Date: Tue May 2 13:47:00 2023 -0300 Merge pull request topfreegames#284 from topfreegames/feature/tls-certificates Add acceptor support for receiving pre-loaded certificates commit a318fda Author: Reinaldo Oliveira <reinaldo.oliveira@wildlifestudios.com> Date: Tue May 2 11:29:40 2023 -0300 Rename NewTCP to NewTLSAcceptor commit 1cc28ec Author: Reinaldo Oliveira <reinaldo.oliveira@wildlifestudios.com> Date: Thu Mar 23 15:04:35 2023 -0300 Add a testcase to TCP Acceptor function
This MR exposes a new constructor for the TCP acceptor that allows for pre-loaded certificates as arguments.
It also refactors internal code in order to make sure the new constructor is used by the old one and make appropriate changes to store a list of pre-loaded certificates instead of a
certFile/certKeypair.Tests were changed accordingly.
You could say that there's a tiny API change, which is that the
NewTCPAcceptormight now panic if the provided certificate file paths are invalid, but it doesn't seem to me as something that relevant as its actually related to a failed initialization case where pitaya would panic somewhere else anyway (insideListenAndServeTLS).