bogus handling of DAEMON_OPTS by chronyd-starter.sh

Hi Simon,
you are right and this is a valid bug for sure.
Yet I have a very busy week ahead - it might take some time ... :-/

Hrm I got to test this now and I can't reproduce with:

Groovy
root@g:~# grep OPT /etc/default/chrony
DAEMON_OPTS="-F -1 -x"
root@g:~# systemctl restart chrony; systemctl status chrony | grep 'sbin\/chronyd'
             ├─118154 /usr/sbin/chronyd -F -1 -x
             └─118155 /usr/sbin/chronyd -F -1 -x

Focal
root@f:~# grep OPT /etc/default/chrony
DAEMON_OPTS="-F -1 -x"
root@f:~# systemctl restart chrony; systemctl status chrony | grep 'sbin\/chronyd'
             ├─80336 /usr/sbin/chronyd -F -1 -x
             └─80337 /usr/sbin/chronyd -F -1 -x

Hmm, maybe my containers I currently use are "too capable"

The script does two things:
1. it checks if it is running in a container
OR
2. it checks if it misses cap_sys_time

If 1 || 2 are true the service would (by default) fail to run at all.
The wrapper automatically adds -x in that case.

So if 1 || 2 you'll see:
  "Adding -x as fallback disabling control of the system clock, see
  /usr/share/doc/chrony/README.container to override this behavior"

But when you add -x in the OPTs yourself it should still be ok.
It runs a getopt loop if there is a -x.
If it is set it does not go into any of the later checks and runs chrony as you told it.

Test on non privileged container
default:
             ├─1459 /usr/sbin/chronyd -F -1 -x
             └─1460 /usr/sbin/chronyd -F -1 -x
...
Oct 05 11:27:20 g-chrony chronyd-starter.sh[1454]: Warning: Running in a container, likely impossible and unintended to sync system clock
Oct 05 11:27:20 g-chrony chronyd-starter.sh[1454]: Adding -x as fallback disabling control of the system clock, see /usr/share/doc/chrony/README.container to override this behavior

Adding -x to the options still does not break it ... hmm:

Groovy:
root@g-chrony:~# grep OPT /etc/default/chrony
DAEMON_OPTS="-F -1 -x"
root@g-chrony:~# systemctl restart chrony; systemctl status chrony | grep 'sbin\/chronyd'
             ├─1688 /usr/sbin/chronyd -F -1 -x
             └─1689 /usr/sbin/chronyd -F -1 -x

Focal:
root@f-chrony:~# grep OPT /etc/default/chrony
DAEMON_OPTS="-F -1 -x"
root@f-chrony:~# systemctl restart chrony; systemctl status chrony | grep 'sbin\/chronyd'
             ├─2221 /usr/sbin/chronyd -F -1 -x
             └─2222 /usr/sbin/chronyd -F -1 -x

Still ok.
Could you add set -x to /usr/lib/systemd/scripts/chronyd-starter.sh and let me know why the getopt loop might not work for you?

From /usr/lib/systemd/scripts/chronyd-starter.sh:
# Check if -x is already set manually, don't process further if that is the case
X_SET=0
while getopts ":x" opt; do
    case $opt in
        x)
            X_SET=1
            ;;
    esac
done

Hello Christian,

One need to use "-F 1 -x" in order to reproduce. The "1" instead of "-1"
is what causes the bug to happen. Thanks!

Regards,
Simon

On 2020-10-05 7:33 a.m., Christian Ehrhardt  wrote:
> Hrm I got to test this now and I can't reproduce with:
>
> Groovy
> root@g:~# grep OPT /etc/default/chrony
> DAEMON_OPTS="-F -1 -x"
> root@g:~# systemctl restart chrony; systemctl status chrony | grep 'sbin\/chronyd'
> ├─118154 /usr/sbin/chronyd -F -1 -x
> └─118155 /usr/sbin/chronyd -F -1 -x
>
> Focal
> root@f:~# grep OPT /etc/default/chrony
> DAEMON_OPTS="-F -1 -x"
> root@f:~# systemctl restart chrony; systemctl status chrony | grep 'sbin\/chronyd'
> ├─80336 /usr/sbin/chronyd -F -1 -x
> └─80337 /usr/sbin/chronyd -F -1 -x
>
> Hmm, maybe my containers I currently use are "too capable"
>
> The script does two things:
> 1. it checks if it is running in a container
> OR
> 2. it checks if it misses cap_sys_time
>
> If 1 || 2 are true the service would (by default) fail to run at all.
> The wrapper automatically adds -x in that case.
>
> So if 1 || 2 you'll see:
> "Adding -x as fallback disabling control of the system clock, see
> /usr/share/doc/chrony/README.container to override this behavior"
>
>
> But when you add -x in the OPTs yourself it should still be ok.
> It runs a getopt loop if there is a -x.
> If it is set it does not go into any of the later checks and runs chrony as you told it.
>
> Test on non privileged container
> default:
> ├─1459 /usr/sbin/chronyd -F -1 -x
> └─1460 /usr/sbin/chronyd -F -1 -x
> ...
> Oct 05 11:27:20 g-chrony chronyd-starter.sh[1454]: Warning: Running in a container, likely impossible and unintended to sync system clock
> Oct 05 11:27:20 g-chrony chronyd-starter.sh[1454]: Adding -x as fallback disabling control of the system clock, see /usr/share/doc/chrony/README.container to override this behavior
>
>
> Adding -x to the options still does not break it ... hmm:
>
> Groovy:
> root@g-chrony:~# grep OPT /etc/default/chrony
> DAEMON_OPTS="-F -1 -x"
> root@g-chrony:~# systemctl restart chrony; systemctl status chrony | grep 'sbin\/chronyd'
> ├─1688 /usr/sbin/chronyd -F -1 -x
> └─1689 /usr/sbin/chronyd -F -1 -x
>
> Focal:
> root@f-chrony:~# grep OPT /etc/default/chrony
> DAEMON_OPTS="-F -1 -x"
> root@f-chrony:~# systemctl restart chrony; systemctl status chrony | grep 'sbin\/chronyd'
> ├─2221 /usr/sbin/chronyd -F -1 -x
> └─2222 /usr/sbin/chronyd -F -1 -x
>
>
> Still ok.
> Could you add set -x to /usr/lib/systemd/scripts/chronyd-starter.sh and let me know why the getopt loop might not work for you?
>
>
>>From /usr/lib/systemd/scripts/chronyd-starter.sh:
> # Check if -x is already set manually, don't process further if that is the case
> X_SET=0
> while getopts ":x" opt; do
> case $opt in
> x)
> X_SET=1
> ;;
> esac
> done
>
>
> ** Changed in: chrony (Ubuntu)
> Status: New => Incomplete
>

Thanks Simon, I missed that little detail when retrying.
I now can confirm.

The impact is gladly rather low as it makes one "-x" to two "-x -x" which should not change the behavior of chronyd.

For the time being anyone affected that sets "-x" manually in /etc/default/chrony and want to avoid the double -x you can just set
   SYNC_IN_CONTAINER="yes"
That will also stop the wrapper from adding -x here.

As mentioned we use getopt ignoring all but -x to select.

opstring so far was ":x"
1. leading ":" => silence errors
2. look for -x

We had hoped that we don't have to "keep track" of the official chronyd arguments that way.

But what happens is that the "1" as argument to -F with an optstring that misses "F:" will make getopt consider it the end of the options.
Therefore the getopt loop ends and won't reach the -x :-/

For some hardening I wonder if for our purpose "is a -x set" I wonder if we should switch away from getopt entirely or improve on it.

The following is already much more robust ignoring any bad args and odd arguments in between
for idx in $(seq 1 $#); do
    OPTIND=$idx
    echo PROCESS OPTIND $OPTIND
    if getopts ":x" opt; then
        echo OPT is $opt
        if [ "$opt" = "x" ]; then
            echo -x is set
        fi
    fi
done

But it would still fails at combined short args like -qdx to recognize -x out of these.

The following is even better, but depends on bash
for arg in $@; do
    echo ARG = $arg
    if [[ "$arg" =~ ^-[a-zA-Z0-9]*x ]]; then
        X_SET=1
    fi
done

Or we could do the same with grep

for arg in $@; do
    echo ARG = $arg
    if echo "$arg" | grep -q -e '^-[a-zA-Z0-9]*x'; then
         X_SET=1
    fi
done

The last two versions catch:
a) -F -1 -x
b) -F 1 -x
c) -dx
d) not confused by --longopts (non existing in chronyd today, but who knows)
e) any combination of a/b/c/d

bash and grep both already have
  Essential: yes
So we'd not need a dependency on it when introducing this change.

Generally people like grep more than bash regex, so I'm going with that.

Uploaded to groovy and not requiring an FFe (since it is a small and pure fix).
Never the less it will wait a bit until the release Team approves it since we are in Freeze for 20.10.

This bug was fixed in the package chrony - 3.5.1-1ubuntu2

---------------
chrony (3.5.1-1ubuntu2) groovy; urgency=medium

  * d/chronyd-starter.sh: fix commandline argument parsing (LP: #1898000)

 -- Christian Ehrhardt <email address hidden> Tue, 06 Oct 2020 12:20:40 +0200