tang icon indicating copy to clipboard operation
tang copied to clipboard

tang: allow running standalone

Open nmav opened this issue 3 years ago • 13 comments

This enables running tangd in environments without systemd (e.g., embedded), without requiring xinetd or other superservers.

nmav avatar Apr 17 '22 19:04 nmav

I had issues trying to run tangd on systems like openwrt that use procd (not systemd), so let me propose this patch set for consideration. This extends tangd to be socket aware itself so it can run either standalone or in systemd, and it would make it much easier using and integrating tangd in these systems. While at it, it adds getopt and a --version option.

In retrospect, it is really sad to see how much code one needs to add to add a listening port in a server.

nmav avatar Apr 17 '22 19:04 nmav

Codecov Report

Merging #86 (88abb28) into master (e2059ee) will decrease coverage by 0.32%. The diff coverage is 54.60%.

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
- Coverage   57.35%   57.03%   -0.33%     
==========================================
  Files           3        4       +1     
  Lines         401      533     +132     
  Branches      114      141      +27     
==========================================
+ Hits          230      304      +74     
- Misses         88      130      +42     
- Partials       83       99      +16     
Impacted Files Coverage Δ
src/tangd.c 50.73% <50.00%> (+1.22%) :arrow_up:
src/socket.c 56.56% <56.56%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e2059ee...88abb28. Read the comment docs.

codecov-commenter avatar Apr 17 '22 20:04 codecov-commenter

It seems workflows timeout or fail during dependency installation, but it is independent of the changes proposed here. I've tried to improve a little the workflows by switching to centos-stream and ubuntu 20.04.

nmav avatar Apr 18 '22 07:04 nmav

Thanks for the work here, also for fixing the CI. Overall it looks good. I will look at it more closely hopefully later today.

sergio-correia avatar May 05 '22 10:05 sergio-correia

Thank you @sergio-correia , any concerns so far?

nmav avatar May 17 '22 10:05 nmav

In general I like this, although it would be nice to have more granularity than all or none for listening interfaces. I do understand that would complicate the code a lot. And while I absolutely would like to eliminate socat/inetd/xinetd from the equation in smaller devices, it seems like this is going to be unnecessary code to maintain for the vast majority of use cases.

All that said, I have a specific issue in that it doesn't compile on FreeBSD. Fixing that is easy, it's just missing two necessary includes in socket.c: +#include <netinet/in.h> +#include <signal.h> I apologize that's not in the form of a clean patch, but I'm not familiar enough with meson to be sure what the right/clean way is to add those as conditional parts of the C source. Does meson pass in anything automatically in terms of say host_machine.system() as a compiler argument or would the meson.build file have to construct something that cpp could use?

hdholm avatar Sep 09 '22 20:09 hdholm

I sent a pull request to nmav:tmp-tangd-standalone to address FreeBSD compatibility. Hopefully @nmav won't have any issues with those changes and can pull it into this request.

hdholm avatar Sep 14 '22 01:09 hdholm

I sent a pull request to nmav:tmp-tangd-standalone to address FreeBSD compatibility. Hopefully @nmav won't have any issues with those changes and can pull it into this request.

I've included it.

nmav avatar Sep 18 '22 07:09 nmav

I think that build failure is my fault. It looks like I failed to include the iproute in the centos:stream* install dependencies when I pushed the update to @nmav

hdholm avatar Sep 18 '22 19:09 hdholm

Maybe I'm looking at it wrong, I don't have a lot of experience with github. But I'm not seeing the changes I asked for specifically changes to src/socket.c, tests.helpers, and units/tangd.rc.in don't seem to be there either in nmav:tmp-tangd-standalone or 7c65483

hdholm avatar Sep 18 '22 20:09 hdholm

I submitted the changes that didn't get made with a (hopefully) much simpler and easier to manage pull request against nmav:tmp-tangd-standalone. For reference the changes are simply:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index df9b4c3..fedb792 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -17,6 +17,8 @@ jobs:
           - debian:testing
           - debian:latest
           - ubuntu:rolling
+          - ubuntu:jammy
+          - ubuntu:focal
           - ubuntu:bionic
         stable: [true]
         include:
diff --git a/.github/workflows/install-dependencies b/.github/workflows/install-dependencies
index 9a8a99a..28abb63 100755
--- a/.github/workflows/install-dependencies
+++ b/.github/workflows/install-dependencies
@@ -33,7 +33,7 @@ centos:*)
     dnf install -y dnf-plugins-core epel-release
     dnf config-manager -y --set-enabled powertools \
         || dnf config-manager -y --set-enabled crb || :
-    dnf -y install meson socat
+    dnf -y install meson socat iproute
     dnf builddep -y tang
     ;;
 esac
diff --git a/tests/helpers b/tests/helpers
index aab7f79..984434e 100755
--- a/tests/helpers
+++ b/tests/helpers
@@ -35,7 +35,11 @@ random_port() {
 }
 
 check_if_port_listening() {
+    if [ -n "${TANG_BSD}" ]; then
+        sockstat -nl|grep "[\:\.]${1}" >/dev/null 2>&1
+    else
         ss -anl|grep "[\:\.]${1}"|grep LISTEN >/dev/null 2>&1
+    fi
 }
 
 wait_for_port()
diff --git a/units/tangd.rc.in b/units/tangd.rc.in
index a152a69..0027df2 100644
--- a/units/tangd.rc.in
+++ b/units/tangd.rc.in
@@ -4,44 +4,26 @@
 #
 
 # Should probably in the future allow running as non-root
-# and enable multiple interfaces in some cleaner way.
+# and enable multiple interfaces in some way in the future.
 
 # PROVIDE: tangd
 # REQUIRE: NETWORKING DAEMON
-# KEYWORD: nojail
 
 . /etc/rc.subr
 
 name="tangd"
 desc="Network Presence Binding Daemon (tang)"
 rcvar="tangd_enable"
-command="/usr/local/bin/socat"
 
 load_rc_config $name
 : ${tangd_enable:=no}
-: ${tangd_ip="127.0.0.1"}
 : ${tangd_port="8888"}
 : ${tangd_jwkdir="@jwkdir@"}
 : ${tangd_logfile="/var/log/tang"}
-: ${tangd_executable="@libexecdir@/tangd"}
 
-pidfile="/var/run/${name}.pid"
-required_files="@libexecdir@/${name}"
 required_dirs="${tangd_jwkdir}"
 
-start_postcmd="${name}_poststart"
-_tangd_listen_args="TCP-LISTEN:${tangd_port},bind=${tangd_ip},fork"
-command_args="${_tangd_listen_args} SYSTEM:\"${tangd_executable} ${tangd_jwkdir} 2>> ${tangd_logfile} \" &"
-
-# Since we may not be the only socat running we can't use the built-in process
-# management, so we'll need to use a pid file and find the pid from unique arguments.
-tangd_poststart() {
-  ps_pid=`ps ax -o pid= -o command= | grep ${_tangd_listen_args} | grep -v grep | awk '{print $1}'`
-  if [ -z "$ps_pid" ]; then
-    err 1 "Cannot get pid for ${command} ${command_args}"
-  fi
-  echo $ps_pid > ${pidfile}
-  return $?
-}
+command="@libexecdir@/${name}"
+command_args="-p ${tangd_port} -l ${tangd_jwkdir} 2>> ${tangd_logfile} &"
 
 run_rc_command "$1"

hdholm avatar Sep 18 '22 23:09 hdholm

I submitted the changes that didn't get made with a (hopefully) much simpler and easier to manage pull request against nmav:tmp-tangd-standalone. For reference the changes are simply:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index df9b4c3..fedb792 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -17,6 +17,8 @@ jobs:
           - debian:testing
           - debian:latest
           - ubuntu:rolling
+          - ubuntu:jammy
+          - ubuntu:focal
           - ubuntu:bionic
         stable: [true]
         include:
diff --git a/.github/workflows/install-dependencies b/.github/workflows/install-dependencies
index 9a8a99a..28abb63 100755
--- a/.github/workflows/install-dependencies
+++ b/.github/workflows/install-dependencies
@@ -33,7 +33,7 @@ centos:*)
     dnf install -y dnf-plugins-core epel-release
     dnf config-manager -y --set-enabled powertools \
         || dnf config-manager -y --set-enabled crb || :
-    dnf -y install meson socat
+    dnf -y install meson socat iproute
     dnf builddep -y tang
     ;;
 esac
diff --git a/tests/helpers b/tests/helpers
index aab7f79..984434e 100755
--- a/tests/helpers
+++ b/tests/helpers
@@ -35,7 +35,11 @@ random_port() {
 }
 
 check_if_port_listening() {
+    if [ -n "${TANG_BSD}" ]; then
+        sockstat -nl|grep "[\:\.]${1}" >/dev/null 2>&1
+    else
         ss -anl|grep "[\:\.]${1}"|grep LISTEN >/dev/null 2>&1
+    fi
 }
 
 wait_for_port()
diff --git a/units/tangd.rc.in b/units/tangd.rc.in
index a152a69..0027df2 100644
--- a/units/tangd.rc.in
+++ b/units/tangd.rc.in
@@ -4,44 +4,26 @@
 #
 
 # Should probably in the future allow running as non-root
-# and enable multiple interfaces in some cleaner way.
+# and enable multiple interfaces in some way in the future.
 
 # PROVIDE: tangd
 # REQUIRE: NETWORKING DAEMON
-# KEYWORD: nojail
 
 . /etc/rc.subr
 
 name="tangd"
 desc="Network Presence Binding Daemon (tang)"
 rcvar="tangd_enable"
-command="/usr/local/bin/socat"
 
 load_rc_config $name
 : ${tangd_enable:=no}
-: ${tangd_ip="127.0.0.1"}
 : ${tangd_port="8888"}
 : ${tangd_jwkdir="@jwkdir@"}
 : ${tangd_logfile="/var/log/tang"}
-: ${tangd_executable="@libexecdir@/tangd"}
 
-pidfile="/var/run/${name}.pid"
-required_files="@libexecdir@/${name}"
 required_dirs="${tangd_jwkdir}"
 
-start_postcmd="${name}_poststart"
-_tangd_listen_args="TCP-LISTEN:${tangd_port},bind=${tangd_ip},fork"
-command_args="${_tangd_listen_args} SYSTEM:\"${tangd_executable} ${tangd_jwkdir} 2>> ${tangd_logfile} \" &"
-
-# Since we may not be the only socat running we can't use the built-in process
-# management, so we'll need to use a pid file and find the pid from unique arguments.
-tangd_poststart() {
-  ps_pid=`ps ax -o pid= -o command= | grep ${_tangd_listen_args} | grep -v grep | awk '{print $1}'`
-  if [ -z "$ps_pid" ]; then
-    err 1 "Cannot get pid for ${command} ${command_args}"
-  fi
-  echo $ps_pid > ${pidfile}
-  return $?
-}
+command="@libexecdir@/${name}"
+command_args="-p ${tangd_port} -l ${tangd_jwkdir} 2>> ${tangd_logfile} &"
 
 run_rc_command "$1"

I cannot merge this because I cannot test, and I do not like its inner workings (the environment variable). At this point my goal is to have the linux version adopted. I think this PR can be a follow up once this is merged.

nmav avatar Sep 25 '22 05:09 nmav

@sarroutbi I think the issues you raised are now addressed. Any remaining issues to merge?

nmav avatar Oct 09 '22 07:10 nmav