From 15841dfaae5be3218c6a552ee0d78fc1c24d063e Mon Sep 17 00:00:00 2001 From: Waldz Date: Mon, 17 Sep 2018 19:26:24 +0300 Subject: [PATCH 1/3] Rename openvpn_session.SessionManager -> openvpn_session.clientMap --- core/service/factory.go | 2 +- .../session/{manager.go => client_map.go} | 21 +++++++------------ .../session/manager_auth_middleware_test.go | 3 --- services/openvpn/session/validator.go | 14 ++++++------- services/openvpn/session/validator_mocks.go | 2 +- services/openvpn/session/validator_test.go | 6 +++--- 6 files changed, 20 insertions(+), 28 deletions(-) rename services/openvpn/session/{manager.go => client_map.go} (75%) diff --git a/core/service/factory.go b/core/service/factory.go index c81eac445..1ece3e01d 100644 --- a/core/service/factory.go +++ b/core/service/factory.go @@ -96,7 +96,7 @@ func NewManager( serviceOptions.OpenvpnProtocol, ) - ovpnSessionManager := openvpn_session.NewManager(manager) + ovpnSessionManager := openvpn_session.NewClientMap(manager) sessionValidator := openvpn_session.NewValidator(ovpnSessionManager, identity.NewExtractor()) return openvpn_node.NewServer( diff --git a/services/openvpn/session/manager.go b/services/openvpn/session/client_map.go similarity index 75% rename from services/openvpn/session/manager.go rename to services/openvpn/session/client_map.go index 2a4214504..d5f01071a 100644 --- a/services/openvpn/session/manager.go +++ b/services/openvpn/session/client_map.go @@ -21,32 +21,26 @@ import ( "errors" "sync" - "github.com/mysteriumnetwork/node/identity" "github.com/mysteriumnetwork/node/session" ) -// NewManager returns session manager which maintains a map of session id -> OpenVPN clientID -func NewManager(m session.Manager) *manager { - return &manager{ +// NewClientMap returns struct which maintains a map of session id -> OpenVPN clientID +func NewClientMap(m session.Manager) *clientMap { + return &clientMap{ sessionManager: m, sessionClientIDs: make(map[session.SessionID]int), sessionMapLock: sync.Mutex{}, } } -type manager struct { +type clientMap struct { sessionManager session.Manager sessionClientIDs map[session.SessionID]int sessionMapLock sync.Mutex } -// Create delegates session creation to underlying session.manager -func (manager *manager) Create(peerID identity.Identity) (session.Session, error) { - return manager.sessionManager.Create(peerID) -} - // FindSession returns OpenVPN session instance by given session id -func (manager *manager) FindSession(clientID int, id session.SessionID) (session.Session, bool, error) { +func (manager *clientMap) FindSession(clientID int, id session.SessionID) (session.Session, bool, error) { sessionInstance, foundSession := manager.sessionManager.FindSession(id) if !foundSession { @@ -63,7 +57,7 @@ func (manager *manager) FindSession(clientID int, id session.SessionID) (session } // UpdateSession updates OpenVPN session with clientID, returns false on clientID conflict -func (manager *manager) UpdateSession(clientID int, id session.SessionID) bool { +func (manager *clientMap) UpdateSession(clientID int, id session.SessionID) bool { manager.sessionMapLock.Lock() defer manager.sessionMapLock.Unlock() @@ -76,9 +70,10 @@ func (manager *manager) UpdateSession(clientID int, id session.SessionID) bool { } // RemoveSession removes given session from underlying session managers -func (manager *manager) RemoveSession(id session.SessionID) { +func (manager *clientMap) RemoveSession(id session.SessionID) { manager.sessionMapLock.Lock() defer manager.sessionMapLock.Unlock() + manager.sessionManager.RemoveSession(id) delete(manager.sessionClientIDs, id) } diff --git a/services/openvpn/session/manager_auth_middleware_test.go b/services/openvpn/session/manager_auth_middleware_test.go index 58740fe06..7a5f9eec7 100644 --- a/services/openvpn/session/manager_auth_middleware_test.go +++ b/services/openvpn/session/manager_auth_middleware_test.go @@ -22,8 +22,6 @@ import ( "github.com/mysteriumnetwork/node/openvpn/management" "github.com/mysteriumnetwork/node/openvpn/middlewares/server/auth" - "github.com/mysteriumnetwork/node/session" - "github.com/stretchr/testify/assert" ) @@ -32,7 +30,6 @@ type fakeAuthenticatorStub struct { password string called bool authenticated bool - manager session.Manager } func (f *fakeAuthenticatorStub) fakeAuthenticator(clientID int, username, password string) (bool, error) { diff --git a/services/openvpn/session/validator.go b/services/openvpn/session/validator.go index df77de070..9fd776253 100644 --- a/services/openvpn/session/validator.go +++ b/services/openvpn/session/validator.go @@ -29,14 +29,14 @@ const SignaturePrefix = "MystVpnSessionId:" // Validator structure that keeps attributes needed Validator operations type Validator struct { - sessionManager *manager + clientMap *clientMap identityExtractor identity.Extractor } // NewValidator return Validator instance -func NewValidator(m *manager, extractor identity.Extractor) *Validator { +func NewValidator(m *clientMap, extractor identity.Extractor) *Validator { return &Validator{ - sessionManager: m, + clientMap: m, identityExtractor: extractor, } } @@ -45,14 +45,14 @@ func NewValidator(m *manager, extractor identity.Extractor) *Validator { // it expects session id as username, and session signature signed by client as password func (v *Validator) Validate(clientID int, sessionString, signatureString string) (bool, error) { sessionID := session.SessionID(sessionString) - currentSession, found, err := v.sessionManager.FindSession(clientID, sessionID) + currentSession, found, err := v.clientMap.FindSession(clientID, sessionID) if err != nil { return false, err } if !found { - v.sessionManager.UpdateSession(clientID, sessionID) + v.clientMap.UpdateSession(clientID, sessionID) } signature := identity.SignatureBase64(signatureString) @@ -66,12 +66,12 @@ func (v *Validator) Validate(clientID int, sessionString, signatureString string // Cleanup removes session from underlying session managers func (v *Validator) Cleanup(sessionString string) error { sessionID := session.SessionID(sessionString) - _, found := v.sessionManager.sessionManager.FindSession(sessionID) + _, found := v.clientMap.sessionManager.FindSession(sessionID) if !found { return errors.New("no underlying session exists: " + sessionString) } - v.sessionManager.RemoveSession(sessionID) + v.clientMap.RemoveSession(sessionID) return nil } diff --git a/services/openvpn/session/validator_mocks.go b/services/openvpn/session/validator_mocks.go index 67d6ede26..655258e4c 100644 --- a/services/openvpn/session/validator_mocks.go +++ b/services/openvpn/session/validator_mocks.go @@ -34,7 +34,7 @@ type MockIdentityExtractor struct { OnExtractReturnError error } -// MockSessionManager mocked session manager +// MockSessionManager mocked session clientMap type MockSessionManager struct { OnFindReturnSession session.Session OnFindReturnSuccess bool diff --git a/services/openvpn/session/validator_test.go b/services/openvpn/session/validator_test.go index f72952b9d..c1d5de536 100644 --- a/services/openvpn/session/validator_test.go +++ b/services/openvpn/session/validator_test.go @@ -40,7 +40,7 @@ var mockExtractor = &MockIdentityExtractor{ nil, } -var fakeManager = NewManager(mockManager) +var fakeManager = NewClientMap(mockManager) var mockValidator = NewValidator(fakeManager, mockExtractor) @@ -54,7 +54,7 @@ func TestValidateReturnsFalseWhenNoSessionFound(t *testing.T) { }, ) - manager := &manager{sessionManager, make(map[session.SessionID]int), sync.Mutex{}} + manager := &clientMap{sessionManager, make(map[session.SessionID]int), sync.Mutex{}} mockValidator := &Validator{manager, mockExtractor} authenticated, err := mockValidator.Validate(1, "not important", "not important") @@ -113,7 +113,7 @@ func TestCleanupReturnsErrorIfSessionNotExists(t *testing.T) { identity.FromAddress("deadbeef"), nil, } - fakeManager := NewManager(mockManager) + fakeManager := NewClientMap(mockManager) mockValidator := NewValidator(fakeManager, mockExtractor) err := mockValidator.Cleanup("nonexistent_session") From 86c88593860382875093116a835c4875822f2509 Mon Sep 17 00:00:00 2001 From: Waldz Date: Mon, 17 Sep 2018 20:28:21 +0300 Subject: [PATCH 2/3] openvpn_session.Validator -> decoupled from SessionManager --- core/service/factory.go | 3 +- services/openvpn/session/client_map.go | 47 ++++++++++------------ services/openvpn/session/validator.go | 22 +++++----- services/openvpn/session/validator_test.go | 18 +++------ 4 files changed, 39 insertions(+), 51 deletions(-) diff --git a/core/service/factory.go b/core/service/factory.go index 1ece3e01d..e9d1d3a7e 100644 --- a/core/service/factory.go +++ b/core/service/factory.go @@ -96,8 +96,7 @@ func NewManager( serviceOptions.OpenvpnProtocol, ) - ovpnSessionManager := openvpn_session.NewClientMap(manager) - sessionValidator := openvpn_session.NewValidator(ovpnSessionManager, identity.NewExtractor()) + sessionValidator := openvpn_session.NewValidator(manager, identity.NewExtractor()) return openvpn_node.NewServer( nodeOptions.Openvpn.BinaryPath, diff --git a/services/openvpn/session/client_map.go b/services/openvpn/session/client_map.go index d5f01071a..22d2c9eb9 100644 --- a/services/openvpn/session/client_map.go +++ b/services/openvpn/session/client_map.go @@ -24,30 +24,21 @@ import ( "github.com/mysteriumnetwork/node/session" ) -// NewClientMap returns struct which maintains a map of session id -> OpenVPN clientID -func NewClientMap(m session.Manager) *clientMap { - return &clientMap{ - sessionManager: m, - sessionClientIDs: make(map[session.SessionID]int), - sessionMapLock: sync.Mutex{}, - } -} - type clientMap struct { sessionManager session.Manager sessionClientIDs map[session.SessionID]int sessionMapLock sync.Mutex } -// FindSession returns OpenVPN session instance by given session id -func (manager *clientMap) FindSession(clientID int, id session.SessionID) (session.Session, bool, error) { - sessionInstance, foundSession := manager.sessionManager.FindSession(id) +// FindClientSession returns OpenVPN session instance by given session id +func (cm *clientMap) FindClientSession(clientID int, id session.SessionID) (session.Session, bool, error) { + sessionInstance, foundSession := cm.sessionManager.FindSession(id) if !foundSession { return session.Session{}, false, errors.New("no underlying session exists, possible break-in attempt") } - sessionClientID, clientIDFound := manager.sessionClientIDs[id] + sessionClientID, clientIDFound := cm.sessionClientIDs[id] if clientIDFound && clientID != sessionClientID { return sessionInstance, false, errors.New("provided clientID does not mach active clientID") @@ -56,24 +47,30 @@ func (manager *clientMap) FindSession(clientID int, id session.SessionID) (sessi return sessionInstance, clientIDFound, nil } -// UpdateSession updates OpenVPN session with clientID, returns false on clientID conflict -func (manager *clientMap) UpdateSession(clientID int, id session.SessionID) bool { - manager.sessionMapLock.Lock() - defer manager.sessionMapLock.Unlock() +// UpdateClientSession updates OpenVPN session with clientID, returns false on clientID conflict +func (cm *clientMap) UpdateClientSession(clientID int, id session.SessionID) bool { + cm.sessionMapLock.Lock() + defer cm.sessionMapLock.Unlock() - _, foundClientID := manager.sessionClientIDs[id] + _, foundClientID := cm.sessionClientIDs[id] if !foundClientID { - manager.sessionClientIDs[id] = clientID + cm.sessionClientIDs[id] = clientID } - return manager.sessionClientIDs[id] == clientID + return cm.sessionClientIDs[id] == clientID } // RemoveSession removes given session from underlying session managers -func (manager *clientMap) RemoveSession(id session.SessionID) { - manager.sessionMapLock.Lock() - defer manager.sessionMapLock.Unlock() +func (cm *clientMap) RemoveSession(id session.SessionID) error { + cm.sessionMapLock.Lock() + defer cm.sessionMapLock.Unlock() + + _, found := cm.sessionManager.FindSession(id) + if !found { + return errors.New("no underlying session exists: " + string(id)) + } - manager.sessionManager.RemoveSession(id) - delete(manager.sessionClientIDs, id) + cm.sessionManager.RemoveSession(id) + delete(cm.sessionClientIDs, id) + return nil } diff --git a/services/openvpn/session/validator.go b/services/openvpn/session/validator.go index 9fd776253..04e864de4 100644 --- a/services/openvpn/session/validator.go +++ b/services/openvpn/session/validator.go @@ -18,7 +18,7 @@ package session import ( - "errors" + "sync" "github.com/mysteriumnetwork/node/identity" "github.com/mysteriumnetwork/node/session" @@ -34,9 +34,13 @@ type Validator struct { } // NewValidator return Validator instance -func NewValidator(m *clientMap, extractor identity.Extractor) *Validator { +func NewValidator(sessionManager session.Manager, extractor identity.Extractor) *Validator { return &Validator{ - clientMap: m, + clientMap: &clientMap{ + sessionManager: sessionManager, + sessionClientIDs: make(map[session.SessionID]int), + sessionMapLock: sync.Mutex{}, + }, identityExtractor: extractor, } } @@ -45,14 +49,14 @@ func NewValidator(m *clientMap, extractor identity.Extractor) *Validator { // it expects session id as username, and session signature signed by client as password func (v *Validator) Validate(clientID int, sessionString, signatureString string) (bool, error) { sessionID := session.SessionID(sessionString) - currentSession, found, err := v.clientMap.FindSession(clientID, sessionID) + currentSession, found, err := v.clientMap.FindClientSession(clientID, sessionID) if err != nil { return false, err } if !found { - v.clientMap.UpdateSession(clientID, sessionID) + v.clientMap.UpdateClientSession(clientID, sessionID) } signature := identity.SignatureBase64(signatureString) @@ -66,12 +70,6 @@ func (v *Validator) Validate(clientID int, sessionString, signatureString string // Cleanup removes session from underlying session managers func (v *Validator) Cleanup(sessionString string) error { sessionID := session.SessionID(sessionString) - _, found := v.clientMap.sessionManager.FindSession(sessionID) - if !found { - return errors.New("no underlying session exists: " + sessionString) - } - - v.clientMap.RemoveSession(sessionID) - return nil + return v.clientMap.RemoveSession(sessionID) } diff --git a/services/openvpn/session/validator_test.go b/services/openvpn/session/validator_test.go index c1d5de536..5ffdc5290 100644 --- a/services/openvpn/session/validator_test.go +++ b/services/openvpn/session/validator_test.go @@ -18,7 +18,6 @@ package session import ( - "sync" "testing" "github.com/mysteriumnetwork/node/identity" @@ -40,13 +39,10 @@ var mockExtractor = &MockIdentityExtractor{ nil, } -var fakeManager = NewClientMap(mockManager) - -var mockValidator = NewValidator(fakeManager, mockExtractor) +var mockValidator = NewValidator(mockManager, mockExtractor) func TestValidateReturnsFalseWhenNoSessionFound(t *testing.T) { mockExtractor := &MockIdentityExtractor{} - sessionManager := session.NewManager( mockedConfigProvider, &session.GeneratorFake{ @@ -54,8 +50,7 @@ func TestValidateReturnsFalseWhenNoSessionFound(t *testing.T) { }, ) - manager := &clientMap{sessionManager, make(map[session.SessionID]int), sync.Mutex{}} - mockValidator := &Validator{manager, mockExtractor} + mockValidator := NewValidator(sessionManager, mockExtractor) authenticated, err := mockValidator.Validate(1, "not important", "not important") assert.Errorf(t, err, "no underlying session exists, possible break-in attempt") @@ -68,8 +63,7 @@ func TestValidateReturnsFalseWhenSignatureIsInvalid(t *testing.T) { nil, } - mockValidator := &Validator{fakeManager, mockExtractor} - + mockValidator := NewValidator(mockManager, mockExtractor) authenticated, err := mockValidator.Validate(1, "not important", "not important") assert.NoError(t, err) @@ -102,7 +96,8 @@ func TestValidateReturnsTrueWhenSessionExistsAndSignatureIsValidAndClientIDMatch func TestCleanupReturnsNoErrorIfSessionIsCleared(t *testing.T) { mockValidator.Validate(1, "not important", "not important") err := mockValidator.Cleanup("not important") - _, found, _ := fakeManager.FindSession(1, "not important") + + _, found, _ := mockValidator.clientMap.FindClientSession(1, "not important") assert.False(t, found) assert.NoError(t, err) } @@ -113,9 +108,8 @@ func TestCleanupReturnsErrorIfSessionNotExists(t *testing.T) { identity.FromAddress("deadbeef"), nil, } - fakeManager := NewClientMap(mockManager) - mockValidator := NewValidator(fakeManager, mockExtractor) + mockValidator := NewValidator(mockManager, mockExtractor) err := mockValidator.Cleanup("nonexistent_session") assert.Errorf(t, err, "no underlying session exists: nonexistent_session") From f390693b9bafdaeb4aa3df0c3d0d4b58c65195ea Mon Sep 17 00:00:00 2001 From: Waldz Date: Tue, 18 Sep 2018 14:17:43 +0300 Subject: [PATCH 3/3] Improve naming --- services/openvpn/session/client_map.go | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/services/openvpn/session/client_map.go b/services/openvpn/session/client_map.go index 22d2c9eb9..b3258f9d1 100644 --- a/services/openvpn/session/client_map.go +++ b/services/openvpn/session/client_map.go @@ -32,19 +32,17 @@ type clientMap struct { // FindClientSession returns OpenVPN session instance by given session id func (cm *clientMap) FindClientSession(clientID int, id session.SessionID) (session.Session, bool, error) { - sessionInstance, foundSession := cm.sessionManager.FindSession(id) - - if !foundSession { + sessionInstance, sessionExist := cm.sessionManager.FindSession(id) + if !sessionExist { return session.Session{}, false, errors.New("no underlying session exists, possible break-in attempt") } - sessionClientID, clientIDFound := cm.sessionClientIDs[id] - - if clientIDFound && clientID != sessionClientID { + sessionClientID, clientIDExist := cm.sessionClientIDs[id] + if clientIDExist && clientID != sessionClientID { return sessionInstance, false, errors.New("provided clientID does not mach active clientID") } - return sessionInstance, clientIDFound, nil + return sessionInstance, clientIDExist, nil } // UpdateClientSession updates OpenVPN session with clientID, returns false on clientID conflict @@ -52,8 +50,8 @@ func (cm *clientMap) UpdateClientSession(clientID int, id session.SessionID) boo cm.sessionMapLock.Lock() defer cm.sessionMapLock.Unlock() - _, foundClientID := cm.sessionClientIDs[id] - if !foundClientID { + _, clientIDExist := cm.sessionClientIDs[id] + if !clientIDExist { cm.sessionClientIDs[id] = clientID } @@ -65,8 +63,8 @@ func (cm *clientMap) RemoveSession(id session.SessionID) error { cm.sessionMapLock.Lock() defer cm.sessionMapLock.Unlock() - _, found := cm.sessionManager.FindSession(id) - if !found { + _, clientIDExist := cm.sessionManager.FindSession(id) + if !clientIDExist { return errors.New("no underlying session exists: " + string(id)) }