|
Description
Ralf Habacker
2017-01-24 07:12:24 UTC
Created attachment 129121 [details] [review] Add glib based test case for testing dbus auth configuration. Comment on attachment 129121 [details] [review] Add glib based test case for testing dbus auth configuration. Review of attachment 129121 [details] [review]: ----------------------------------------------------------------- Looks reasonable to me. A few comments below. ::: test/test-auth.c @@ +1,1 @@ > +#include <config.h> Missing a copyright/licensing header. @@ +63,5 @@ > +{ > + g_assert (f->address != NULL); > + if (f->address) > + f->client_conn = test_connect_to_bus (f->ctx, f->address); > + g_assert(f->client_conn != NULL); Nitpick: Missing space before `(` (and in a few other places below). Created attachment 129161 [details] [review] Add glib based test case for testing dbus auth configuration (update 1) changes: - add copyright header - indention fix - fix of space after method name - added sha1 auth test - fixed auth-invalid.conf.in location Comment on attachment 129161 [details] [review] Add glib based test case for testing dbus auth configuration (update 1) Review of attachment 129161 [details] [review]: ----------------------------------------------------------------- This is a good starting point, but I suspect we have test coverage for most of this already. How far are you intending to take this? The reason test/manual-authz.c is a manual test is that it's difficult to actually fail authentication without either trying to connect as a different uid (for which we need to be root), or deliberately being a non-spec-compliant implementation (because obviously libdbus is always going to do its best to connect successfully, so it will never present the wrong SHA-1 or anything like that). I suspect that exercising failed authentication would require doing lower-level connection-opening using GSocket or something, and deliberately doing part of the authentication handshake incorrectly? ::: test/data/valid-config-files/auth-all.conf.in @@ +1,1 @@ > +<!-- Bus that listens on a debug pipe and doesn't create any restrictions --> Isn't this the same as debug-allow-all.conf.in? ::: test/data/valid-config-files/auth-anonymous.conf.in @@ +3,5 @@ > +<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN" > + "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd"> > +<busconfig> > + <listen>@TEST_LISTEN@</listen> > + <auth>ANONYMOUS</auth> Don't you need <allow_anonymous/> for this to be useful? Or if you're deliberately setting up a non-useful situation I think it deserves a comment saying so. ::: test/data/valid-config-files/auth-invalid.conf.in @@ +1,1 @@ > +<!-- Bus that listens on a debug pipe and doesn't create any restrictions --> The Makefile says this is in the invalid-config-files directory. Which is it? (Tests in bus/config.parser.c check that files in v-c-f are valid and that files in i-c-f are invalid, but I don't know whether validity and supportedness of auth mechanisms are checked at that level or at a higher level.) ::: test/data/valid-config-files/auth-sha1.conf.in @@ +1,1 @@ > +<!-- Bus that listens on a debug pipe and doesn't create any restrictions --> Is this the same as debug-allow-all-sha1.conf.in? ::: test/test-auth.c @@ +1,3 @@ > +/* test case for dbus daemon auth configuration > + * > + * Author: Ralf Habacker <ralf.habacker@freenet.de> Please put a copyright statement (yourself, unless you're doing this on someone else's behalf). (The reason I sometimes put "Author:" in addition is that I am often doing D-Bus work for Collabora or one of its clients, so they are the copyright holder, not me.) @@ +47,5 @@ > +typedef struct { > + const char *bug_ref; > + guint min_messages; > + const char *config_file; > + enum { SPECIFY_ADDRESS = 0, RELY_ON_DEFAULT } connect_mode; If you aren't going to use some of these fields, remove them. I think you only actually use config_file. bug_ref is only useful if you pass it to g_test_bug(), like dbus-daemon.c. That lets GLib's GTest framework report it (in conjunction with the string passed to g_test_bug_base()) as being a relevant bug for the test. @@ +72,5 @@ > +{ > + g_assert (f->address == NULL); > + if (f->address) > + f->client_conn = test_connect_to_bus (f->ctx, f->address); > + g_assert (f->client_conn == NULL); If the first assertion doesn't fail, then the "if" will never be true and the client_conn will always be NULL, so this is sort of trivial. In general test_get_dbus_daemon() can return NULL in any situation where we can't do the test, in particular if DBUS_TEST_DATA is unset. The caller is expected to skip the test if that happens. We've previously only ever done tests where test_get_dbus_daemon() is expected to succeed. But are you asserting here that the dbus-daemon will exit unsuccessfully? If you are, then I think we need to have a different dbus-daemon-starting function that waits for it to terminate, to assert that it does in fact terminate. It could just use g_spawn_sync(). @@ +79,5 @@ > +static void > +test_anonymous (Fixture *f, > + gconstpointer context) > +{ > + g_assert (f->address != NULL); This assertion is going to fail if DBUS_TEST_DATA isn't in the environment (it isn't guaranteed to be there). Skip the test if f->address is NULL instead. @@ +89,5 @@ > +static void > +test_external (Fixture *f, > + gconstpointer context) > +{ > + g_assert (f->address != NULL); Again, skip if the address is NULL. @@ +92,5 @@ > +{ > + g_assert (f->address != NULL); > + if (f->address) > + f->client_conn = test_connect_to_bus (f->ctx, f->address); > + g_assert (f->client_conn != NULL); Um, isn't this assertion going to fail on Windows, where EXTERNAL doesn't exist at all? @@ +99,5 @@ > +static void > +test_all (Fixture *f, > + gconstpointer context) > +{ > + g_assert (f->address != NULL); Again, skip if the address is NULL. Created attachment 129169 [details] [review] Add glib based test case for testing dbus auth configuration. Bug: Created attachment 129170 [details] [review] Propagate DBUS_VERBOSE settings from test-auth to spawned dbus-daemon. Bug: Comment on attachment 129161 [details] [review] Add glib based test case for testing dbus auth configuration (update 1) superseeded (In reply to Simon McVittie from comment #4) > Comment on attachment 129161 [details] [review] [review] > Add glib based test case for testing dbus auth configuration (update 1) > > Review of attachment 129161 [details] [review] [review]: > ----------------------------------------------------------------- > > This is a good starting point, but I suspect we have test coverage for most > of this already. How far are you intending to take this? see below. > I suspect that exercising failed > authentication would require doing lower-level connection-opening using > GSocket or something, and deliberately doing part of the authentication > handshake incorrectly? This has been mentioned also for the SSPI implementation and will be implemented in a second step. The first step is to have test cases for the dbus auth configuration issues detected with bug 96577 and fixed with attachment 125953 [details] [review] attachment 125955 [details] [review] attachment 127685 [details] [review] > ::: test/data/valid-config-files/auth-all.conf.in > @@ +1,1 @@ > > +<!-- Bus that listens on a debug pipe and doesn't create any restrictions --> > > Isn't this the same as debug-allow-all.conf.in? Except the debug pipe listen yes. The daemon started with test-auth.c does not provide debug pipe only @TEST_LISTEN@ which is expanded to a unix socket. > > ::: test/data/valid-config-files/auth-anonymous.conf.in > @@ +3,5 @@ > > +<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN" > > + "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd"> > > +<busconfig> > > + <listen>@TEST_LISTEN@</listen> > > + <auth>ANONYMOUS</auth> > > Don't you need <allow_anonymous/> for this to be useful? Or if you're > deliberately setting up a non-useful situation I think it deserves a comment > saying so. If you say that I add this BTW: what is the reason that this additional setting is required ? > ::: test/data/valid-config-files/auth-invalid.conf.in > @@ +1,1 @@ > > +<!-- Bus that listens on a debug pipe and doesn't create any restrictions --> > > The Makefile says this is in the invalid-config-files directory. Which is > it? (Tests in bus/config.parser.c check that files in v-c-f are valid and > that files in i-c-f are invalid, but I don't know whether validity and > supportedness of auth mechanisms are checked at that level or at a higher > level.) will be fixed. > ::: test/data/valid-config-files/auth-sha1.conf.in > @@ +1,1 @@ > > +<!-- Bus that listens on a debug pipe and doesn't create any restrictions --> > > Is this the same as debug-allow-all-sha1.conf.in? same issue as mentioned above. > ::: test/test-auth.c > @@ +1,3 @@ > > +/* test case for dbus daemon auth configuration > > + * > > + * Author: Ralf Habacker <ralf.habacker@freenet.de> > > Please put a copyright statement (yourself, unless you're doing this on > someone else's behalf). will fix that > @@ +47,5 @@ > > +typedef struct { > > + const char *bug_ref; > > + guint min_messages; > > + const char *config_file; > > + enum { SPECIFY_ADDRESS = 0, RELY_ON_DEFAULT } connect_mode; > > If you aren't going to use some of these fields, remove them. I think you > only actually use config_file. okay > @@ +72,5 @@ > > +{ > > + g_assert (f->address == NULL); > > + if (f->address) > > + f->client_conn = test_connect_to_bus (f->ctx, f->address); > > + g_assert (f->client_conn == NULL); > > If the first assertion doesn't fail, then the "if" will never be true and > the client_conn will always be NULL, so this is sort of trivial. I see > In general test_get_dbus_daemon() can return NULL in any situation where we > can't do the test, in particular if DBUS_TEST_DATA is unset. The caller is > expected to skip the test if that happens. I will add an additional function returning state if it was able to start the daemon. void setup(...) { if (!test_get_dbus_daemon_2 (...)) g_test_skip("...") } unfortunally using g_test_skip("...") in setup() does not show any text > We've previously only ever done tests where test_get_dbus_daemon() is > expected to succeed. But are you asserting here that the dbus-daemon will > exit unsuccessfully? If you are, then I think we need to have a different > dbus-daemon-starting function that waits for it to terminate, to assert that > it does in fact terminate. It could just use g_spawn_sync(). I will take a look if this is required > @@ +79,5 @@ > > +static void > > +test_anonymous (Fixture *f, > > + gconstpointer context) > > +{ > > + g_assert (f->address != NULL); > > This assertion is going to fail if DBUS_TEST_DATA isn't in the environment > (it isn't guaranteed to be there). will be fixed with f->skip as done at other test cases. Created attachment 129172 [details] [review] Fixup of f5b0bcc02f3868832c8989a523e6dc786c57210f Bug: Created attachment 129173 [details] [review] Fix endless loop in case spawned process exits in spawn_dbus_daemon(). Bug: There is still a problem that tests seems to be okay in case DBUS_TEST_DATA is not set. I used g_test_skip in setup()
> DBUS_VERBOSE=0 DBUS_TEST_DAEMON=$PWD/bin/dbus-daemon bin/test-auth 2>/dev/null
/auth/invalid: OK
/auth/external: OK
/auth/sha1: OK
/auth/anonymous: OK
/auth/any: OK
Also using the following code does not help
static void
test_invalid (Fixture *f,
gconstpointer context)
{
if (f->skip)
{
g_test_skip ("test skipped");
return;
}
Any idea how to fix ?
BTW: I did not update the whole patch with the minor fixup I recognized to be required after submitting in case you are already reviewing which may force to review the whole patch again.
(In reply to Simon McVittie from comment #4) > Um, isn't this assertion going to fail on Windows, where EXTERNAL doesn't > exist at all? windows fixed are coming in a separate patch. (In reply to Ralf Habacker from comment #8) > The daemon started with test-auth.c does > not provide debug pipe only @TEST_LISTEN@ which is expanded to a unix socket. Is there a reason you specifically need the debug pipe? (which isn't actually a pipe on Unix I don't think... if it was, EXTERNAL couldn't work) > > > + <auth>ANONYMOUS</auth> > > > > Don't you need <allow_anonymous/> for this to be useful? Or if you're > > deliberately setting up a non-useful situation I think it deserves a comment > > saying so. > If you say that I add this Well, have you tested this? Does it work? <auth>ANONYMOUS</auth> is about authentication: it means you can tell the dbus-daemon "I am anonymous" and it will accept that, rather than requiring one of the other authentication mechanisms. It applies to the same stage in the handshake as EXTERNAL, which means you can tell the dbus-daemon "I am smcv, ask the kernel if you don't believe me" and DBUS_COOKIE_SHA1, which means you can tell the dbus-daemon "I am smcv, and to prove it I will access a private file in smcv's home directory". <allow_anonymous/> is about authorization: it means someone who told the dbus-daemon "I am anonymous" is allowed to proceed to use the bus. It applies to the same stage in the handshake as <allow user="*"/>. (For instance see https://serverfault.com/questions/57077/what-is-the-difference-between-authentication-and-authorization if you are not sure about the distinction between those two terms) In 99.9% of cases <allow_anonymous/> is a very bad idea, and it's one of the things that made http://www.bbc.co.uk/news/technology-33650491 possible. I'm still very tempted to hard-code the dbus-daemon (but not libdbus) to not accept it. (In reply to Simon McVittie from comment #13) > (In reply to Ralf Habacker from comment #8) > > The daemon started with test-auth.c does > > not provide debug pipe only @TEST_LISTEN@ which is expanded to a unix socket. > > Is there a reason you specifically need the debug pipe? Oh, I understand now, you are *not* using the debug pipe in this test. Ignore that then, you're doing the right thing. These comments
> > +<!-- Bus that listens on a debug pipe and doesn't create any restrictions -->
shouldn't be in a config file that doesn't use the debug pipe :-)
(In reply to Simon McVittie from comment #4) > In general test_get_dbus_daemon() can return NULL in any situation where we > can't do the test, in particular if DBUS_TEST_DATA is unset. The caller is > expected to skip the test if that happens. I'm going to add a local replacement _test_get_dbus_daemon() returning exec state and not asserting. > > We've previously only ever done tests where test_get_dbus_daemon() is > expected to succeed. But are you asserting here that the dbus-daemon will > exit unsuccessfully? With attachment 125953 [details] [review] dbus-daemon exits immediatly in failure case. > If you are, then I think we need to have a different dbus-daemon-starting > function that waits for it to terminate, to assert that it does in fact > terminate. It could just use g_spawn_sync(). This will not work, in non error case dbus-daemon needs to run in background until it is terminated by test-auth. Instead using a 3rd party funtion what about using _dbus_spawn_async_with_babysitter() for that ? Created attachment 129183 [details] [review] Add glib based test case for testing dbus auth configuration. This patch contains an endless loop fix in spawn_dbus_daemon() in case the spawned process exits unexpected. Bug: (In reply to Ralf Habacker from comment #16) > This will not work, in non error case dbus-daemon needs to run in background > until it is terminated by test-auth. Right - what I meant is that you'd use assert_spawning_bus_fails() (or whatever you wanted to call it) for the case where you want it to fail, and test_get_dbus_daemon() for the case where you want it to continue to run. Comment on attachment 129183 [details] [review] Add glib based test case for testing dbus auth configuration. Review of attachment 129183 [details] [review]: ----------------------------------------------------------------- ::: test/data/valid-config-files/debug-allow-invalid.conf.in @@ +1,1 @@ > +<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN" I think this one deserves a comment about how it is in this directory because it's syntactically valid (hence BusConfigParser accepts it), even though the <auth> is semantically invalid (hence dbus-daemon exits unsuccessfully). ::: test/test-auth.c @@ +60,5 @@ > + TEST_USER_ME, > + &f->daemon_pid, &f->address)) > + { > + g_test_skip ("could not start dbus daemon"); > + f->skip = TRUE; In the <auth>INVALID</auth> case, aren't you trying to assert that the dbus-daemon exits unsuccessfully? If that is what you want then you need _test_get_dbus_daemon to return a tristate as described below: { skip, exited_unsuccessfully, started }. Or something like that. Surely you don't want to set f->skip for the "exited unsuccessfully" case, because the desired result of that is not actually skipping the test? @@ +98,5 @@ > +{ > + if (f->skip) > + return; > + if (f->address != NULL) > + g_test_fail(); g_error ("Was unexpectedly able to start a dbus-daemon for %s", config->config_file); or similar. (That just makes the test crash, but with a diagnostic message at least. We don't usually bother making these tests continue after an error has been encountered, so it's fine to just crash it.) @@ +108,5 @@ > + if (f->skip) > + return; > + if (f->address == NULL) > + { > + g_test_fail(); g_error ("Unable to run dbus-daemon with %s", config->config_file); would be better. ::: test/test-utils-glib.c @@ +227,5 @@ > } > > g_close (address_fd, NULL); > > + return is_running ? g_string_free (address, FALSE) : 0; Please spell the null pointer as NULL (not 0) in C. This leaks the GString and its contents in the case where the dbus-daemon isn't running. @@ +237,5 @@ > GPid *daemon_pid) > { > + gchar **address = 0; > + if (_test_get_dbus_daemon (config_file, user, daemon_pid, address)) > + return *address; I can tell you haven't tried running this, because if you had, you would have noticed it dereferencing a null pointer and segfaulting. I think you mean gchar *address = NULL; if (_test_get_dbus_daemon (..., &address)) return address; else return NULL; @@ +243,5 @@ > + return NULL; > +} > + > +dbus_bool_t > +_test_get_dbus_daemon (const gchar *config_file, Perhaps call this "test_try_get_dbus_daemon" since the difference is that it doesn't just crash out on failure to start the dbus-daemon? It probably needs a way to return three possible results: * test has been skipped * dbus-daemon failed to start (it exited with code 1 or 2 or whatever is the right one) * dbus-daemon started up successfully, its pid is in *daemon_pid test_get_dbus_daemon() needs to pass the first and the last of those through, but it should probably crash (fail the test, g_error() would do) if the dbus-daemon didn't start up. If the dbus-daemon exits, but not with the right status (or with !WIFEXITED), then this should probably just g_error() and crash the test. @@ +265,5 @@ > { > g_test_message ("set DBUS_TEST_DATA to a directory containing %s", > config_file); > g_test_skip ("DBUS_TEST_DATA not set"); > + goto fail; This should be "goto out" because you are also going to reach the label when successful (like try/finally). "goto fail" is like try/catch or try/except. @@ +451,5 @@ > + DWORD ret = WaitForSingleObject(pid, 0); > + return ret == WAIT_TIMEOUT; > +#else > + int wstatus; > + if (waitpid (pid, &wstatus, WNOHANG) == -1) You need to put the result of waitpid() in a variable and test it afterwards, because there are three cases: * -1: error (probably make it fatal with g_error(), waitpid() should never actually be failing here) * 0: pid has not exited yet, wstatus is meaningless and should not be used * positive: pid has exited, wstatus is meaningful @@ +453,5 @@ > +#else > + int wstatus; > + if (waitpid (pid, &wstatus, WNOHANG) == -1) > + return FALSE; > + if (WIFEXITED (wstatus)) I think WIFEXITED doesn't mean what you think it means. WIFEXITED is true if the process exited because of exit(), _exit(), normal return from main(), or similar. It's false if the process exited because it crashed or something. I think for both Windows and Unix, we have three possible states here: * still running (WAIT_TIMEOUT, or waitpid() returns 0) * exited in the way we expect (WIFEXITED (wstatus) && WEXITSTATUS (wstatus) == 1, or possibly 2, I can't remember what dbus-daemon's unsuccessful exit statuses are) * exited, but not in the way we expect Comment on attachment 129170 [details] [review] Propagate DBUS_VERBOSE settings from test-auth to spawned dbus-daemon. Review of attachment 129170 [details] [review]: ----------------------------------------------------------------- This change shouldn't be necessary. If envp is NULL, it defaults to inheriting the parent's environment. (It's also harmful, because the dbus-daemon won't have LD_LIBRARY_PATH or HOME or anything like that in its environment.) (In reply to Simon McVittie from comment #18) > (In reply to Ralf Habacker from comment #16) > > This will not work, in non error case dbus-daemon needs to run in background > > until it is terminated by test-auth. > > Right - what I meant is that you'd use assert_spawning_bus_fails() (or > whatever you wanted to call it) for the case where you want it to fail, and > test_get_dbus_daemon() for the case where you want it to continue to run. Unfortunally test_get_dbus_daemon() is complex. Writing a replacement for a special case looks to much efforts. It is much easier to extend test_get_dbus_daemon() to support this additional use case. (In reply to Simon McVittie from comment #13) > (In reply to Ralf Habacker from comment #8) > > The daemon started with test-auth.c does > > not provide debug pipe only @TEST_LISTEN@ which is expanded to a unix socket. > > Is there a reason you specifically need the debug pipe? (which isn't > actually a pipe on Unix I don't think... if it was, EXTERNAL couldn't work) > > > > > + <auth>ANONYMOUS</auth> > > > > > > Don't you need <allow_anonymous/> for this to be useful? Or if you're > > > deliberately setting up a non-useful situation I think it deserves a comment > > > saying so. > > If you say that I add this > > Well, have you tested this? I'm not sure that the test case in the recent state works on linux as expected. See comment 11. Created attachment 129188 [details] [review] Add glib based test case for testing dbus auth configuration. This patch contains an endless loop fix in spawn_dbus_daemon() in case the spawned process exits unexpected. Bug: Created attachment 129189 [details] [review] Add glib based test case for testing dbus auth configuration. This patch contains an endless loop fix in spawn_dbus_daemon() in case the spawned process exits unexpected. Bug: Comment on attachment 129189 [details] [review] Add glib based test case for testing dbus auth configuration. patch superseeded Created attachment 129190 [details] [review] Add glib based test case for testing dbus auth configuration. This patch contains an endless loop fix in spawn_dbus_daemon() in case the spawned process exits unexpected. Bug: Comment on attachment 129190 [details] [review] Add glib based test case for testing dbus auth configuration. fixed most issues from comment 19 (In reply to Ralf Habacker from comment #22) > (In reply to Simon McVittie from comment #13) > > (In reply to Ralf Habacker from comment #8) > > > The daemon started with test-auth.c does > > > not provide debug pipe only @TEST_LISTEN@ which is expanded to a unix socket. > > > > Is there a reason you specifically need the debug pipe? (which isn't > > actually a pipe on Unix I don't think... if it was, EXTERNAL couldn't work) > > > > > > > + <auth>ANONYMOUS</auth> > > > > > > > > Don't you need <allow_anonymous/> for this to be useful? Or if you're > > > > deliberately setting up a non-useful situation I think it deserves a comment > > > > saying so. > > > If you say that I add this > > > > Well, have you tested this? > I'm not sure that the test case in the recent state works on linux as > expected. See comment 11. Adding --tag gives some more insights. DBUS_TEST_DAEMON=$PWD/bin/dbus-daemon _DBUS_TEST_DATA=$PWD/test/data bin/test-auth --tap # random seed: R02S3ce993714b10acd955b59e92b617dd66 # Start of auth tests # set DBUS_TEST_DATA to a directory containing valid-config-files/debug-allow-invalid.conf ok 1 /auth/invalid # SKIP DBUS_TEST_DATA not set # set DBUS_TEST_DATA to a directory containing valid-config-files/debug-allow-external.conf ok 2 /auth/external # SKIP DBUS_TEST_DATA not set # set DBUS_TEST_DATA to a directory containing valid-config-files/debug-allow-sha1.conf ok 3 /auth/sha1 # SKIP DBUS_TEST_DATA not set # set DBUS_TEST_DATA to a directory containing valid-config-files/debug-allow-anonymous.conf ok 4 /auth/anonymous # SKIP DBUS_TEST_DATA not set # set DBUS_TEST_DATA to a directory containing valid-config-files/debug-allow-any.conf ok 5 /auth/any # SKIP DBUS_TEST_DATA not set # End of auth tests 1..5 running test-auth with valid test data dir returns: DBUS_TEST_DAEMON=$PWD/bin/dbus-daemon DBUS_TEST_DATA=$PWD/test/data bin/test-auth --tap # random seed: R02S095777bf9e70130ddbe19f33fc346979 # Start of auth tests dbus-daemon[10156]: Failed to start message bus: Unsupported auth mechanism "INVALID" in bus config file detected. Supported mechanisms are "EXTERNAL, DBUS_COOKIE_SHA1, ANONYMOUS". ok 1 /auth/invalid dbus-daemon[10157]: org.freedesktop.DBus.Error.NotSupported: cannot change fd limit on this platform ok 2 /auth/external dbus-daemon[10158]: org.freedesktop.DBus.Error.NotSupported: cannot change fd limit on this platform dbus-daemon[10158]: Using your real home directory for testing, set DBUS_TEST_HOMEDIR to avoid dbus[10155]: Using your real home directory for testing, set DBUS_TEST_HOMEDIR to avoid ok 3 /auth/sha1 dbus-daemon[10159]: org.freedesktop.DBus.Error.NotSupported: cannot change fd limit on this platform ok 4 /auth/anonymous dbus-daemon[10160]: org.freedesktop.DBus.Error.NotSupported: cannot change fd limit on this platform ok 5 /auth/any # End of auth tests 1..5 so the answer to your question relating to ANONYMOUS protocol is yes, it works. (In reply to Ralf Habacker from comment #8) > This has been mentioned also for the SSPI implementation and will be > implemented in a second step. The first step is to have test cases for the > dbus auth configuration issues detected with bug 96577 and fixed with > attachment 125953 [details] [review] [review] > attachment 125955 [details] [review] [review] > attachment 127685 [details] [review] [review] These patches are not part of the SSPI implementation. The related issues has been raised on testing the sspi auth, which results into adding test-auth. Running test-auth shows this issues, which let me think that those attachment should be moved into this or (a) new bug(s). What do you think ? (In reply to Ralf Habacker from comment #29) > These patches are not part of the SSPI implementation. The related issues > has been raised on testing the sspi auth, which results into adding > test-auth. Running test-auth shows this issues, which let me think that > those attachment should be moved into this or (a) new bug(s). What do you > think ? New bugs please. If in doubt, anything that's orthogonal (in the sense that we might merge one of a set of changes while rejecting a related but different change) is probably best tracked separately. Created attachment 129264 [details] [review] Patch rebased to git master branch (In reply to Ralf Habacker from comment #11) > There is still a problem that tests seems to be okay in case DBUS_TEST_DATA > is not set. I used g_test_skip in setup() > > > DBUS_VERBOSE=0 DBUS_TEST_DAEMON=$PWD/bin/dbus-daemon bin/test-auth 2>/dev/null > /auth/invalid: OK > /auth/external: OK > /auth/sha1: OK > /auth/anonymous: OK > /auth/any: OK > This is an issue with glib-2.44 which is used on opensuse LEAP 42.1: In g_test_log () the ok case is printed for skipped case too. ... fail = largs[0] != G_TEST_RUN_SUCCESS && largs[0] != G_TEST_RUN_SKIPPED; ... g_print ("%s %d %s", fail ? "not ok" : "ok", test_run_count, string1); The mingw32 glib package which is version 2.48 prints out > /auth/invalid: SKIP > /auth/external: SKIP > /auth/sha1: SKIP > /auth/anonymous: SKIP > /auth/any: SKIP which confirms that this is a glib2 version 2.44 issue. (In reply to Ralf Habacker from comment #31) > Created attachment 129264 [details] [review] [review] > Patch rebased to git master branch With depending bugs fixed the test-auth testcase works as expected and step 1 looks to be completed. Next step would be solve blocking 96577 by committing this patch to git master and closing this bug. Please finish review. Created attachment 129327 [details] [review] Add glib based test case for testing dbus auth configuration. - Fix issue not killing started dbus-daemon caused by using g_error () instead of g_test_fail () Created attachment 129335 [details] [review] Add glib based test case for testing dbus auth configuration. - autotools compile fix (In reply to Simon McVittie from comment #4) > Comment on attachment 129161 [details] [review] [review] > Add glib based test case for testing dbus auth configuration (update 1) > > Review of attachment 129161 [details] [review] [review]: > ----------------------------------------------------------------- > > This is a good starting point, but I suspect we have test coverage for most > of this already. How far are you intending to take this? For a daily dbus programmer it might be clear how to test auth but for others like me the complexibility of auth makes it very hard and a time consuming process to get an idea if changes affect auth or not. Best example is bug 96577 where checking how dbus behaves using different auth configurations was much more complicate. > The reason test/manual-authz.c is a manual test is that it's difficult to > actually fail authentication without either trying to connect as a different > uid In the DBUS_COOKIE_SHA1 case would it not be enough to emulate a different id by choosing a different user home to prevent access to the cookie ? >(for which we need to be root), Would not integrating sudo into the test case could help in this situation ? At least on linux it is possible to check if a user has sudo access (for example http://superuser.com/questions/553932/how-to-check-if-i-have-sudo-access). If there is no sudo access test-auth could print a message similiar to "There is currently no sudo access for you. Please run 'sudo su another-user'" to enable additional auth tests and rerun test-auth" or similiar. If using sudo may not be possible for any reason test-auth could print out some information how to perform additional tests. > or deliberately being a > non-spec-compliant implementation (because obviously libdbus is always going > to do its best to connect successfully, so it will never present the wrong > SHA-1 or anything like that). I suspect that exercising failed > authentication would require doing lower-level connection-opening using > GSocket or something, and deliberately doing part of the authentication > handshake incorrectly? From a quick search I found several auth related scripts test/data/auth subdir of dbus sources. This are scripts to check auth protocol ? Could they not be used or extended to manipulate the auth process ? (In reply to Ralf Habacker from comment #36) > For a daily dbus programmer it might be clear how to test auth but for > others like me the complexibility of auth makes it very hard and a time > consuming process to get an idea if changes affect auth or not. You're among the most active dbus developers. It's this hard for everyone :-( > In the DBUS_COOKIE_SHA1 case would it not be enough to emulate a different > id by choosing a different user home to prevent access to the cookie ? Maybe? Try it? > >(for which we need to be root), > > Would not integrating sudo into the test case could help in this situation ? > At least on linux it is possible to check if a user has sudo access It would be fine to have tests that skip for uid != 0, and reduce privileges to various different users during runtime - we already have at least one like that (I remember writing one). I specifically don't want to escalate privileges from ordinary user to root automatically. A small mistake in code that does that could be disastrous. Tests that rely on being started as root, and drop privileges to someone else, are nicely "opt in" - nobody is going to run them as root without accepting the risk and taking appropriate precautions, like testing in an expendable VM or container instead of on a production system. Automating the sort of tests that you can do with manual-authz would be great, but I'm afraid I don't have enough D-Bus time to do that at the moment. > From a quick search I found several auth related scripts test/data/auth > subdir of dbus sources. This are scripts to check auth protocol ? Could they > not be used or extended to manipulate the auth process ? Aha, yes. Those are used in dbus/dbus-auth-util.c, which is a unit test for the authentication state machine (not a full-stack integration test with a dbus-daemon and everything). I think they get compiled into dbus/test-dbus. -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/162. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.