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-470 pass vpn options structure #236

Merged
merged 9 commits into from May 15, 2018

Conversation

Projects
None yet
3 participants
@tadovas
Copy link
Member

commented May 10, 2018

No description provided.


vpnServerFactory func(sessionManager session.Manager, serviceLocation dto_discovery.Location,
providerID identity.Identity, callback state.Callback) *openvpn.Server
vpnServerFactory func(sessionManager session.Manager, primitives *tls.Primitives, callback state.Callback) *openvpn.Server

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

I would change naming for 'callback' -> serverStateCB or stateCallBack


// NewClientConfigGenerator returns function generating config params for remote client
func NewClientConfigGenerator(primitives *tls.Primitives, vpnServerIP string, port int, protocol string) ClientConfigGenerator {
return func() session.VPNConfig {

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

Shouldn't this better be *reference?

"strconv"
)

func NewConfig() *Config {
func NewConfig(configdir string) *Config {

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

configdir -> configDir

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

or configDirectory

@@ -48,16 +51,16 @@ func (c *Config) SetDevice(deviceName string) {
}

func (c *Config) SetTLSCACertificate(caFile string) {

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

undocumented public method

@@ -300,7 +300,7 @@ func (fd *fakeDialog) Request(producer communication.RequestProducer) (responseP
Message: "Everything is great!",
Session: session.SessionDto{
ID: "vpn-connection-id",
Config: "vpn-connection-config",
Config: session.VPNConfig{},

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

I would rename 'VPNConfig' to 'OpenVPNConfig' since session might have other configs for other transports in the future.

}

func validPort(config session.VPNConfig) error {
if config.RemotePort > 65535 || config.RemotePort < 1 {

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

should be 'config.RemotePort < 1024', first 1023 ports are reserved for superUser processes and cannot be used with userSpace processes.


func validPort(config session.VPNConfig) error {
if config.RemotePort > 65535 || config.RemotePort < 1 {
return errors.New("invalid port range")

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

I would print more verbose message: "invalid port range, should fall within 1024 .. 65535 range"

func validIPFormat(config session.VPNConfig) error {
parsed := net.ParseIP(config.RemoteIP)
if parsed == nil {
return errors.New("unable to parse ip address" + config.RemoteIP)

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

""unable to parse IP address: " - upcase and space after message.

func validCACertificate(config session.VPNConfig) error {
pemBlock, _ := pem.Decode([]byte(config.CACertificate))
if pemBlock.Type != "CERTIFICATE" {
return errors.New("invalid ca. Certificate block expected")

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

ca -> CA certificate

}
}

// Server structure describes openvpn server
// ServerCertificate structure describes openvpn server

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

should be 'Server'

"testing"
)

const fakeRunDir = "testdataoutput"

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

not used anymore

@@ -6,17 +6,29 @@ import (

const endpointSessionCreate = communication.RequestEndpoint("session-create")

//SessionCreateRequest structure represents message from service consumer to initiate session for given proposal id

This comment has been minimized.

Copy link
@zolia

zolia May 11, 2018

Member

space?

}

//VPNConfig structure represents VPN configuration options for given session
type VPNConfig struct {

This comment has been minimized.

Copy link
@Waldz

Waldz May 11, 2018

Member

Are we mixing Openvpn specific config session into session.config?

&session.UUIDGenerator{},
)
},
vpnServerFactory: func(manager session.Manager, serviceLocation dto.Location, providerID identity.Identity, callback state.Callback) *openvpn.Server {
serverConfigGenerator := openvpn.NewServerConfigGenerator(options.DirectoryRuntime, serviceLocation, providerID, options.Protocol)
vpnServerFactory: func(manager session.Manager, primitives *tls.Primitives, callback state.Callback) *openvpn.Server {

This comment has been minimized.

Copy link
@Waldz

Waldz May 11, 2018

Member

Too long lines :/

This comment has been minimized.

Copy link
@tadovas

tadovas May 14, 2018

Author Member

Reformated :) however a notice added to TODO box - cleanup server factory

config.SetTLSCrypt(tlsCryptKeyPath)
func newClientConfig(configDir string) *ClientConfig {
config := ClientConfig{NewConfig(configDir)}

config.RestrictReconnects()

This comment has been minimized.

Copy link
@Waldz

Waldz May 11, 2018

Member

Strange that 2 values are here, and others in NewClientConfigFromSession

This comment has been minimized.

Copy link
@tadovas

tadovas May 14, 2018

Author Member

This function is bigger - the diff only shows two lines

sessionMap: make(map[session.SessionID]session.Session),
creationLock: sync.Mutex{},
idGenerator: idGenerator,
configProvider: serviceConfigProvider,

This comment has been minimized.

Copy link
@Waldz

Waldz May 11, 2018

Member

SessionManager became not openvpn-specific, lets move out of package openvpn/session

@tadovas tadovas force-pushed the feature/MYST-470-pass-vpn-options-structure branch 2 times, most recently from 6e73c51 to 12091ef May 14, 2018

@tadovas

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

Fixes ready. Ping @zolia @Waldz

@tadovas tadovas force-pushed the feature/MYST-470-pass-vpn-options-structure branch from 12091ef to 7e4719c May 14, 2018

@zolia

zolia approved these changes May 14, 2018

Copy link
Member

left a comment

lgtm

@@ -12,7 +12,7 @@ func (c *ServerConfig) SetServerMode(port int, network, netmask string) {

func (c *ServerConfig) SetTLSServer() {
c.setFlag("tls-server")
c.AddOptions(OptionFile("dh", "none"))
c.AddOptions(OptionFile("dh", "none", "none"))

This comment has been minimized.

Copy link
@Waldz

Waldz May 14, 2018

Member

Are you writing "none" to file content? no good

This comment has been minimized.

Copy link
@tadovas

tadovas May 15, 2018

Author Member

Done

@@ -79,16 +79,26 @@ func NewCommandWith(
)
},

sessionManagerFactory: func(vpnServerIP string) session.Manager {
clientConfigGenerator := openvpn.NewClientConfigGenerator(options.DirectoryRuntime, vpnServerIP, options.Protocol)
sessionManagerFactory: func(primitives *tls.Primitives, vpnServerIP string) session.Manager {

This comment has been minimized.

Copy link
@Waldz

Waldz May 14, 2018

Member

We are creating several instances of sessionManager, but it does not state anymore.
Add TODO for this problem

This comment has been minimized.

Copy link
@tadovas

tadovas May 15, 2018

Author Member

Commented

@tadovas tadovas force-pushed the feature/MYST-470-pass-vpn-options-structure branch from 6fc9358 to eae0606 May 14, 2018

@tadovas

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

@Waldz comments addresed.

@Waldz

Waldz approved these changes May 15, 2018

@tadovas tadovas merged commit 29cfc6c into master May 15, 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

@tadovas tadovas deleted the feature/MYST-470-pass-vpn-options-structure branch May 15, 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.