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

MYST-173 securing openvpn #66

Merged
merged 12 commits into from Dec 22, 2017

Conversation

Projects
None yet
4 participants
@zolia
Copy link
Member

commented Dec 20, 2017

I see that nats code was heavily changed. Tried to adapt, not sure if it matches the idea.

@zolia zolia requested review from ignasbernotas, Waldz and tadovas Dec 20, 2017

vpnServerIp, err := cmd.IpifyClient.GetIp()
// if for some reason we will need truly external IP, use GetPublicIP()
vpnServerIp, err := cmd.IpifyClient.GetOutboundIP()

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 20, 2017

Member

Our convention is not to leave a blank line before handling an error:)

if err != nil {
return "", err
}
defer conn.Close()

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 20, 2017

Member

Shouldn't this be at the top before error handling? Is it possible, that the connection could be established with an error?:)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

yes, defer function should be registered, just after resource construction

/.env
bin/deploy_zolia

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 20, 2017

Member

This one is interesting

@@ -62,6 +64,10 @@ func (cmd *CommandRun) Run(options CommandOptions) (err error) {
SourceAddress: "10.8.0.0/24",
TargetIp: vpnServerIp,
})

// clear probable stale entries
cmd.NatService.Stop()

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

That's already implemented in command stop function cmd.Kill()

This comment has been minimized.

Copy link
@zolia

zolia Dec 20, 2017

Author Member

Yes, but it is not called. And we need just to clear stale iptables entries.

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 22, 2017

Member

Still have question how to handle that

if err != nil {
fmt.Errorf("failed to get outbound IP for NATS")
}
return NewAddress(ipToBind, 4222, identity.Address)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

NATS server address will be injected by variable similar like MYSTERIUM_API_URL.
And we have separate ticket for that MYST-27

This comment has been minimized.

Copy link
@zolia

zolia Dec 20, 2017

Author Member

For now we may assume that NATS server and myst node runs on the same machine.

func NewAddressForIdentity(identity identity.Identity, ipifyClient ipify.Client) *NatsAddress {
ipToBind, err := ipifyClient.GetOutboundIP()
if err != nil {
fmt.Errorf("failed to get outbound IP for NATS")

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Unhandled error

@@ -51,6 +52,18 @@ func (client *clientRest) GetIp() (string, error) {
return ipResponse.IP, nil
}

func (client *clientRest) GetOutboundIP() (string, error) {
conn, err := net.Dial("udp", "8.8.8.8:53")

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Is 53 DNS port? Can it be unavailable?

This comment has been minimized.

Copy link
@zolia

zolia Dec 20, 2017

Author Member

It can, but it does not matter. It is datagram, it has no state. 8.8.8.8 can even be not reachable, but we will still be able to get outbound IP

return client.ip, nil
}

func (client *clientFake) GetOutboundIP() (string, error) {
log.Info(IPIFY_API_LOG_PREFIX, "IP faked: ", client.ip)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 20, 2017

Member

Should be client.outboundIp

@Waldz Waldz changed the title Feature/myst 173 securing openvpn MYST-173 securing openvpn Dec 20, 2017

func NewAddressForIdentity(identity identity.Identity) *NatsAddress {
return NewAddress("127.0.0.1", 4222, identity.Address)
func NewAddressForIdentity(identity identity.Identity) (*NatsAddress, error) {
return NewAddress(natsServerIp, 4222, identity.Address), nil

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 22, 2017

Member

Error is never happending

address := nats_discovery.NewAddressForIdentity(identity)
address, err := nats_discovery.NewAddressForIdentity(identity)
if err != nil {
panic(err)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 22, 2017

Member

We dont handle errors with panic.
+But this error is never happening.

@Waldz

Waldz approved these changes Dec 22, 2017

@Waldz Waldz merged commit 57a3ea0 into master Dec 22, 2017

@Waldz Waldz deleted the feature/MYST-173-securing-openvpn branch Dec 22, 2017

@Waldz Waldz restored the feature/MYST-173-securing-openvpn branch Dec 22, 2017

@Waldz Waldz deleted the feature/MYST-173-securing-openvpn branch Dec 22, 2017

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.