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 437 sudoers for debs #263

Merged
merged 35 commits into from Jul 3, 2018

Conversation

Projects
None yet
4 participants
@zolia
Copy link
Member

commented Jun 8, 2018

mysterium-node runs on linux without root permissions.
mysterium-client runs on linux without root permissions.
Tasks that were split off to reduce the scope defined at MYST-602
Refactored to use openvpn server factory based on platform (openvpn.CreateNewServer)
Debs and Dockerfile changes
Docs updated
Most changes are due to openvpn package lint error cleanups and a lot of scripting done.

@zolia zolia requested review from shroomist, ignasbernotas, tadovas, donce and Waldz Jun 8, 2018

@donce
Copy link
Contributor

left a comment

This is huge, I'm unable to review it without getting bigger picture :O

echo "$option"
split_into_parts $option
if [ "$part1" = "dhcp-option" ] ; then
if [ "$part2" = "DNS" ] ; then

This comment has been minimized.

Copy link
@donce

donce Jun 13, 2018

Contributor

Why 8 spaces for indentation, isn't 2 enough?

INSTALL.md Outdated
@@ -45,6 +45,16 @@ sudo docker logs -f mysterium-node
### Download
* https://github.com/MysteriumNetwork/node/releases/download/{VERSION}/mysterium-node_linux_amd64.deb
* https://github.com/MysteriumNetwork/node/releases/download/{VERSION}/mysterium-node_linux_armhf.deb

### Install latest OpenVPN

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

.deb file defines all dependencies, not only openvpn. Are we seeking to use them?

# Openvpn envar parsing taken from the script in debian's openvpn package.
# Smushed together and improved by @andrewgdotcom.

#

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

Should not be config/linux/update-resolv-conf same as config/osx/update-resolv-conf?

&& systemctl disable mysterium-client \
&& rm -f /lib/systemd/system/mysterium-client.service
printf "Disabling systemd script '/lib/systemd/system/mysterium-client.service'..\n"
systemctl disable mysterium-client

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

Should we add set -e?

This comment has been minimized.

Copy link
@zolia

zolia Jun 19, 2018

Author Member

no, this is exact the problem, sometimes, when you try to remove package or purge, it may fail due to how dpkg works. It may try to execute removal scripts twice producing and error and leaving package in inconsistent state.

DISCOVERY="--discovery-address=https://testnet-api.mysterium.network/v1"
BROKER="--broker-address=testnet-broker.mysterium.network"
PROTO-"--openvpn.proto=tcp"

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

Lets map only OS specific options (config dir, runtime dir, logs dir), and lets default everything else in executable

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Not fixed

EnvironmentFile=-/etc/default/mysterium-node
ExecStart=/usr/bin/mysterium_server ${DAEMON_OPTS}

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

I would like to have systemd consistent with installation/initd.sh

...
--config-dir=$OS_DIR_CONFIG \
        --data-dir=$OS_DIR_DATA \
        --runtime-dir=$OS_DIR_RUN \
        ${DAEMON_OPTS}
...

This comment has been minimized.

Copy link
@zolia

zolia Jun 19, 2018

Author Member

As far as I understand here, the same logic is not possible. systemd.service has limited flexibility in this regard; was not able to find analogous solution.

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Easily fixable just by migrating args to ExecStart e.g.
ExecStart=/usr/bin/mysterium_server --config-dir=$OS_DIR_CONFIG

This comment has been minimized.

@@ -119,3 +130,46 @@ func NewClientConfigFromSession(vpnConfig *VPNConfig, configDir string, configFi
func wrapWithDoubleQuotes(val string) string {
return fmt.Sprintf(`"%s"`, val)
}

type newVPNServer func(options commands.ServerOptions, manager session.Manager, primitives *tls.Primitives, callback state.Callback) *Server

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

Dependency structure now is:

command:
  -> uses openvpn -> uses command (because of ServerOptions)

openvpn package can work wo client_command package, but for client_command required dependency is openvpn

Dependencies should be one way:

command:
  -> uses openvpn ()
  -> uses tun
  -> etc.

if exists, err := service.deviceExists(); exists {
log.Info(tunLogPrefix, service.device.Name+" device already exists, attempting to use it")
return err

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

Is there a point of error in boolean function?
Why to return it to upper layer, looks like it's omitted?

func (service *serviceLinuxTun) deleteDevice() (err error) {
var stderr bytes.Buffer
cmd := exec.Command(
"bash",

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

Keep in mind, that bash can be missing dependency e.g. in Alpine.
Can we call script ip directly?

This comment has been minimized.

Copy link
@zolia

zolia Jun 19, 2018

Author Member

afaik it is not possible. Tried various options - none worked. We could substitute with 'sh', which should be more universal.

This comment has been minimized.

Copy link
@zolia

zolia Jun 19, 2018

Author Member

btw, we do package bash with alpine..

# username host=(user:group) tag:commands
# let mysterium-node run any command just as root without password
mysterium-node ALL=NOPASSWD: /sbin/sysctl
mysterium-node ALL=NOPASSWD: /sbin/iptables

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

We should call these full paths from our code.

This comment has been minimized.

Copy link
@zolia

zolia Jun 19, 2018

Author Member

true, changed

config.SetDevice("tun")
config.SetPersistTun()
config.SetDevice("tun0")
config.setParam("iproute", "/usr/sbin/nonpriv-ip")

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 18, 2018

Member

I suggest to use similar logic as updateResolvConfScriptPath := wrapWithDoubleQuotes(configDir, ...).
Advantages:

  • packaging scripts is already for that, nothing breaks
  • packaging system already injects config-dir with executable

This comment has been minimized.

Copy link
@zolia

zolia Jun 19, 2018

Author Member

not exactly, since there are multiple platforms, scripts / tools will be platform dependent and we will have to deal with subdirectories..

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

I dont like calling random name /usr/sbin/nonpriv-ip which can be missing in some platforms, which bypassed .deb packaging with sudoers

Having them in config directory, would make hard dependency between packaging+runtime namings

@zolia zolia force-pushed the feature/MYST-437-sudoers-for-debs branch from 37d5198 to 30d00cd Jun 18, 2018

@donce donce changed the title Feature/myst 437 sudoers for debs WIP: Feature/myst 437 sudoers for debs Jun 20, 2018

@tadovas tadovas force-pushed the feature/MYST-437-sudoers-for-debs branch 2 times, most recently from 411484a to 8e6b097 Jun 21, 2018

@zolia zolia changed the title WIP: Feature/myst 437 sudoers for debs Feature/myst 437 sudoers for debs Jun 21, 2018

@zolia zolia force-pushed the feature/MYST-437-sudoers-for-debs branch 2 times, most recently from 999e2bf to bcfe1bd Jun 21, 2018

@tadovas tadovas force-pushed the feature/MYST-437-sudoers-for-debs branch 2 times, most recently from c885576 to 7b9620f Jun 26, 2018

@tadovas

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

@Waldz I think we are done with this PR. Please review again

@tadovas
Copy link
Member

left a comment

LGTM

&& rm -rf /var/cache/apk/*

COPY bin/helpers/prepare-run-env.sh /usr/local/bin/prepare-run-env.sh
COPY bin/client_docker/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh
ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]

COPY bin/client_package/config /etc/mysterium-client
COPY bin/client_package/config/linux /etc/mysterium-client
#COPY bin/client_package/config/common /etc/mysterium-client

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

why?

This comment has been minimized.

Copy link
@zolia

zolia Jun 28, 2018

Author Member

moved to common_package, common between client / server and different platforms

bin/common_package/=${OS_DIR_CONFIG}/ \
bin/client_package/sudoers/=${OS_DIR_SUDOERS}/ \
bin/client_package/installation/=${OS_DIR_INSTALLATION}/ \
README.md=${OS_DIR_DOC}/ \

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

No point to add README, it's meant for Github homepage

This comment has been minimized.

Copy link
@zolia

zolia Jun 28, 2018

Author Member

ok, removing README. Should leave INSTALL? It is good practice to have some sort of documentation with packages..

}

return nil
service.enableIPForwarding()

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Dont secretly swallow errors

```bash
apt-get update && apt-get install -y curl
curl -s https://swupdate.openvpn.net/repos/repo-public.gpg | apt-key add && echo "deb http://build.openvpn.net/debian/openvpn/stable xenial main" > /etc/apt/sources.list.d/openvpn-aptrepo.list && rm -rf /var/cache/apt/* /var/lib/apt/lists/*
apt-get update

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Would be nice to test same instructions in Ubuntu Docker file, because now it's all handy tested

This comment has been minimized.

Copy link
@zolia

zolia Jun 28, 2018

Author Member

excessive install removed. Tested once again.

# down /etc/openvpn/update-resolv-conf

# This check helps to avoid running this script in linux (win :)) based environments
[[ $OSTYPE = "darwin"* ]] || exit 0

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Dont need this hack anymore


err := NewDefaultValidator().IsValid(vpnConfig)
if err != nil {
return nil, err
}

clientFileConfig := newClientConfig(configDir)
clientFileConfig := newClientConfig(runtimeDir, configDir)

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Arguments are swapped here

This comment has been minimized.

Copy link
@zolia

zolia Jun 28, 2018

Author Member

not swapped. Wrong naming inside function. Fixed.

// additional needDoubleQuotes param indicates if scriptName should be surounded by double quotes ( example --up --down scripts)
func (c *GenericConfig) SetScriptParam(paramName, scriptName string, needDoubleQuotes bool) {
fullPath := filepath.Join(c.scriptSearchPath, scriptName)
if needDoubleQuotes {

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

I suggest avoid conditionals


// SetScriptParam adds parameter with special handling of the value - it's treated as script name inside script search directory
// additional needDoubleQuotes param indicates if scriptName should be surounded by double quotes ( example --up --down scripts)
func (c *GenericConfig) SetScriptParam(paramName, scriptName string, needDoubleQuotes bool) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Could be implemented as optionScript same as optionFile which was created exactly for serialisation problem


err := ls.Process.Start()
if err != nil {
ls.tunService.Stop()

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Complex resource supervision, which can be replaced with defer function on resource creation

This comment has been minimized.

Copy link
@zolia

zolia Jun 28, 2018

Author Member

this one cannot be deferred since later in code we have go routing which executes exact the same stop independently.

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 28, 2018

Member

Also resource crosess single function boundary: goroutine starts anonymous function

func (ls *linuxOpenvpnProcess) Start() error {
//TODO - can we avoid hardcoding device name?
//i.e. search for first available which is down (loop from 0 to x for fixed iteration count)
ls.tunService.Add(linux.TunnelDevice{"tun0"})

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 27, 2018

Member

Drop Add fuction and inject in factory phase to avoid hardcoding

@tadovas tadovas force-pushed the feature/MYST-437-sudoers-for-debs branch 2 times, most recently from 13dff38 to d5ec0fa Jun 28, 2018

@tadovas

This comment has been minimized.

Copy link
Member

commented Jun 28, 2018

@Waldz all comments addressed

@tadovas tadovas force-pushed the feature/MYST-437-sudoers-for-debs branch from d5ec0fa to cc7af6f Jun 28, 2018

CONF_DIR="--config-dir=/etc/mysterium-client"
RUN_DIR="--runtime-dir=/var/run/mysterium-client"
DISCOVERY="--discovery-address=https://testnet-api.mysterium.network/v1"

This comment has been minimized.

Copy link
@zolia

zolia Jul 2, 2018

Author Member

remove discrovery

This comment has been minimized.

Copy link
@tadovas

tadovas Jul 2, 2018

Member

Not yet

OS_DIR_INSTALLATION="/usr/lib/mysterium-client/installation"
OS_DIR_SUDOERS="/etc/sudoers.d"
OS_DIR_TOOLS="/usr/sbin"

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 2, 2018

Member

Not needed anymore

zolia added some commits Jun 4, 2018

tadovas and others added some commits Jun 21, 2018

@tadovas tadovas force-pushed the feature/MYST-437-sudoers-for-debs branch from cc7af6f to a42178b Jul 3, 2018

@Waldz

Waldz approved these changes Jul 3, 2018

@tadovas

tadovas approved these changes Jul 3, 2018

Copy link
Member

left a comment

LGTM

@zolia zolia merged commit 2a93765 into master Jul 3, 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

@zolia zolia deleted the feature/MYST-437-sudoers-for-debs branch Jul 3, 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.