diff mbox

[1/7] Add MM Auth test; add auth_action_str() function

Message ID 1459024539-31433-2-git-send-email-nhofmeyr@sysmocom.de
State New
Headers show

Commit Message

Neels Hofmeyr March 26, 2016, 8:35 p.m. UTC 
Add basic MM Authentication test setup, with fake DB access and RAND_bytes().

So far implement simple tests for IO error during DB access and missing auth
entry.

To print the auth action during tests, implement auth_action_str() inline
function.
---
 openbsc/.gitignore                    |   1 +
 openbsc/configure.ac                  |   1 +
 openbsc/include/openbsc/auth.h        |  18 +++++
 openbsc/tests/Makefile.am             |   2 +-
 openbsc/tests/mm_auth/Makefile.am     |  21 ++++++
 openbsc/tests/mm_auth/mm_auth_test.c  | 119 ++++++++++++++++++++++++++++++++++
 openbsc/tests/mm_auth/mm_auth_test.ok |   8 +++
 openbsc/tests/testsuite.at            |   7 ++
 8 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100644 openbsc/tests/mm_auth/Makefile.am
 create mode 100644 openbsc/tests/mm_auth/mm_auth_test.c
 create mode 100644 openbsc/tests/mm_auth/mm_auth_test.ok

Comments

Harald Welte March 27, 2016, 8:53 a.m. UTC | #1 
Hi Neels,

On Sat, Mar 26, 2016 at 09:35:33PM +0100, Neels Hofmeyr wrote:
> +static inline const char *auth_action_str(enum auth_action a)

we normally try to avoid introducing some custom value/string converter
code like this and use 'struct value_string' for this.

> +#define AUTH_CASE(X) \
> +	case X: return #X

you can also define a macro like this that generates a
	{ X, #X },
for struct value_string
> +	case -1:
> +		return "(internal error)";

this would be an ugly

	{ -1, "(internal error") }

which is soon replaced by your #define AUTH_ERROR anyway.

... and please refrain from having non-trivial functions (or data
definitions), and particularly non-performance-critical functions like
this inline in a header file.

I think I know why you do that (to use it in the test case and in the
main code) but this still doesn't rectify it, sorry ;)

I would merge the entire patch-set, if you could address that one issue. Thanks!
Neels Hofmeyr March 29, 2016, 10:16 a.m. UTC | #2 
On Sun, Mar 27, 2016 at 10:53:53AM +0200, Harald Welte wrote:
> Hi Neels,
> 
> On Sat, Mar 26, 2016 at 09:35:33PM +0100, Neels Hofmeyr wrote:
> > +static inline const char *auth_action_str(enum auth_action a)
> 
> we normally try to avoid introducing some custom value/string converter
> code like this and use 'struct value_string' for this.

ah :)
(sorry for the noise from previous mail)

> > +#define AUTH_CASE(X) \
> > +	case X: return #X
> 
> you can also define a macro like this that generates a
> 	{ X, #X },
> for struct value_string
> > +	case -1:
> > +		return "(internal error)";
> 
> this would be an ugly
> 
> 	{ -1, "(internal error") }
> 
> which is soon replaced by your #define AUTH_ERROR anyway.

yes, I preferred to first have the test suite in place before introducing
more enum values...

> ... and please refrain from having non-trivial functions (or data
> definitions), and particularly non-performance-critical functions like
> this inline in a header file.

ack

> I would merge the entire patch-set, if you could address that one issue. Thanks!

great, am onto it!

~Neels
diff mbox

Patch

diff --git a/openbsc/.gitignore b/openbsc/.gitignore
index 55f4a31..28fdcc8 100644
--- a/openbsc/.gitignore
+++ b/openbsc/.gitignore
@@ -80,6 +80,7 @@  tests/sgsn/sgsn_test
 tests/subscr/subscr_test
 tests/oap/oap_test
 tests/gtphub/gtphub_test
+tests/mm_auth/mm_auth_test
 
 tests/atconfig
 tests/atlocal
diff --git a/openbsc/configure.ac b/openbsc/configure.ac
index 24dbc30..60601fe 100644
--- a/openbsc/configure.ac
+++ b/openbsc/configure.ac
@@ -216,6 +216,7 @@  AC_OUTPUT(
     tests/subscr/Makefile
     tests/oap/Makefile
     tests/gtphub/Makefile
+    tests/mm_auth/Makefile
     doc/Makefile
     doc/examples/Makefile
     Makefile)
diff --git a/openbsc/include/openbsc/auth.h b/openbsc/include/openbsc/auth.h
index d41d141..6c463d4 100644
--- a/openbsc/include/openbsc/auth.h
+++ b/openbsc/include/openbsc/auth.h
@@ -11,6 +11,24 @@  enum auth_action {
 	AUTH_DO_AUTH		= 3,	/* Only authentication, no ciphering */
 };
 
+static inline const char *auth_action_str(enum auth_action a)
+{
+#define AUTH_CASE(X) \
+	case X: return #X
+
+	switch (a) {
+	AUTH_CASE(AUTH_NOT_AVAIL);
+	AUTH_CASE(AUTH_DO_AUTH_THEN_CIPH);
+	AUTH_CASE(AUTH_DO_CIPH);
+	AUTH_CASE(AUTH_DO_AUTH);
+	case -1:
+		return "(internal error)";
+	default:
+		return "(unknown auth_action)";
+	}
+#undef AUTH_CASE
+}
+
 int auth_get_tuple_for_subscr(struct gsm_auth_tuple *atuple,
                               struct gsm_subscriber *subscr, int key_seq);
 
diff --git a/openbsc/tests/Makefile.am b/openbsc/tests/Makefile.am
index 04b8e34..09298a3 100644
--- a/openbsc/tests/Makefile.am
+++ b/openbsc/tests/Makefile.am
@@ -1,4 +1,4 @@ 
-SUBDIRS = gsm0408 db channel mgcp gprs abis gbproxy trau subscr
+SUBDIRS = gsm0408 db channel mgcp gprs abis gbproxy trau subscr mm_auth
 
 if BUILD_NAT
 SUBDIRS += bsc-nat bsc-nat-trie
diff --git a/openbsc/tests/mm_auth/Makefile.am b/openbsc/tests/mm_auth/Makefile.am
new file mode 100644
index 0000000..516df00
--- /dev/null
+++ b/openbsc/tests/mm_auth/Makefile.am
@@ -0,0 +1,21 @@ 
+AM_CPPFLAGS = $(all_includes) -I$(top_srcdir)/include
+AM_CFLAGS=-Wall \
+	  $(LIBOSMOCORE_CFLAGS) \
+	  $(LIBOSMOGSM_CFLAGS) \
+	  $(LIBCRYPTO_CFLAGS)
+
+noinst_PROGRAMS = mm_auth_test
+
+EXTRA_DIST = mm_auth_test.ok
+
+mm_auth_test_SOURCES = mm_auth_test.c
+
+mm_auth_test_LDFLAGS = \
+	-Wl,--wrap=db_get_authinfo_for_subscr \
+	-Wl,--wrap=db_get_lastauthtuple_for_subscr \
+	-Wl,--wrap=db_sync_lastauthtuple_for_subscr
+
+mm_auth_test_LDADD = $(top_builddir)/src/libmsc/libmsc.a \
+		     $(top_builddir)/src/libcommon/libcommon.a \
+		     $(LIBOSMOCORE_LIBS) \
+		     $(LIBOSMOGSM_LIBS)
diff --git a/openbsc/tests/mm_auth/mm_auth_test.c b/openbsc/tests/mm_auth/mm_auth_test.c
new file mode 100644
index 0000000..d8e4475
--- /dev/null
+++ b/openbsc/tests/mm_auth/mm_auth_test.c
@@ -0,0 +1,119 @@ 
+#include <stdbool.h>
+
+#include <osmocom/core/application.h>
+#include <osmocom/core/logging.h>
+
+#include <openbsc/debug.h>
+#include <openbsc/gsm_data.h>
+#include <openbsc/gsm_subscriber.h>
+#include <openbsc/auth.h>
+
+/* override, requires '-Wl,--wrap=db_get_authinfo_for_subscr' */
+int __real_db_get_authinfo_for_subscr(struct gsm_auth_info *ainfo,
+				      struct gsm_subscriber *subscr);
+
+int test_get_authinfo_rc = 0;
+struct gsm_auth_info test_auth_info = {0};
+struct gsm_auth_info default_auth_info = {
+	.auth_algo = AUTH_ALGO_COMP128v1,
+	.a3a8_ki_len = 16,
+	.a3a8_ki = { 0 }
+};
+
+int __wrap_db_get_authinfo_for_subscr(struct gsm_auth_info *ainfo,
+				      struct gsm_subscriber *subscr)
+{
+	*ainfo = test_auth_info;
+	printf("wrapped: db_get_authinfo_for_subscr(): rc = %d\n", test_get_authinfo_rc);
+	return test_get_authinfo_rc;
+}
+
+/* override, requires '-Wl,--wrap=db_get_lastauthtuple_for_subscr' */
+int __real_db_get_lastauthtuple_for_subscr(struct gsm_auth_tuple *atuple,
+					   struct gsm_subscriber *subscr);
+
+int test_get_lastauthtuple_rc = 0;
+struct gsm_auth_tuple test_last_auth_tuple = { 0 };
+struct gsm_auth_tuple default_auth_tuple = { 0 };
+
+int __wrap_db_get_lastauthtuple_for_subscr(struct gsm_auth_tuple *atuple,
+					   struct gsm_subscriber *subscr)
+{
+	*atuple = test_last_auth_tuple;
+	printf("wrapped: db_get_lastauthtuple_for_subscr(): rc = %d\n", test_get_lastauthtuple_rc);
+	return test_get_lastauthtuple_rc;
+}
+
+/* override, requires '-Wl,--wrap=db_sync_lastauthtuple_for_subscr' */
+int __real_db_sync_lastauthtuple_for_subscr(struct gsm_auth_tuple *atuple,
+					    struct gsm_subscriber *subscr);
+int test_sync_lastauthtuple_rc = 0;
+int __wrap_db_sync_lastauthtuple_for_subscr(struct gsm_auth_tuple *atuple,
+					    struct gsm_subscriber *subscr)
+{
+	test_last_auth_tuple = *atuple;
+	printf("wrapped: db_sync_lastauthtuple_for_subscr(): rc = %d\n", test_sync_lastauthtuple_rc);
+	return test_sync_lastauthtuple_rc;
+}
+
+int auth_get_tuple_for_subscr_verbose(struct gsm_auth_tuple *atuple,
+				      struct gsm_subscriber *subscr,
+				      int key_seq)
+{
+	int auth_action;
+	auth_action = auth_get_tuple_for_subscr(atuple, subscr, key_seq);
+	printf("auth_get_tuple_for_subscr(key_seq=%d) --> auth_action == %s\n",
+	       key_seq, auth_action_str(auth_action));
+	return auth_action;
+}
+
+/* override libssl RAND_bytes() to get testable crypto results */
+int RAND_bytes(uint8_t *rand, int len)
+{
+	memset(rand, 23, len);
+	return 1;
+}
+
+static void test_error()
+{
+	int auth_action;
+
+	struct gsm_auth_tuple atuple = {0};
+	struct gsm_subscriber subscr = {0};
+	int key_seq = 0;
+
+	printf("\n* test_error()\n");
+
+	/* any error (except -ENOENT) */
+	test_get_authinfo_rc = -EIO;
+	auth_action = auth_get_tuple_for_subscr_verbose(&atuple, &subscr,
+							key_seq);
+	OSMO_ASSERT(auth_action == -1);
+}
+
+static void test_auth_not_avail()
+{
+	int auth_action;
+
+	struct gsm_auth_tuple atuple = {0};
+	struct gsm_subscriber subscr = {0};
+	int key_seq = 0;
+
+	printf("\n* test_auth_not_avail()\n");
+
+	/* no entry */
+	test_get_authinfo_rc = -ENOENT;
+	auth_action = auth_get_tuple_for_subscr_verbose(&atuple, &subscr,
+							key_seq);
+	OSMO_ASSERT(auth_action == AUTH_NOT_AVAIL);
+}
+
+int main(void)
+{
+	osmo_init_logging(&log_info);
+	log_set_log_level(osmo_stderr_target, LOGL_INFO);
+
+	test_error();
+	test_auth_not_avail();
+	return 0;
+}
diff --git a/openbsc/tests/mm_auth/mm_auth_test.ok b/openbsc/tests/mm_auth/mm_auth_test.ok
new file mode 100644
index 0000000..5efb3de
--- /dev/null
+++ b/openbsc/tests/mm_auth/mm_auth_test.ok
@@ -0,0 +1,8 @@ 
+
+* test_error()
+wrapped: db_get_authinfo_for_subscr(): rc = -5
+auth_get_tuple_for_subscr(key_seq=0) --> auth_action == (internal error)
+
+* test_auth_not_avail()
+wrapped: db_get_authinfo_for_subscr(): rc = -2
+auth_get_tuple_for_subscr(key_seq=0) --> auth_action == AUTH_NOT_AVAIL
diff --git a/openbsc/tests/testsuite.at b/openbsc/tests/testsuite.at
index 6a1c77f..dab9568 100644
--- a/openbsc/tests/testsuite.at
+++ b/openbsc/tests/testsuite.at
@@ -117,3 +117,10 @@  AT_CHECK([test "$enable_gtphub_test" != no || exit 77])
 cat $abs_srcdir/gtphub/gtphub_test.ok > expout
 AT_CHECK([$abs_top_builddir/tests/gtphub/gtphub_test], [], [expout], [ignore])
 AT_CLEANUP
+
+AT_SETUP([mm_auth])
+AT_KEYWORDS([mm_auth])
+cat $abs_srcdir/mm_auth/mm_auth_test.ok > expout
+AT_CHECK([$abs_top_builddir/tests/mm_auth/mm_auth_test], [], [expout], [ignore])
+AT_CLEANUP
+