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.

Sign up for GitHub

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

Jump to bottom

Add acceptor support for receiving pre-loaded certificates #284

Merged
merged 4 commits into from
May 2, 2023
Merged

Add acceptor support for receiving pre-loaded certificates #284

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a31a32a
Add acceptor support for receiving pre-loaded certificates
Aug 15, 2022
bfbb40d
Merge branch 'main' into feature/tls-certificates
reinaldooli Mar 22, 2023
1cc28ec
Add a testcase to TCP Acceptor function
reinaldooli Mar 23, 2023
a318fda
Rename NewTCP to NewTLSAcceptor
reinaldooli May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions acceptor/tcp_acceptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ type TCPAcceptor struct {
connChan chan PlayerConn
listener net.Listener
running bool
certFile string
keyFile string
certs []tls.Certificate
proxyProtocol bool
}

Expand Down Expand Up @@ -78,21 +77,26 @@ func (t *tcpPlayerConn) GetNextMessage() (b []byte, err error) {

// NewTCPAcceptor creates a new instance of tcp acceptor
func NewTCPAcceptor(addr string, certs ...string) *TCPAcceptor {
keyFile := ""
certFile := ""
certificates := []tls.Certificate{}
if len(certs) != 2 && len(certs) != 0 {
panic(constants.ErrInvalidCertificates)
} else if len(certs) == 2 {
certFile = certs[0]
keyFile = certs[1]
cert, err := tls.LoadX509KeyPair(certs[0], certs[1])
if err != nil {
panic(constants.ErrInvalidCertificates)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be good to add a test validating this behavior.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are some already: https://github.com/topfreegames/pitaya/pull/284/files#diff-91d8960c4bbf5e4ec59f1cc6232c15ea612244fef31e5a2e8c49edbd4c348997L52

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
certificates = append(certificates, cert)
}

return NewTCP(addr, certificates...)
}

func NewTCP(addr string, certs ...tls.Certificate) *TCPAcceptor {
reinaldooli marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@rsafonseca rsafonseca Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This name seems fairly ambiguous, could we name it something like NewTLSAcceptor or something like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the current naming standards, I agree with Rafa on changing to NewTLSAcceptor

return &TCPAcceptor{
addr: addr,
connChan: make(chan PlayerConn),
running: false,
certFile: certFile,
keyFile: keyFile,
certs: certs,
proxyProtocol: false,
}
}
Expand All @@ -117,13 +121,13 @@ func (a *TCPAcceptor) Stop() {
}

func (a *TCPAcceptor) hasTLSCertificates() bool {
return a.certFile != "" && a.keyFile != ""
return len(a.certs) > 0
Copy link
Contributor

@rsafonseca rsafonseca Apr 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't this be == 2 ? Otherwise it will return true if it only has 1 cert or if it has 10

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

}

// ListenAndServe using tcp acceptor
func (a *TCPAcceptor) ListenAndServe() {
if a.hasTLSCertificates() {
a.ListenAndServeTLS(a.certFile, a.keyFile)
a.listenAndServeTLS()
return
}

Expand All @@ -143,7 +147,14 @@ func (a *TCPAcceptor) ListenAndServeTLS(cert, key string) {
logger.Log.Fatalf("Failed to listen: %s", err.Error())
}

tlsCfg := &tls.Config{Certificates: []tls.Certificate{crt}}
a.certs = append(a.certs, crt)

a.listenAndServeTLS()
}

// ListenAndServeTLS listens using tls
func (a *TCPAcceptor) listenAndServeTLS() {
tlsCfg := &tls.Config{Certificates: a.certs}

listener, err := tls.Listen("tcp", a.addr, tlsCfg)
if err != nil {
Expand Down
9 changes: 4 additions & 5 deletions acceptor/tcp_acceptor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ var tcpAcceptorTables = []struct {
{"test_1", "0.0.0.0:0", []string{"./fixtures/server.crt", "./fixtures/server.key"}, nil},
{"test_2", "0.0.0.0:0", []string{}, nil},
{"test_3", "127.0.0.1:0", []string{"wqd"}, constants.ErrInvalidCertificates},
{"test_4", "127.0.0.1:0", []string{"wqd", "wqdqwd", "wqdqdqwd"}, constants.ErrInvalidCertificates},
{"test_4", "127.0.0.1:0", []string{"wqd", "wqdqwd"}, constants.ErrInvalidCertificates},
{"test_5", "127.0.0.1:0", []string{"wqd", "wqdqwd", "wqdqdqwd"}, constants.ErrInvalidCertificates},
}

func TestNewTCPAcceptorGetConnChanAndGetAddr(t *testing.T) {
Expand All @@ -58,11 +59,9 @@ func TestNewTCPAcceptorGetConnChanAndGetAddr(t *testing.T) {
})

if len(table.certs) == 2 {
assert.Equal(t, table.certs[0], a.certFile)
assert.Equal(t, table.certs[1], a.keyFile)
assert.Len(t, a.certs, 1)
} else {
assert.Equal(t, "", a.certFile)
assert.Equal(t, "", a.keyFile)
assert.Len(t, a.certs, 0)
}
assert.NotNil(t, a)
}
Expand Down