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 <[email protected]> > >> > >> 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 <[email protected]>
Acked-by: Tyler Hicks <[email protected]> (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 > >> [email protected] > >> Modify settings or unsubscribe at: > >> https://lists.ubuntu.com/mailman/listinfo/apparmor >
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
