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

Use chroot to verify iptables on the host #227

Merged
merged 2 commits into from
Nov 28, 2019

Conversation

sridhargaddam
Copy link
Member

@sridhargaddam sridhargaddam commented Nov 26, 2019

On some platforms, its seen that iptables on the host is a symlink to a file
(f.e, /etc/alternatives/iptables) which again is a symlink to another file
(f.e., /usr/sbin/iptables-legacy). On such platforms, with the current code,
when we validate the presence of iptables using "[[ -x /host/usr/sbin/iptables ]]",
it fails since "-x" dereferences the symbolic links and the symlink would point to
a file on the container file system (instead of checking the host filesystem).

This patch uses chroot with the host-filesystem for validating the presence of iptables.

Signed-off-by: Sridhar Gaddam sgaddam@redhat.com


This change is Reviewable

@sridhargaddam
Copy link
Member Author

Another alternative is to validate the presence of file as follows.
[[ -L /host/usr/sbin/iptables ]]
OR
[[ -x /host/usr/sbin/iptables || -L /host/usr/sbin/iptables ]]

it works. But I feel that using chroot is a more robust and efficient mechanism.

Comment on lines 13 to 14
chroot /host test -x /usr/sbin/$1 && { echo "0"; return; }
chroot /host test -x /sbin/$1 && { echo "1"; return; }
Copy link
Member

Choose a reason for hiding this comment

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

Instead of hard-coding return-value-to-paths mappings, why not “return” the path directly?

    for file in {/usr,}/sbin/$1; do if [ -x $file ]; then echo $file; return; fi; done
    echo ""

then

iptables=$(verify_iptables_on_host)

if [ -n "$iptables" ]; then
    ...
else
    # Oops, no iptables
fi

Copy link
Member Author

Choose a reason for hiding this comment

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

Depending on the location of the iptables/iptables-save utility on the host, we have to copy one of the wrapper files (i.e., L30 and L32 below). If we are just checking for [ -n "$iptables" ], how can we distinguish which file to copy?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, yes, I missed that.

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem @skitt. I missed a small change and realized that i pushed a wrong file. Fixed it now. Thanks for reviewing.

On some platforms, its seen that iptables on the host is a symlink to a file
(f.e, /etc/alternatives/iptables) which again is a symlink to another file
(f.e., /usr/sbin/iptables-legacy). On such platforms, with the current code,
when we validate the presence of iptables using "[[ -x /host/usr/sbin/iptables ]]",
it fails since "-x" dereferences the symbolic links and the symlink would point to
a file on the container file system (instead of checking the host filesystem).

This patch uses chroot with the host-filesystem for validating the presence of iptables.

Signed-off-by: Sridhar Gaddam <sgaddam@redhat.com>
Copy link
Contributor

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dimaunx, @skitt, and @sridhargaddam)


package/submariner-route-agent.sh, line 14 at r1 (raw file):

Previously, sridhargaddam (Sridhar Gaddam) wrote…

No problem @skitt. I missed a small change and realized that i pushed a wrong file. Fixed it now. Thanks for reviewing.

Why don't we change to:

function verify_iptables_on_host() {
    chroot /host test -x /usr/sbin/$1 && { echo "/usr/sbin"; return; }
    chroot /host test -x /sbin/$1 && { echo "/sbin"; return; }
}

In such way, the later piece is code is more evident, "0" "1" seemed to be like you were returning true/false, and I was going to propose doing && false / && true ... but then I saw it dint't make sense


package/submariner-route-agent.sh, line 27 at r2 (raw file):

for f in iptables-save iptables; do
  iptablesExists=$(verify_iptables_on_host $f)
  if [ $iptablesExists == "0" ]; then

== "/usr/sbin"


package/submariner-route-agent.sh, line 30 at r2 (raw file):

    echo "$f is present on the host at /usr/sbin/$f"
    cp /usr/sbin/${f}.wrapper /usr/sbin/$f
  elif [ $iptablesExists == "1" ]; then

== "/sbin"

Copy link

@dimaunx dimaunx left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @skitt and @sridhargaddam)

@mangelajo mangelajo merged commit 7213c10 into submariner-io:master Nov 28, 2019
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.

4 participants