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 TLS infos to ClientAuth interface #385

Merged
merged 3 commits into from
Dec 22, 2016
Merged

Add TLS infos to ClientAuth interface #385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f94983a
Add TLS infos to ClientAuth interface
cdevienne Oct 31, 2016
872c9e0
Test GetTLSConnectionState
cdevienne Dec 21, 2016
6962964
Comment GetTLSConnectionState()
cdevienne Dec 22, 2016
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
6 changes: 6 additions & 0 deletions server/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

package server

import (
"crypto/tls"
)

// Auth is an interface for implementing authentication
type Auth interface {
// Check if a client is authorized to connect
Expand All @@ -12,6 +16,8 @@ type Auth interface {
type ClientAuth interface {
// Get options associated with a client
GetOpts() *clientOpts
// If TLS is enabled, TLS ConnectionState, nil otherwise
GetTLSConnectionState() *tls.ConnectionState
// Optionally map a user after auth.
RegisterUser(*User)
}
12 changes: 12 additions & 0 deletions server/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package server

import (
"bufio"
"crypto/tls"
"encoding/json"
"fmt"
"math/rand"
Expand Down Expand Up @@ -146,6 +147,17 @@ func (c *client) GetOpts() *clientOpts {
return &c.opts
}

// GetTLSConnectionState returns the TLS ConnectionState if TLS is enabled, nil
// otherwise. Implements the ClientAuth interface.
func (c *client) GetTLSConnectionState() *tls.ConnectionState {
tc, ok := c.nc.(*tls.Conn)
if !ok {
return nil
}
state := tc.ConnectionState()
Copy link
Member

Choose a reason for hiding this comment

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

Can be one line. Does ConnectionState need to be protected, how will it be used?

Copy link
Contributor Author

@cdevienne cdevienne Nov 28, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

If I make it a one-liner, I get:

# github.com/nats-io/gnatsd/server
server/client.go:155: cannot take the address of tc.ConnectionState()

By protected to you mean against concurrent accesses ?

I do not think so because it will be used for read-only operations, and the function is called early in the connection life-cycle, meaning no other routine will access the connection during the Check().

Moreover in the case we have (which is the general case imo) we need the ConnectionState only during the Check() in order to authenticate the remote user. Once it is done, we no longer need access to it.

At this point I fail to see a case were it needs protection.

Copy link
Member

Choose a reason for hiding this comment

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

Understood, but we do not know that it will be read only. I like the idea of wanting to use TLS as an auth mechanism, just think there may be a better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underlying function (https://golang.org/pkg/crypto/tls/#Conn.ConnectionState) already uses a lock during state preparation.

I think the ConnectionState, in the crypto/tls api, is meant for read-only operations, and modifying the slices in it may lead to serious problems, but nothing is done to avoid that.

Question is: should we protect something crypto/tls does not bother to protect?

Copy link
Member

Choose a reason for hiding this comment

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

You might raise a good point..

return &state
}

type subscription struct {
client *client
subject []byte
Expand Down
13 changes: 13 additions & 0 deletions server/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,14 @@ func TestClientCreateAndInfo(t *testing.T) {
}
}

func TestNonTLSConnectionState(t *testing.T) {
_, c, _ := setupClient()
state := c.GetTLSConnectionState()
if state != nil {
t.Error("GetTLSConnectionState() returned non-nil")
}
}

func TestClientConnect(t *testing.T) {
_, c, _ := setupClient()

Expand Down Expand Up @@ -713,6 +721,11 @@ func TestTLSCloseClientConnection(t *testing.T) {
if cli == nil {
t.Fatal("Did not register client on time")
}
// Test GetTLSConnectionState
state := cli.GetTLSConnectionState()
if state == nil {
t.Error("GetTLSConnectionState() returned nil")
}
// Fill the buffer. Need to send 1 byte at a time so that we timeout here
// the nc.Close() would block due to a write that can not complete.
done := false
Expand Down