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.

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

Feature/myst 497 improve session authorisation #248

Merged
merged 13 commits into from May 22, 2018

Conversation

Projects
None yet
3 participants
@zolia
Copy link
Member

commented May 18, 2018

  • prohibit second client with the same credentials
  • cleanup session after client disconnects
  • fixed bogus tests
  • added new test

@zolia zolia requested review from donce, tadovas and Waldz as code owners May 18, 2018

checkCredentials CredentialsChecker
commandWriter management.Connection
currentEvent clientEvent
credentialsValidator CredentialsChecker

This comment has been minimized.

Copy link
@donce

donce May 21, 2018

Contributor

So Validator and Checker is the same? Can we name them the same then?

)

var mockedVPNConfig = "config_string"

This comment has been minimized.

Copy link
@donce

donce May 21, 2018

Contributor

const

)


var mockedVPNConfig = "config_string"

This comment has been minimized.

Copy link
@donce

donce May 21, 2018

Contributor

const

@@ -39,3 +39,8 @@ func (manager *ManagerFake) Create(peerID identity.Identity) (Session, error) {
func (manager *ManagerFake) FindSession(id SessionID) (Session, bool) {
return Session{}, false
}

// RemoveSession removes given session from underlying session manager

This comment has been minimized.

Copy link
@donce

donce May 21, 2018

Contributor

This comment is false for this implementation - it doesn't remove anything. Comment should describe this method, not an interface method.

What about

// RemoveSession does nothing for this stub

In that case, // just stub comment wouldn't be needed anymore.

type manager struct {
sessionManager session.Manager
sessionClientIDMap map[session.SessionID]int
creationLock sync.Mutex

This comment has been minimized.

Copy link
@donce

donce May 21, 2018

Contributor

It's unaligned - have you run go fmt?

extractor := &extractor{}
signerID, err := extractor.Extract(message, signature)
assert.NoError(t, err)
assert.Exactly(t, originalSignerID, signerID, "Original signer should be extracted")

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

You are comparing two signerIDs here, error message should be "Signers should match"

This comment has been minimized.

Copy link
@donce

donce May 21, 2018

Contributor

To keep the detailed explanation and to specify failure, it could be something like this: Extracted signer should match original signer

// start enumerating clients from '1', since non-existing key, might return '0' as clientID value
clientID++
sessionInstance, found := manager.FindSession(id)
activeClientID := manager.sessionClientIDMap[id]

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

instead of expecting clientID as 0 if not found, use second return value from map return:
clientId, found := map[id] if !found do ...

)

// NewManager returns session manager which maintains a map of session id -> session
func NewManager(m session.Manager) *manager {

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

What is the purpose of this "session" manager inside openvpn package? we already have one.

This comment has been minimized.

Copy link
@zolia

zolia May 21, 2018

Author Member

Doc updated, this manager is kind of decorator for underlying session manager to account for openvpn clientID

}

// FindSession finds session and sets clientID if it is not set yet, returns false on clientID conflict
func (manager *manager) FindUpdateSession(clientID int, id session.SessionID) (session.Session, bool) {

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

Finds and Updates? Maybe this method does too much and needs to be split.

checkCredentials CredentialsChecker
commandWriter management.Connection
currentEvent clientEvent
credentialsValidator CredentialsChecker

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

I think that instead of having more and more depenencies on each use case (validating, cleaning) maybe an event channel could be a more generic solution, and middleware will have well defined purpose - generating events from incoming client messages through management connection and passing them up

This comment has been minimized.

Copy link
@zolia

zolia May 21, 2018

Author Member

I got the idea, but maybe its better to split this one into separate case.

type manager struct {
sessionManager session.Manager
sessionClientIDMap map[session.SessionID]int
creationLock sync.Mutex

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

Is this used anywhere? By the way this manager (map) is not thread-safe


// RemoveSession removes given session from underlying session managers
func (manager *manager) RemoveSession(id session.SessionID) {
manager.sessionManager.RemoveSession(id)

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

You need mutex locking here

This comment has been minimized.

Copy link
@zolia

zolia May 21, 2018

Author Member

set a lock in underlying RemoveSession function.

}

// Cleanup removes session from underlying session managers
func (v *Validator) Cleanup(sessionString string) error {

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

Cleanup is just callback from underlyging middleware, what caller has to do with returned error? Error handling (logging) can be performed here

This comment has been minimized.

Copy link
@zolia

zolia May 21, 2018

Author Member

Cleanup is called as callback from middleware.handleClientEvent, error is handled there..

This comment has been minimized.

Copy link
@tadovas

tadovas May 21, 2018

Member

That's my poiont. Why middleware which is lower level should handle cleanup errors? It doesn't know what to do with them anyway - except to log.

This comment has been minimized.

Copy link
@zolia

zolia May 21, 2018

Author Member

I think its important to be able to test for error returned by Cleanup callback, which can be nicely tested for in tests as in TestCleanupReturnsErrorIfSessionNotExists()

@zolia

This comment has been minimized.

Copy link
Member Author

commented May 21, 2018

remarks fixed..

@tadovas
Copy link
Member

left a comment

more or less LGTM

// TODO: consider implementing event channel to communicate required callbacks
credentialsValidator CredentialsValidator
sessionCleaner SessionCleaner
commandWriter management.Connection

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

Not related to this PR, but this variable and class naming difference bugs me - why not call interface management.CommandWriter instead of management.Connection? Isn't Connection just a detail of implementation?

// NewMiddleware creates server user_auth challenge authentication middleware
func NewMiddleware(credentialsChecker CredentialsChecker) *middleware {
func NewMiddleware(credentialsChecker CredentialsValidator, cleaner SessionCleaner) *middleware {

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

credentialsChecker -> credentialsValidator


// SignatureCredentialsProvider returns session id as username and id signed with given signer as password
func SignatureCredentialsProvider(id session.SessionID, signer identity.Signer) auth.CredentialsProvider {
return func() (string, string, error) {

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

List of return values is hard to understand, especially with 2 unnamed strings. You have to jump to auth.CredentialsProvider to understand what this function returns.

Naming return values solves this:

	return func() (username string, password string, err error) {
		signature, err := signer.Sign([]byte(ovpnsession.SignaturePrefix + id))
		return string(id), signature.Base64(), err
	}
}

// Create delegates session creation to underlying session.manager
func (manager *manager) Create(peerID identity.Identity) (sessionInstance session.Session, err error) {

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

No need to name return values here, they are pretty obvious and not used.

return &Validator{
sessionManager: m,
identityExtractor: extractor,
}

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

Very strange indentation.

return session.Session{}, false, errors.New("no underlying session exists, possible break-in attempt")
}

activeClientID, foundClientID := manager.sessionClientIDMap[id]

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

Naming is very confusing - activeClientID and foundClientID sounds like 2 differents client IDs, one which is active and another one which was found.
Maybe sessionClientID and clientIDFound is more readable?


activeClientID, foundClientID := manager.sessionClientIDMap[id]

if foundClientID && clientID != activeClientID {

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

This condition is not very readable - it looks like 3 different client IDs are here. This comment about foundClientID would fix this.

manager.sessionMapLock.Lock()
defer manager.sessionMapLock.Unlock()

_, foundClientID := manager.sessionClientIDMap[id]

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

Do we really need to have Map in variable name? sessionClientIDs[id] looks quite readable as well for me.
I.e. we don't name arrays with Array prefix, as in SessionArray


func provideMockedVPNConfig() string {
return mockedVPNConfig
}

This comment has been minimized.

Copy link
@donce

donce May 22, 2018

Contributor

Why do we need this function, can't we take const directly?

This comment has been minimized.

Copy link
@zolia

zolia May 22, 2018

Author Member

We cannot use plain string here since 'session.NewManager()' requires function as an argument where it is used in TestValidateReturnsFalseWhenNoSessionFound()

@donce

donce approved these changes May 22, 2018

@zolia zolia merged commit 46f7a43 into master May 22, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@zolia zolia deleted the feature/MYST-497-improve-session-authorisation branch May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.