3
\$\begingroup\$

So this is a follow up questions to Find proceses listening on the network outside of default package manager

I managed to make it a little faster thanks to using /proc and hopefully managed to implement most of the suggestions. Some minor questions

  • Can we get rid of the pipes in favor of here strings? I attempted but ran into issues
  • Can we get rid of the custom readarray function? It takes too much space and is only used once. Was struggling to find a succinct replacement.
  • As before the code needs to support bash 3.2+ so a lot of fancy features are unfortunately not available.

As before it works just as intended

Improved code

#!/bin/bash

# finds processes listening in on the network outside outside of the default package manager

usage=$(cat <<-EOF
$(basename "$0").sh [-h] [-f] [-a] [-w] [-v] [-p]

Lists packages listening on the network outside of the default package manager

where:
    -h,  --help                     show this help text
    -f,  --whitelist-file           pick a file[s] containing whitelisted programs
    -w,  --whitelist                explicitly add whitelisted programs
    -W,  --ignore-whitelist         ignore the whitelist, includes env variables
    -v,  --view-whitelist           view the complete whitelist
    -p,  --package-manager          which package-manager to use (rpm, dkpg, ...)
    -P,  --ignore-package-manager   ignores the check for a working package manager

output:
  COMMAND    PATH                PID    USER     FD   TYPE  DEVICE    SIZE/OFF  NODE  NAME
  mattermos  /app/main/mattermo  3351   361000   25u  IPv4  84625698  0t0       TCP   129.240...(ESTABLISHED)

examples:
    List all processes listening on the network
      usit_network_network_listening_processes --list-all 

    List only packages not maintained by the central package manager system
      usit_network_network_listening_processes --list-all 

    Whitelist some processes 
      usit_network_network_listening_processes --whitelist mattermos,systemd
      usit_network_network_listening_processes --whitelist 'mattermos systemd'

    Whitelist some processes using whitelist files
      usit_network_network_listening_processes -f /etc/uio/uio_network_listening_processes_whitelist_01.txt

    Multiple whitelist files is supported
      usit_network_network_listening_processes --whitelist foo.txt,bar.txt

environment variables:
  The following environment variables are available
  
    USIT_LISTENER_WHITELIST
    USIT_LISTENER_WHITELIST_FILES
  
  These form the base for the whitelist and whitelist files respectively. 
  Use a string with spaces or comma [,] as seperator for the values.
EOF
)

fatal() {
    printf "ERROR: $*" >&2
    exit 1
}

# This is a substitute for readarray for older bash versions
# https://stackoverflow.com/a/64793921/1048781
if ! type -t readarray >/dev/null; then
  # Very minimal readarray implementation using read. Does NOT work with lines that contain double-quotes due to eval()
  readarray() {
    local cmd opt t v=MAPFILE
    while [ -n "$1" ]; do
      case "$1" in
      -h|--help) echo "minimal substitute readarray for older bash"; exit; ;;
      -r) shift; opt="$opt -r"; ;;
      -t) shift; t=1; ;;
      -u) 
          shift; 
          if [ -n "$1" ]; then
            opt="$opt -u $1"; 
            shift
          fi
          ;;
      *)
          if [[ "$1" =~ ^[A-Za-z_]+$ ]]; then
            v="$1"
            shift
          else
            echo -en "${C_BOLD}${C_RED}Error: ${C_RESET}Unknown option: '$1'\n" 1>&2
            exit
          fi
          ;;
      esac
    done
    cmd="read $opt"
    eval "$v=()"
    while IFS= eval "$cmd line"; do      
      line=$(echo "$line" | sed -e "s#\([\"\`]\)#\\\\\1#g" )
      eval "${v}+=(\"$line\")"
    done
  }
fi

# Transform long options to short ones
args=()

for arg; do
    case $arg in
    '--whitelist')                args+=(-w) ;;
    '--whitelist-file')           args+=(-f) ;;
    '--ignore-whitelist')         args+=(-W) ;;
    '--ignore-package-manager')   args+=(-P) ;;
    '--view-whitelist')           args+=(-v) ;;
    '--package-manager')          args+=(-p) ;;
    '--help')                     args+=(-h) ;;
    *)                            args+=("$arg") ;;
    esac
done

set -- "${args[@]}"

# The '//,/ ' allows us to input variables using , seperator as well as SPACE
whitelist=()
whitelist_files=()
read -ar whitelist <<< "${USIT_LISTENER_WHITELIST//,/ }"
read -ar whitelist_files <<< "${USIT_LISTENER_WHITELIST_FILES//,/ }"
view_whitelist=false
ignore_whitelist=false
ignore_package_manager=false
while getopts ":w:f:p:vPWh" flag; do
    case "${flag}" in
    h)
        echo "$usage"
        exit 0
        ;;
    v) view_whitelist=true ;;
    W) ignore_whitelist=true ;;
    P) ignore_package_manager=true ;;
    w) read -ra parts <<< "${OPTARG//,/ }"
       whitelist+=("${parts[@]}")
       ;;
    f) read -ra parts <<< "${OPTARG//,/ }"
       whitelist_files+=("${parts[@]}")
       ;;
    p) package_manager=${OPTARG} ;;
    \?)
        fatal "illegal option: -%s${OPTARG} \n\n$usage\n\n"
        ;;
    esac
done

# -i : This option filters only processes whith an IPv[46] address:
# -P : This option inhibits the conversion of port numbers to port names for network files.
# -n : This option inhibits the conversion of network numbers to host names for network files.
# -l : This option inhibits the conversion of user ID numbers to login names.
# +M : Enables the reporting of portmapper registrations for local TCP and UDP ports.
# The awk filters only unique PID's by  _[val] looks up val in the hash _(a regular variable).
# NR > 1 is used to skip the header
network_listening_processes=$(
  lsof -Pnl +M -i | 
    awk 'NR>1 && !seen[$1]++ { print }' 
)
# we use the realpath -m to extract the execution path from the process id (pid)
network_listening_processes_with_path=$(
    while read -r a pid b; do
        printf "%s %s %s %s\n" "$a" "'$(realpath -m /proc/"$pid"/exe)'" "$pid" "$b"
    done <<< "${network_listening_processes[@]}"
)
if "${ignore_whitelist}" && "${ignore_package_manager}"; then
    echo "${network_listening_processes_with_path}" | column -t
    exit
fi

function is_in_array {
    local array="$1[@]"
    local seeking="${!2}"
    local result=1
    for element in "${!array}"; do
        if [[ $element == "$seeking" ]]; then
            result=0
            break
        fi
    done
    return $result
}

function is_empty_array {
    local array="$1[@]"
    for element in "${!array}"; do
        return 1
    done
    return 0
}

function print_array {
    local array="$1[@]"
    for element in "${!array}"; do
        echo "${element}"
    done
}

# Block of code for getting the correct package manager
is_in_standard_repo_rpm() {
    rpm -qf "${1}" &>/dev/null
}
is_in_standard_repo_dpkg() {
    dpkg -S "${1}" &>/dev/null
}
is_in_standard_repo_any() {
    local package="${1}"
    return 1
}
if "${ignore_package_manager}"; then
    package_manager="any"
else
    dpkg_equivalents=("dpkg" "apt" "")
    rpm_equivalents=("rpm" "yum" "dnf" "")
    if is_in_array dpkg_equivalents package_manager && dpkg --version &>/dev/null; then
        package_manager="dpkg"
    elif is_in_array rpm_equivalents package_manager && rpm --version &>/dev/null; then
        package_manager="rpm"
    else
        fatal "No supported package manager found! Try specifying one with -p --package-manager"
    fi
fi
is_in_standard_repo () {
    is_in_standard_repo_$package_manager "${!1}"
}

# block of code for handling the whitelist
if "${ignore_whitelist}"; then
    whitelist=()
else
    if ! is_empty_array whitelist_files; then
        readarray -t whitelist_from_file < <(sort -u "${whitelist_files[@]}")
        whitelist+=("${whitelist_from_file[@]}")
    fi
fi
if "${view_whitelist}"; then
    ! is_empty_array whitelist && print_array whitelist
    exit
fi

# Finally we list every process not in the default packagen_manager
# Which is listening on the network
while IFS= read -r line; do
    programname=$(awk '{print $1}' <<< "${line}")
    path=$(awk '{print $2}' <<< "${line}")
    path_without_quotes=$(sed 's/^.\(.*\).$/\1/' <<< "${path}")
    if ! is_in_standard_repo path_without_quotes && ! is_in_array whitelist programname; then
        echo "${line}"
    fi
done <<< "${network_listening_processes_with_path}" | column -t
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Use shellcheck

shellcheck points out violations of good practices.

Let me highlight just a few interesting ones, I recommend to review all.


The variable package is unused. So you can delete the line.

is_in_standard_repo_any() {
    local package="${1}"
    return 1
}

Instead of:

printf "ERROR: $*" >&2

The recommended form is:

printf "ERROR: %s" "$*" >&2

Note that printf doesn't print a newline by default. I think that's not good UX when the output of a script ends without a newline. You could add a \n there, but then this is really the same as what a simpler echo does, so I would just do:

echo "$*" >&2

... and there are even more, I suggest to review and address them all.

Do not construct Bash function names using string concatenation

This is a dirty hack:

is_in_standard_repo () {
    is_in_standard_repo_$package_manager "${!1}"
}

On my first pass, I was looking for usages of is_in_standard_repo_rpm and I couldn't find it, because the function name is constructed from string parts. Confusing.

As I recommended for the previous iteration, it would be better to use is_in_standard_repo as a variable, assign to it the function that should be called, and then call the function using "$is_in_standard_repo" ARGS....

Avoid indirect expansion when possible

The array helper functions rely heavily on indirect expansion, that is, the ${!name} construct to refer to the variable named by $name. I find it confusing, and I would only use it as a last resort. shellcheck is also not able to handle this well, for example it reports rpm_equivalents as unused, even though it is used, with indirect expansion.

You can avoid it by reworking the array helpers to take variable contents directly. Let's take for example is_in_array. Instead of taking:

  • the name of an array to search in
  • the name of the variable whose value to search for

Make it work with:

  • a value to search for as first argument
  • the array elements as the remaining elements

Like this:

is_in_array() {
    local seeking=$1
    shift

    for element; do
        if [[ $element == "$seeking" ]]; then
            return
        fi
    done
    return 1
}

This will make the calls slightly longer:

is_in_array "$package_manager" "${rpm_equivalents[@]}"

But I think it's much easier to understand the code, and this also resolve the shellcheck issues.

Avoid flag variables when possible

Flag variables, that are set during a loop and then acted on later are error prone. For example they might get accidentally overwritten, or not acted on at all. For example the result variable in this code:

local result=1
for element in "${!array}"; do
    if [[ $element == "$seeking" ]]; then
        result=0
        break
    fi
done
return $result

You can avoid the flag variable by acting immediately:

for element in "${!array}"; do
    if [[ $element == "$seeking" ]]; then
        return 0
    fi
done
return 1

Although I wrote an explicit return 0, a simple return would be equivalent.

Use pipes instead of storing command outputs for one-time use

The network_listening_processes and network_listening_processes_with_path variables store the output of commands, and they are used only once per execution path. You could make them functions, call them when they are needed, and pipe the result for further processing:

network_listening_processes() {
  lsof -Pnl +M -i | 
    awk 'NR > 1 && !seen[$1]++'
)

network_listening_processes_with_path() {
    network_listening_processes | while read -r a pid b; do
        printf "%s %s %s %s\n" "$a" "'$(realpath -m /proc/"$pid"/exe)'" "$pid" "$b"
    done
)

if "${ignore_whitelist}" && "${ignore_package_manager}"; then
    network_listening_processes_with_path | column -t
    exit
fi

# ...

network_listening_processes_with_path | while IFS= read -r line; do
    # ...
done | column -t

Read pair of variables from lines

Instead of:

while IFS= read -r line; do
    programname=$(awk '{print $1}' <<< "${line}")
    path=$(awk '{print $2}' <<< "${line}")
    # ...
done

You can read pair of variables without additional awk processes for every line of input, like this:

while read -r programname path rest; do
    # ...
done

read can read multiple values separated by whitespace, and stores all excess values in the last variable (here rest).

Unhelpful error message

In this code:

if is_in_array "$package_manager" "${dpkg_equivalents[@]}" && dpkg --version &>/dev/null; then
    package_manager="dpkg"
elif is_in_array "$package_manager" "${rpm_equivalents[@]}" && rpm --version &>/dev/null; then
    package_manager="rpm"
else
    fatal "No supported package manager found! Try specifying one with -p --package-manager"
fi

It seems to me that the error message is not helpful. When a package manager is not specified, the if-else chain will try to use dpkg or rpm if possible. If not possible, then I don't see what value a user could possibly set that will not result in the failure.

I think you need to rethink the logic of this validation, and either tell the user that their system is not supported (and why), or make the message more specific to be helpful.

Use consistent style

Some functions are using modern style, for example fatal() { ... }, while others use old style, for example function is_in_array { ... }. Use consistently modern style.

Avoid redundant return 0

The return 0 at the end of this function is unnecessary, you can simply drop it:

function is_empty_array {
    local array="$1[@]"
    for element in "${!array}"; do
        return 1
    done
    return 0
}

awk tips

Instead of:

awk 'NR>1 && !seen[$1]++ { print }' 

I suggest:

awk 'NR > 1 && !seen[$1]++'

print is the default action to do when the filter is true, that's why it can be dropped. And it's easier to read when there are spaces around the operator in NR>1.

Why the hacky readarray now?

The previous iteration of your code already used readarray, under the same Bash 3.2+ requirements. So I'm puzzled why bring in a hacky implementation now...

\$\endgroup\$
3
  • \$\begingroup\$ In the v1 review, I suggested using parameter substitution to choose between the two sets of functions. I agree that it has potential to interrupt the reader, but OTOH it scales well and IMO it's less intrusive than code that's mostly cases to choose the implementation to use. \$\endgroup\$ Aug 29, 2022 at 8:44
  • \$\begingroup\$ I think readability and simplicity in this use case are more important than scalability. \$\endgroup\$
    – janos
    Aug 29, 2022 at 9:20
  • \$\begingroup\$ I meant scalability in terms of number of functions and of package managers, rather than number of processes; I should have been clearer about that. Nevertheless, your suggestion of using a variable for each function is a good one, and arguably better. Possibly an associative-array variable if we need many such functions. \$\endgroup\$ Aug 29, 2022 at 9:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.