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

openvpn auth poc with some cli fixes #113

Merged
merged 24 commits into from Jan 25, 2018

Conversation

Projects
None yet
3 participants
@zolia
Copy link
Member

commented Jan 22, 2018

No description provided.

return m.state
}

func (m *middleware) ConsumeLine(line string) (consumed bool, err error) {

This comment has been minimized.

Copy link
@donce

donce Jan 23, 2018

Contributor

This is a huge function, can it be split into smaller ones to get a big-picture easier?

This comment has been minimized.

Copy link
@zolia

zolia Jan 23, 2018

Author Member

true true, did split off some inner function from it.

@@ -147,7 +147,7 @@ func (c *Command) connect(line string) {
}

func (c *Command) unlock(line string) {
unlockArgs := strings.TrimSpace(line[7:])
unlockArgs := strings.TrimSpace(line[6:])

This comment has been minimized.

Copy link
@donce

donce Jan 23, 2018

Contributor

There is quite a lot of boiler-plate code in cli - this error might have been avoided by having "unlock" string const and using i.e. strings.TrimSpace(line[len(UNLOCK)]). But that's beyond scope of this PR.

username string
password string
clientId int
keyId int

This comment has been minimized.

Copy link
@donce

donce Jan 23, 2018

Contributor

it should be clientID, keyID

}

func (m *middleware) Start(connection net.Conn) error {
m.connection = connection

This comment has been minimized.

Copy link
@donce

donce Jan 23, 2018

Contributor

connection can be passed in in NewMiddleware - we'll have less mutability then.

This comment has been minimized.

Copy link
@zolia

zolia Jan 23, 2018

Author Member

according to current structure, Start() is called from management.Start() method, where listener is created. ManagementMiddleware interface defines this. We cannot change without reworking all implementations.

This comment has been minimized.

Copy link
@donce

donce Jan 23, 2018

Contributor

True, lets keep it as it is for now :)

password string
clientId int
keyId int
state middlewares.State

This comment has been minimized.

Copy link
@donce

donce Jan 23, 2018

Contributor

Are all fields required to be struct fields? I.e. password seems only needed in ConsumeLine method - limiting it's scope to that method might simplify this struct.

This comment has been minimized.

Copy link
@zolia

zolia Jan 23, 2018

Author Member

ConsumeLine works by analysing line by line. Thus it needs to store some parsed data as a state to be used for later time by the same method.

@donce donce dismissed their stale review Jan 23, 2018

changed my mind

func (cmd *CommandRun) checkClientIPWhenConnected(state state_client.State) error {
if state == state_client.STATE_CONNECTED {
// state.NewMiddleware(cmd.checkClientIPWhenConnected)
func (cmd *CommandRun) checkClientIPWhenConnected(state middlewares.State) error {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

middlewares.State very strange package for State structure.
What if if move it to openvpn.State ?

type middleware struct {
authenticator Authenticator
connection net.Conn
username string

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

I dont get why username and password is needed in global state.
Is it lastUsername or currentUser?

This comment has been minimized.

Copy link
@zolia

zolia Jan 23, 2018

Author Member

yes, that determines only current session being authorised. You cannot perform more than 1 authorisation at the same time via openvpn management interface. Access is serial, thus we only care about last (current) user. Will rename that.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

If so, we cant expose these internals to public methods. I mean middleware.State() function

match := rule.FindStringSubmatch(line)
if len(match) > 0 {
m.Reset()
m.state = middlewares.STATE_AUTH

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

Strange, that You dont read real Openvpn states

line string
}{
{">SOME_LINE_DELIVERED"},
{">ANOTHER_LINE_DELIVERED"},

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

PASSWORD: should generate error

for _, test := range tests {
consumed, err := middleware.ConsumeLine(test.line)
assert.NoError(t, err, test.line)
assert.True(t, consumed, test.line)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

Should be asserting values, passed to authenticator

type Authenticator func(username, password string) (bool, error)

// NewAuthenticator returns Authenticator callback
func NewAuthenticator() Authenticator {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

2 names are the same. What about AuthChecker or AuthService?

return err
}

func (m *middleware) State() middlewares.State {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

How can server have one single state? It has state for each client, is not it?

This comment has been minimized.

Copy link
@zolia

zolia Jan 23, 2018

Author Member

No, management interface processes all incoming request serially. Looks like auth calls are atomic.

func (m *middleware) Reset() {
m.username = ""
m.password = ""
m.clientId = -1

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

Strange value, I like -69 more

}

match = rule.FindStringSubmatch(line)
if len(match) > 0 {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

Function too long, at least each state action could be moved to different functions

}

for _, test := range tests {
authenticator := NewFakeAuthenticator()

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

I miss state there authentificationChecker has mocked login result

This comment has been minimized.

Copy link
@zolia

zolia Jan 23, 2018

Author Member

Should we test AuthenticatorChecker in authenticator_test.go separately ? middleware exposes only ConsumeLine method which does not give you authentication outcome result.

@@ -39,6 +41,10 @@ func (middleware *middleware) Stop() error {
return err
}

func (middleware *middleware) State() middlewares.State {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 23, 2018

Member

We should not expose private variable publicly

vpnMiddlewares := []openvpn.ManagementMiddleware{
auth.NewMiddleware(authenticator),
}
return openvpn.NewServer(vpnServerConfig, options.DirectoryRuntime, vpnMiddlewares...)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

You dont need to construct array here, just pass 1 middleware to kwargs openvpn.NewServer(vpnServerConfig, options.DirectoryRuntime, authMiddleware)

This comment has been minimized.

Copy link
@zolia

zolia Jan 24, 2018

Author Member

its more general case, just like in client factory. There might be more than 1 middleware initialised.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Array is needed then appending middlewares dynamically

"testing"
)

var currentState openvpn.State

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Is it used?


func updateCurrentState(state State) error {
func updateCurrentState(state openvpn.State) error {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Is it used?

This comment has been minimized.

Copy link
@zolia

zolia Jan 24, 2018

Author Member

not my code, but I see its used later in state middleware Test_ConsumeLineTakes

}
}

func NewFakeMiddleware(authenticator *FakeAuthenticatorChecker) openvpn.ManagementMiddleware {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Looks like unused function + Should not be public, I dont think it's usable outside test

{">CLIENT:ENV,username=username"},
}

var fac FakeAuthenticatorChecker

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Why this variable is needed? Usualy You just call call factory function package.New...() in package


func Test_Factory(t *testing.T) {
authenticator := &fakeAuthenticator{}
middleware := NewMiddleware(authenticator.auth)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

You dont need struct fakeAuthenticator here, just pass auth callback

{">SOME_LINE_TO_BE_DELIVERED"},
{">ANOTHER_LINE_TO_BE_DELIVERED"},
}
var fac FakeAuthenticatorChecker

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Unneeded struct here also

@@ -26,14 +26,15 @@ type ManagementMiddleware interface {
Start(connection net.Conn) error
Stop() error
ConsumeLine(line string) (consumed bool, err error)
State() (state State)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Interface is collapsing

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

You need State() only in tests. So construct exact structure (not interface) there

consumed, err := middleware.ConsumeLine(test.line)
assert.NoError(t, err, test.line)
assert.True(t, consumed, test.line)
assert.Equal(t, test.expectedState, middleware.State())

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Lets not expose internal state, your test can read it from private attrribute middleware.state

for _, test := range tests {
var fac FakeAuthenticatorChecker
authenticator := fac.NewFakeAuthenticatorChecker()
middleware := NewMiddleware(authenticator.authenticator)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

In tests I suggest construct middleware without factory NewMiddleware() - gives more control

}

// NewMiddleware creates clint user_auth challenge authentication middleware
func NewMiddleware(authenticator Authenticator) openvpn.ManagementMiddleware {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Return more exact type of your return *middleware

}

// NewMiddleware creates server user_auth challenge authentication middleware
func NewMiddleware(authenticator AuthenticatorChecker) openvpn.ManagementMiddleware {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Return more exact type of your return *middleware

@@ -4,6 +4,6 @@
function get_linker_ldflags {
echo "
-X 'github.com/mysterium/node/server.mysteriumApiUrl=${MYSTERIUM_API_URL}'

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

Not the same with mysteriumApiUrl?

This comment has been minimized.

Copy link
@donce

donce Jan 24, 2018

Contributor

No, that was was not renamed in code yet.

@@ -47,8 +47,11 @@ func (c *Config) SetDevice(deviceName string) {
c.setParam("dev", deviceName)
}

func (c *Config) SetTlsCertificate(caFile, certFile, certKeyFile string) {
func (c *Config) SetTlsCACertificate(caFile string) {

This comment has been minimized.

Copy link
@donce

donce Jan 24, 2018

Contributor

from linter:

method SetTlsCACertificate should be SetTLSCACertificate

c.AddOptions(OptionFile("ca", caFile))
}

func (c *Config) SetTlsPrivatePubKeys(certFile string, certKeyFile string) {

This comment has been minimized.

Copy link
@donce

donce Jan 24, 2018

Contributor

from linter:

method SetTlsPrivatePubKeys should be SetTLSPrivatePubKeys

state openvpn.State
}

// NewMiddleware creates clint user_auth challenge authentication middleware

This comment has been minimized.

Copy link
@donce

donce Jan 24, 2018

Contributor

client

@@ -39,6 +40,10 @@ func (middleware *middleware) Stop() error {
return err
}

func (middleware *middleware) State() openvpn.State {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

This middleware doest not have state at all

@@ -9,9 +9,10 @@ import (
type middleware struct {
listeners []clientStateCallback
connection net.Conn
state openvpn.State

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Unused state attribute

type AuthenticatorChecker func(username, password string) (bool, error)

// NewAuthenticatorFake returns AuthenticatorChecker callback
func NewAuthenticatorFake() AuthenticatorChecker {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Dont repeat package name in subnames:

  • authenticator.go -> checker.go
  • NewAuthenticatorFake() -> NewCheckerFake()
assert.True(t, consumed, test.line)
assert.Equal(t, test.expectedState, middleware.state)
}
assert.Equal(t, "username1", middleware.lastUsername)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Should assert if authenticator was called + has this username&password pair

consumed, err := middleware.ConsumeLine(test.line)
assert.NoError(t, err, test.line)
assert.False(t, consumed, test.line)
assert.NotEqual(t, test.unexpectedState, middleware.state)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Should assert if authenticator was not called

{">CLIENT:ENV,username=username1", openvpn.STATE_AUTH},
{">CLIENT:ENV,END", openvpn.STATE_UNDEFINED},
}
authFake := NewAuthenticatorFake()

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Construct local callback function and You will be able to assert if authFake was called

assert.True(t, consumed, test.line)
assert.Equal(t, test.expectedState, middleware.state)
}
authenticated, _ := authFake(middleware.lastUsername, middleware.lastPassword)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Dont call authentication but rather check that is was called by middleware

assert.True(t, consumed, test.line)
assert.Equal(t, test.expectedState, middleware.state)
}
authenticated, _ := authFake(middleware.lastUsername, middleware.lastPassword)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Same here

@@ -66,8 +67,6 @@ func NewCommandWith(
return openvpn_session.NewManager(openvpn.NewClientConfig(
vpnServerIP,
filepath.Join(options.DirectoryConfig, "ca.crt"),
filepath.Join(options.DirectoryConfig, "client.crt"),
filepath.Join(options.DirectoryConfig, "client.key"),

This comment has been minimized.

Copy link
@donce

donce Jan 25, 2018

Contributor

We should remove these files as well.

This comment has been minimized.

Copy link
@zolia

zolia Jan 25, 2018

Author Member

removed

@Waldz

Waldz approved these changes Jan 25, 2018

@donce

donce approved these changes Jan 25, 2018

@zolia zolia merged commit becdcea into master Jan 25, 2018

@zolia zolia deleted the feature/MYST-255-openvpn-auth-POC-middleware-auth branch Jan 25, 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.