[apparmor] [PATCH 1/4] tests: Update the regression tests for v6 policy
Tyler Hicks
tyhicks at canonical.com
Wed Mar 26 22:34:56 UTC 2014
On 2014-03-26 15:06:10, John Johansen wrote:
> On 03/26/2014 10:10 AM, Tyler Hicks wrote:
> > On 2014-03-26 12:00:45, Tyler Hicks wrote:
> >> From: John Johansen <john.johansen at canonical.com>
> >>
> >> This updates the regression tests for v6 policy. It refactors the
> >> required_features test into a have_features fn, and a new
> >> requires_features fn (renamed to catch all instances make sure they
> >> where right)
> >>
> >> The have_features fn is then applied to several test to make them
> >> conditionally apply based off of availability of the feature
> >> and policy version.
> >>
> >> Signed-off-by: John Johansen <john.johansen at canonical.com>
Acked-by: Tyler Hicks <tyhicks at canonical.com>
(with a few ignorable comments below)
> >
> > Note that I didn't ack this patch yet. I have a little bit of review
> > feedback below.
> >
> >> ---
> >> tests/regression/apparmor/dbus_eavesdrop.sh | 2 +-
> >> tests/regression/apparmor/dbus_message.sh | 2 +-
> >> tests/regression/apparmor/dbus_service.sh | 2 +-
> >> tests/regression/apparmor/prologue.inc | 23 ++++++++++++++++++-----
> >> tests/regression/apparmor/tcp.sh | 1 +
> >> tests/regression/apparmor/unix_fd_server.sh | 12 +++++++-----
> >> tests/regression/apparmor/unix_socket_file.sh | 1 +
> >> 7 files changed, 30 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/tests/regression/apparmor/dbus_eavesdrop.sh b/tests/regression/apparmor/dbus_eavesdrop.sh
> >> index 8006003..fe26b91 100755
> >> --- a/tests/regression/apparmor/dbus_eavesdrop.sh
> >> +++ b/tests/regression/apparmor/dbus_eavesdrop.sh
> >> @@ -18,7 +18,7 @@ pwd=`cd $pwd ; /bin/pwd`
> >> bin=$pwd
> >>
> >> . $bin/prologue.inc
> >> -required_features dbus
> >> +requires_features dbus
> >> . $bin/dbus.inc
> >>
> >> args="--session"
> >> diff --git a/tests/regression/apparmor/dbus_message.sh b/tests/regression/apparmor/dbus_message.sh
> >> index aeefe2a..30b1592 100755
> >> --- a/tests/regression/apparmor/dbus_message.sh
> >> +++ b/tests/regression/apparmor/dbus_message.sh
> >> @@ -18,7 +18,7 @@ pwd=`cd $pwd ; /bin/pwd`
> >> bin=$pwd
> >>
> >> . $bin/prologue.inc
> >> -required_features dbus
> >> +requires_features dbus
> >> . $bin/dbus.inc
> >>
> >> listnames="--type=method_call --session --name=org.freedesktop.DBus /org/freedesktop/DBus org.freedesktop.DBus.ListNames"
> >> diff --git a/tests/regression/apparmor/dbus_service.sh b/tests/regression/apparmor/dbus_service.sh
> >> index 8a44a2c..451a661 100755
> >> --- a/tests/regression/apparmor/dbus_service.sh
> >> +++ b/tests/regression/apparmor/dbus_service.sh
> >> @@ -17,7 +17,7 @@ pwd=`cd $pwd ; /bin/pwd`
> >> bin=$pwd
> >>
> >> . $bin/prologue.inc
> >> -required_features dbus
> >> +requires_features dbus
> >> . $bin/dbus.inc
> >>
> >> service="--$bus --name=$dest $path $iface"
> >> diff --git a/tests/regression/apparmor/prologue.inc b/tests/regression/apparmor/prologue.inc
> >> index b50d3d5..396d203 100755
> >> --- a/tests/regression/apparmor/prologue.inc
> >> +++ b/tests/regression/apparmor/prologue.inc
> >> @@ -21,19 +21,32 @@
> >> #
> >> # For this file, functions are first, entry point code is at end, see "MAIN"
> >>
> >> -required_features()
> >> +#use $() to retreive the failure message or "true" if success
> >> +have_features()
> >> {
> >> if [ ! -e "/sys/kernel/security/apparmor/features/" ] ; then
> >> - echo "Kernel feature masks not supported. Skipping tests ..."
> >> - exit 0
> >> + echo "Kernel feature masks not supported."
> >> + return 1;
> >> fi
> >>
> >> for f in $@ ; do
> >> if [ ! -e "/sys/kernel/security/apparmor/features/$f" ] ; then
> >> - echo "Required feature $f not available. Skipping tests ..."
> >> - exit 0
> >> + echo "Required feature '$f' not available."
> >> + return 2;
> >> fi
> >> done
> >> +
> >> + echo "true"
> >
> > Not a blocker by any means, but I thought it was odd to echo "true" upon
> > success... (continued below)
> >
> so yes a little odd, I can't remember all the reasons for doing it this
> way but a couple reasons are it allows for this
>
> if [ $(have_features $@) != "true" ]
>
> while at the same time allowing
>
> msg=$(have_features $@)
> if [ $msg != "true" ] ; then
> do something with message
>
> of course you can also map message strings to different error codes
>
>
> I don't really care, either way
>
> >> + return 0;
> >> +}
> >> +
> >> +requires_features()
> >> +{
> >> + local res=$(have_features $@)
> >> + if [ "$res" != "true" ] ; then
> >
> > ... because it seems like checking for a 0 return code would be cleaner.
> >
> > This is just a silly test suite function, so I don't care much either
> > way. Its just an observation.
> >
> yeah it probably would be, though IIRC I didn't want the error message
> output for every invocation of the fn. But we could get that by
> outputting the message and returning 0. So we can test for the return
> code only when needed, and also use the message return
Eh, lets not waste time on changing this.
>
> >> + echo "$res. Skipping tests ..."
> >> + exit 0
> >> + fi
> >> }
> >>
> >> requires_query_interface()
> >> diff --git a/tests/regression/apparmor/tcp.sh b/tests/regression/apparmor/tcp.sh
> >> index f1c884d..73eff1b 100755
> >> --- a/tests/regression/apparmor/tcp.sh
> >> +++ b/tests/regression/apparmor/tcp.sh
> >> @@ -15,6 +15,7 @@ pwd=`cd $pwd ; /bin/pwd`
> >> bin=$pwd
> >>
> >> . $bin/prologue.inc
> >> +requires_features network
> >>
> >> port=34567
> >> ip="127.0.0.1"
> >> diff --git a/tests/regression/apparmor/unix_fd_server.sh b/tests/regression/apparmor/unix_fd_server.sh
> >> index 4de3b26..6bc5158 100755
> >> --- a/tests/regression/apparmor/unix_fd_server.sh
> >> +++ b/tests/regression/apparmor/unix_fd_server.sh
> >> @@ -132,10 +132,12 @@ runchecktest "fd passing; confined -> confined (no perm)" fail $file $socket $fd
> >> sleep 1
> >> rm -f ${socket}
> >>
> >> -# FAIL - confined client, no access to the socket file
> >> +if [ "$(have_features policy/versions/v6)" == "true" ] ; then
> >
> > See below
> >
> >> + # FAIL - confined client, no access to the socket file
> >>
> >> -genprofile $file:$okperm $socket:rw $fd_client:px -- image=$fd_client $file:$okperm
> >> -runchecktest "fd passing; confined client w/o socket access" fail $file $socket $fd_client
> >> + genprofile $file:$okperm $socket:rw $fd_client:px -- image=$fd_client $file:$okperm
> >> + runchecktest "fd passing; confined client w/o socket access" fail $file $socket $fd_client
> >>
> >> -sleep 1
> >> -rm -f ${socket}
> >> + sleep 1
> >> + rm -f ${socket}
> >> +fi
> >> diff --git a/tests/regression/apparmor/unix_socket_file.sh b/tests/regression/apparmor/unix_socket_file.sh
> >> index 6f38acb..dbb923c 100755
> >> --- a/tests/regression/apparmor/unix_socket_file.sh
> >> +++ b/tests/regression/apparmor/unix_socket_file.sh
> >> @@ -27,6 +27,7 @@ pwd=`cd $pwd ; /bin/pwd`
> >> bin=$pwd
> >>
> >> . $bin/prologue.inc
> >> +requires_features policy/versions/v6
> >
> > My current Ubuntu Trusty kernel and the several previous release kernels
> > don't have $aafs/features/policy/versions/v6, or the versions/
> > directory at all, but these tests ran just fine. With this change, these
> > tests will be skipped on those kernels.
> >
> > What's the point of this v6 check and what kernels have
> > $aafs/features/policy/versions/v6?
> >
> right so this is the mediate unix sockets on connect behavior. This was added
> in Saucy but done poorly and there was no way to auto detect between old and
> new semantics.
>
> This lead to problems for the backport kernels, and chroots, lxc, running older
> userspaces on a newer kernel.
>
> To fix this we introduced extra versioning. This appears in the ipc test kernels
> in the dbus-deb ppa.
>
> So older userspaces that don't understand the v6 semanitc, or newer userspaces
> run on older kernels, work with the old v5 semantic.
>
> This does lead to one unfortunate case of a trusty kernel on saucy will fail
> open. That is it does not enforce the mediate on unix socket behavior that
> saucy did (because the userspace does not provide anything to the kernel to
> be able to determine it should enforce the new semantic). This should be
> acceptable because that combination is not supported, and saucy will go out
> of support entirely soon. Making this work for precise, trusty and forward
> was far more important.
Got it. Thanks for explaining!
Tyler
>
>
> > Tyle
> >
> >>
> >> client=$bin/unix_socket_file_client
> >> socket=${tmpdir}/unix_socket_file.sock
> >> --
> >> 1.9.1
> >>
> >>
> >> --
> >> AppArmor mailing list
> >> AppArmor at lists.ubuntu.com
> >> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140326/01f01b31/attachment-0001.pgp>
More information about the AppArmor
mailing list