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

darwin nat implementation #298

Merged
merged 3 commits into from Jul 23, 2018
Merged

Conversation

zolia
Copy link
Contributor

@zolia zolia commented Jul 16, 2018

Client still has to be on different machine.
This is due to 10.8.0.0/24 subnet clash. That is server wants to tunnel it through utun1, but client would need to tunnel it through utun2, but cannot do it since route already exists.

@zolia zolia requested review from tadovas and donce July 16, 2018 08:17
@zolia zolia requested a review from Waldz as a code owner July 16, 2018 08:17
@zolia zolia force-pushed the feature/MYST-641-darwin-nat-support branch 2 times, most recently from 9af81a0 to 91e79b9 Compare July 17, 2018 08:45
}

func (service *servicePFCtl) disableRules() {
arguments := fmt.Sprintf("/sbin/pfctl -a nat-anchor:myst -F nat")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No arguments, do You really need to format it?

return err
}
natRule := fmt.Sprintf("nat on %v inet from %v to any -> %v", iface, rule.SourceAddress, rule.TargetIP)
arguments := fmt.Sprintf("echo \"%v\" | /sbin/pfctl -vEf -", natRule)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use qoute ` to avoid \" escaping

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could could be just /sbin/pfctl -vE -f %s?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, its input from stdin (-)

}

func (service *servicePFCtl) disableRules() {
arguments := fmt.Sprintf("/sbin/pfctl -F nat")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sprintf is redundant, because there aren't variables

cmd := exec.Command(
"sh",
"-c",
arguments,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like sub-command quoting is missing, is not it?

}
}
}
return "undefined", nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not it error?

return
}

cmd := utils.SplitCommand("/usr/sbin/sysctl", "-w net.inet.ip.forwarding=0")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like duplicated code, could be DRY'ied

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its not, different path of command and different arguments.

if output, err := cmd.CombinedOutput(); err != nil {
if !strings.Contains(string(output), natRule) {
log.Warn("Failed to create pfctl rule: ", cmd.Args, " Returned exit error: ", err.Error(), " Cmd output: ", string(output))
log.Flush()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is so special about your logs, that you flush it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to flush logs on every step. Flushing is done on main package just before program exiting

@zolia zolia force-pushed the feature/MYST-641-darwin-nat-support branch 2 times, most recently from 960725f to 2aa9e30 Compare July 18, 2018 08:06
@zolia zolia requested a review from Waldz July 18, 2018 08:07
@Waldz Waldz force-pushed the feature/MYST-641-darwin-nat-support branch from 2aa9e30 to 3148ca4 Compare July 23, 2018 12:36
@zolia zolia force-pushed the feature/MYST-641-darwin-nat-support branch from 3148ca4 to 2aa9e30 Compare July 23, 2018 12:41
@Waldz Waldz force-pushed the feature/MYST-641-darwin-nat-support branch from 2aa9e30 to db5642e Compare July 23, 2018 12:55
Signed-off-by: Waldz <valdas@mysterium.network>
@Waldz Waldz force-pushed the feature/MYST-641-darwin-nat-support branch from db5642e to f51efd0 Compare July 23, 2018 13:04
Copy link
Contributor

@tadovas tadovas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zolia zolia merged commit 786c78f into master Jul 23, 2018
@zolia zolia deleted the feature/MYST-641-darwin-nat-support branch July 23, 2018 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants