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-259 validate session and signature on service provider side #126

Conversation

Projects
None yet
4 participants
@tadovas
Copy link
Member

commented Jan 30, 2018

No description provided.

type ManagerFake struct{}

func (manager *ManagerFake) Create() (Session, error) {
return Session{"new-id", "new-config"}, nil
//Create function creates and returns fake session

This comment has been minimized.

Copy link
@donce

donce Jan 30, 2018

Contributor

Most of comments for public objects seem to include space after //, that is // Create... - this seems more readable.

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

This is also in all effective-go examples: https://golang.org/doc/effective_go.html#commentary

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Fixing wherever I see it.

}

func (dialog *dialog) Close() error {
return nil
}

func (dialog *dialog) GetPeerID() identity.Identity {

This comment has been minimized.

Copy link
@zolia

zolia Jan 30, 2018

Member

Usual go convention is not to use get, just PeerID()

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Fixed

"github.com/mysterium/node/session"
)

const sessionSignaturePrefix = "SessionId:"

This comment has been minimized.

Copy link
@zolia

zolia Jan 30, 2018

Member

IMHO too generic, I would use word 'Myst' somewhere.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

-> "MystVpnSessionId:". It's just for signing/validating session/signature anyway.

}

func (manager *manager) Create() (sessionInstance session.Session, err error) {
func (manager *manager) Create(peerId identity.Identity) (sessionInstance session.Session, err error) {
manager.creationLock.Lock()

This comment has been minimized.

Copy link
@zolia

zolia Jan 30, 2018

Member

why do we need this Lock?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 30, 2018

Author Member

Because session manager method "create" can be accessed concurrently by multiple peer dialogs and map type is not thread safe in golang


//NewSignedSessionIdCredentialsProvider returns session id as username and id signed with given signer as password
func NewSignedSessionIdCredentialsProvider(id session.SessionID, signer identity.Signer) auth.CredentialsProvider {
signature, err := signer.Sign([]byte(sessionSignaturePrefix + id))

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 31, 2018

Member

Strange that factory does some work.

  • error is omitted

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

Use of factory looks very artificial here. What if we updated this method to return (auth.CredentialsProvider, error)? Then we could handle error immediately without propagating error more deeply, which makes it harder to track it down.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

That's a design of CredentialsProvider which expects error if it's not possible to get credentials from somewhere. Moved signing to function itself

statsHandler := bytescount.NewCompositeStatsHandler(statsSaver, statsSender)

authenticator := auth.NewAuthenticatorFake()
credentialsProvider := openvpnSession.NewSignedSessionIdCredentialsProvider(vpnSession.ID, signer)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 31, 2018

Member

Could be something shorter :)

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Renamed to SignatureCredentialsProvider as we are already in session package

return false, nil
}

verifier := sa.createVerifier(currentSession.ConsumerIdentity)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 31, 2018

Member

createVerifier looks like we'are overengineering. Factory + DI, just to call 1 if condition

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Verifier removed :) left extractor and extractedIdentity == PeerIdentity

return session.Session{
ID: session.SessionID("fake-id"),
Config: "vpn-session-configuration-string",
ConsumerIdentity: identity.FromAddress("deadbeef"),

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 31, 2018

Member

ConsumerID, identity is package name

sessionInstance.ID = manager.idGenerator.Generate()

sessionInstance.ConsumerIdentity = peerId

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 31, 2018

Member

peerID

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Renamed

type ManagerFake struct{}

func (manager *ManagerFake) Create() (Session, error) {
return Session{"new-id", "new-config"}, nil
//Create function creates and returns fake session

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

This is also in all effective-go examples: https://golang.org/doc/effective_go.html#commentary


//NewSignedSessionIdCredentialsProvider returns session id as username and id signed with given signer as password
func NewSignedSessionIdCredentialsProvider(id session.SessionID, signer identity.Signer) auth.CredentialsProvider {
signature, err := signer.Sign([]byte(sessionSignaturePrefix + id))

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

Use of factory looks very artificial here. What if we updated this method to return (auth.CredentialsProvider, error)? Then we could handle error immediately without propagating error more deeply, which makes it harder to track it down.

}

//ValidateSession validates provided session id and signature, by retrieving peer identity and checking session signature
func (sa *sessionAuthenticator) ValidateSession(sessionString, signatureString string) (bool, error) {

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

sessionAuthenticator struct has one method - ValidateSession.
We could simplify this into function type:

type SessionValidator func(sessionString, signatureString string) (bool, error)
func NewSessionValidator(sessionLookup sessionLookupFunc, verifierCreator verifierFactory) SessionValidator {
...
}

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

FunctionFactory which returns function. Fixed

@@ -7,25 +7,28 @@ import (
"regexp"
)

// CredentialsProvider returns client's current auth primitives (i.e. customer identity signature / node's sessionId)
type CredentialsProvider func() (username string, password string, err error)

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

username, password pair is strongly coupled - it's being passed around the system in many places and we have quite a few methods which returns (username, password string, err error).
Maybe it's time to merge them into struct?

type Credentials struct {
    username, password string
}

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

I.e. we wouldn't have to explain what is "auth primitives" in many places, as we do now - we could do that in one place.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Yeah that's true. but since our middleware packageing is a bit different, we don't have a possibility to share credentials structure between client and server. But good point and it can be improved with separate PR.

lastUsername string
lastPassword string
state openvpn.State
credentials CredentialsProvider

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

credentials name is a bit confusing - you don't have credentials here, but you have a function which can (in some way) get you credentials. But not always, because it can return you an error as well!

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

renamed to fetchCredentials

vpnMiddlewares := []openvpn.ManagementMiddleware{
bytescount.NewMiddleware(statsHandler, 1*time.Minute),
auth.NewMiddleware(authenticator),
auth.NewMiddleware(credentialsProvider),

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

Might be not related to this PR, but it's extremely confusing that auth.NewMiddleware can mean two different functions, depending whether you import server or client middleware.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

We will rethink this packaging later

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2018

@Waldz @donce @zolia comments reviewed/fixed/replied

clientConfig *openvpn.ClientConfig
sessions []session.SessionID
sessionMap map[session.SessionID]session.Session
creationLock *sync.Mutex

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

This mutex doesn't have to be pointer - it can be just sync.Mutex.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Fixed

"testing"
)

var returnSessionNotFound = func(sessionId session.SessionID) (session.Session, bool) {

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

Why not a simple function?

func returnSessionNotFound(sessionId session.SessionID) (session.Session, bool) {

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Refactored to configurable structure with single method.

}

func (mv *mockExtractor) Extract(message []byte, signature identity.Signature) (identity.Identity, error) {
return mv.onExtractReturnIdentity, mv.onExtractReturnError

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

What does mv stands for?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Renamed to extractor

}
}

func (m *middleware) Start(connection net.Conn) error {
m.connection = connection
log.Info("starting client user-pass authenticator middleware")
log.Info("starting client user-pass fetchCredentials middleware")

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

this is not fetchCredentials middleware.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Goland is too smart when renaming. Fixed

}
}

type sessionLookupFunc func(session session.SessionID) (session.Session, bool)

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

sessionFinder?

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Fixed

@@ -170,7 +173,7 @@ func (m *middleware) authenticateClient() (consumed bool, err error) {

log.Info("authenticating user: ", m.lastUsername, " clientID: ", m.clientID, " keyID: ", m.keyID)

authenticated, err := m.authenticator(m.lastUsername, m.lastPassword)
authenticated, err := m.checkCredentials(m.lastUsername, m.lastPassword)
if err != nil {
return false, err

This comment has been minimized.

Copy link
@donce

donce Jan 31, 2018

Contributor

If some error occurs in authenticator, we leave openvpn client waiting - maybe we should reply with client-deny, just as when password is incorrect? We can specify custom message in client-deny message, i.e. internal server error occured.

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 31, 2018

Author Member

Functionality added. Deny client login on credentials checking error

This comment has been minimized.

Copy link
@donce

donce Feb 1, 2018

Contributor

Don't see updates regarding this - where was functionality added?

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2018

@donce PR updated

@@ -170,7 +173,7 @@ func (m *middleware) authenticateClient() (consumed bool, err error) {

log.Info("authenticating user: ", m.lastUsername, " clientID: ", m.clientID, " keyID: ", m.keyID)

authenticated, err := m.authenticator(m.lastUsername, m.lastPassword)
authenticated, err := m.checkCredentials(m.lastUsername, m.lastPassword)
if err != nil {
return false, err

This comment has been minimized.

Copy link
@donce

donce Feb 1, 2018

Contributor

Don't see updates regarding this - where was functionality added?


// NewSessionValidator provides glue code for openvpn management interface to validate incoming client login request,
// it expects session id as username, and session signature signed by client as password
func NewSessionValidator(sessionLookup sessionFinder, extractor identity.Extractor) server_auth.CredentialsChecker {

This comment has been minimized.

Copy link
@donce

donce Feb 1, 2018

Contributor

sessionLookup should be renamed here as well.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 1, 2018

Author Member

Renamed to findSession to reflect intent


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

This comment has been minimized.

Copy link
@donce

donce Feb 1, 2018

Contributor

I'm not fully aware, what do we gain from returning function here instead of credentials itself:

  • The on-demand aspect doesn't really look useful here, because we're not gaining much.
  • We doesn't seem to be mocking this anywhere.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 1, 2018

Author Member

That was designed with other PR. Might be one of the places for future refactoring

if err != nil {
log.Error("Authentication error: ", err)
denyClientAuthWithMessage(m.connection, m.clientID, m.keyID, "internal error")
return false, err

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 1, 2018

Member

Do we really need bad crediantials reason as queue consuming error?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 1, 2018

Author Member

Improved

Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-259-validate-session-and-signature-on-service-provider-side

* 'master' of github.com:MysteriumNetwork/node: (21 commits)
  Documentation added
  Defaults are mentioned already
  Segmention fault fixed
  Documentation added
  Remove openvpn config sandbox
  Country autodetect database is configurable
  Try to lookup registration country aswell
  Possibility to inject country to Docker container
  Possibility to announce custom country
  Build correct docker file during release
  Rename 'service_discovery' package to 'discovery'
  Use default min-confidence for linter on new code
  Refetch proposals on "proposals" CLI command
  Add provider id autocompletion for "connect" CLI command
  Add CLI command for proposals
  Show tests success or failure message
  Rename "durationSeconds" to "duration"
  Refactor client.go
  Show connection statistics only after connection started
  Refactor linter warnings
  ...
@Waldz

Waldz approved these changes Feb 1, 2018

@tadovas tadovas merged commit 9999a04 into master Feb 1, 2018

@tadovas tadovas deleted the feature/MYST-259-validate-session-and-signature-on-service-provider-side branch Feb 1, 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.