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

Add TLS infos to ClientAuth interface #385

Merged
merged 3 commits into from Dec 22, 2016

Conversation

Projects
None yet
5 participants
@cdevienne
Contributor

cdevienne commented Nov 28, 2016

It makes it possible to implement a Auth that uses client TLS certificates
to identify them.

@@ -12,6 +16,9 @@ type Auth interface {
type ClientAuth interface {
// Get options associated with a client
GetOpts() *clientOpts
// If TLS is enabled, return true and the TLS ConnectionState
// If not, return false
GetTLSConnectionState() (bool, tls.ConnectionState)

This comment has been minimized.

@derekcollison

derekcollison Nov 28, 2016

Member

Can you not just return a pointer, which can be nil if TLS is not enabled? Do we need to copy the struct via the return arg?

This comment has been minimized.

@cdevienne

cdevienne Nov 28, 2016

Contributor

We do not need the copy since the underlying call (GetConnectionState) already returns a copy.
Thanks for the suggestion!

@cdevienne cdevienne force-pushed the orus-io:clientauth-tls branch 2 times, most recently from 6c7f781 to 9b2fac2 Nov 28, 2016

if !ok {
return nil
}
state := tc.ConnectionState()

This comment has been minimized.

@derekcollison

derekcollison Nov 28, 2016

Member

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

This comment has been minimized.

@cdevienne

cdevienne Nov 28, 2016

Contributor

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.

This comment has been minimized.

@derekcollison

derekcollison Nov 28, 2016

Member

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.

This comment has been minimized.

@cdevienne

cdevienne Nov 28, 2016

Contributor

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?

This comment has been minimized.

@derekcollison

derekcollison Nov 28, 2016

Member

You might raise a good point..

@cdevienne

This comment has been minimized.

Contributor

cdevienne commented Nov 28, 2016

I would prefer not to hit the code coverage, but I felt a bit lost in the tests when I had a look.
Any advice on were I can insert something that will cover the new code ?

cdevienne added some commits Oct 31, 2016

Add TLS infos to ClientAuth interface
It makes it possible to implement a Auth that uses client TLS certificates
to identify them.
Test GetTLSConnectionState
Run GetTLSConnectionState on a non-tls connection (in a dedicated test)
and a tls connection.
Because initializing the TLS connection in the tests is non-trivial,

I hijacked the TestTLSCloseClientConnection test.

@cdevienne cdevienne force-pushed the orus-io:clientauth-tls branch from 9b2fac2 to 872c9e0 Dec 21, 2016

@cdevienne

This comment has been minimized.

Contributor

cdevienne commented Dec 21, 2016

@derekcollison I added tests that fully cover the new code of my PR.
I don't get why the coverage decreased.
Any chance the PR get merged ?

@kozlovic

This comment has been minimized.

Member

kozlovic commented Dec 22, 2016

@cdevienne I was asked to look at this PR. I agree that we don't need extra locking since tls.Conn.ConnectionState() internally uses its own lock. Furthermore, a copy of the state is returned, so even if the caller was to mess up with the returned state, it would not have an impact on the TLS connection.
The only note I would have is to add comment to GetTLSConnectionState since it is an exported method.

@cdevienne

This comment has been minimized.

Contributor

cdevienne commented Dec 22, 2016

Thanks for the review @kozlovic.
Totally agree the function needs a comment (in my own code I do not tolerate non-commented exported functions), I added it and updated the PR.

@coveralls

This comment has been minimized.

coveralls commented Dec 22, 2016

Coverage Status

Coverage increased (+0.2%) to 89.541% when pulling 6962964 on orus-io:clientauth-tls into 5dcad24 on nats-io:master.

@kozlovic

This comment has been minimized.

Member

kozlovic commented Dec 22, 2016

LGTM

@kozlovic kozlovic merged commit ad1804c into nats-io:master Dec 22, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 89.541%
Details

@cdevienne cdevienne deleted the orus-io:clientauth-tls branch Dec 23, 2016

@cdevienne cdevienne referenced this pull request Sep 5, 2017

Merged

Authorization cleanup #474

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment