[PATCH 1/4] Added new auth policy black-list.
In this mode by default we set authorized = 1 for all new subscribers. BSC accepts all MS, except subscribers not authorized in DB. All subscribers with authorized = 0 are part of the black list and not accepted. --- openbsc/include/openbsc/gsm_data.h | 1 + openbsc/src/libbsc/bsc_vty.c | 5 +++-- openbsc/src/libcommon/gsm_data.c | 1 + openbsc/src/libmsc/db.c | 12 +++++++++--- openbsc/src/libmsc/gsm_04_08.c | 2 ++ 5 files changed, 16 insertions(+), 5 deletions(-) diff --git a/openbsc/include/openbsc/gsm_data.h b/openbsc/include/openbsc/gsm_data.h index 8741505..99e9b27 100644 --- a/openbsc/include/openbsc/gsm_data.h +++ b/openbsc/include/openbsc/gsm_data.h @@ -194,6 +194,7 @@ enum gsm_auth_policy { GSM_AUTH_POLICY_CLOSED, /* only subscribers authorized in DB */ GSM_AUTH_POLICY_ACCEPT_ALL, /* accept everyone, even if not authorized in DB */ GSM_AUTH_POLICY_TOKEN, /* accept first, send token per sms, then revoke authorization */ + GSM_AUTH_POLICY_BLACK_LIST /* accept everyone, except subscribers not authorized in DB */ }; #define GSM_T3101_DEFAULT 10 diff --git a/openbsc/src/libbsc/bsc_vty.c b/openbsc/src/libbsc/bsc_vty.c index 5748945..7a89ca6 100644 --- a/openbsc/src/libbsc/bsc_vty.c +++ b/openbsc/src/libbsc/bsc_vty.c @@ -1186,12 +1186,13 @@ DEFUN(cfg_net_name_long, DEFUN(cfg_net_auth_policy, cfg_net_auth_policy_cmd, - "auth policy (closed|accept-all|token)", + "auth policy (closed|accept-all|token|black-list)", "Authentication (not cryptographic)\n" "Set the GSM network authentication policy\n" "Require the MS to be activated in HLR\n" "Accept all MS, whether in HLR or not\n" - "Use SMS-token based authentication\n") + "Use SMS-token based authentication\n" + "Accept all MS, except not authorized in HLR\n") { enum gsm_auth_policy policy = gsm_auth_policy_parse(argv[0]); struct gsm_network *gsmnet = gsmnet_from_vty(vty); diff --git a/openbsc/src/libcommon/gsm_data.c b/openbsc/src/libcommon/gsm_data.c index 5f7e32e..31b65ee 100644 --- a/openbsc/src/libcommon/gsm_data.c +++ b/openbsc/src/libcommon/gsm_data.c @@ -256,6 +256,7 @@ static const struct value_string auth_policy_names[] = { { GSM_AUTH_POLICY_CLOSED, "closed" }, { GSM_AUTH_POLICY_ACCEPT_ALL, "accept-all" }, { GSM_AUTH_POLICY_TOKEN, "token" }, + { GSM_AUTH_POLICY_BLACK_LIST, "black-list"}, { 0, NULL } }; diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 21abce9..440509a 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -319,6 +319,7 @@ struct gsm_subscriber *db_create_subscriber(struct gsm_network *net, char *imsi) { dbi_result result; struct gsm_subscriber *subscr; + int authorized = 0; /* Is this subscriber known in the db? */ subscr = db_get_subscriber(net, GSM_SUBSCRIBER_IMSI, imsi); @@ -337,17 +338,22 @@ struct gsm_subscriber *db_create_subscriber(struct gsm_network *net, char *imsi) if (!subscr) return NULL; subscr->flags |= GSM_SUBSCRIBER_FIRST_CONTACT; + + if (net->auth_policy == GSM_AUTH_POLICY_BLACK_LIST) + authorized = 1; + result = dbi_conn_queryf(conn, "INSERT INTO Subscriber " - "(imsi, created, updated) " + "(imsi, created, updated, authorized) " "VALUES " - "(%s, datetime('now'), datetime('now')) ", - imsi + "(%s, datetime('now'), datetime('now'), %d) ", + imsi, authorized ); if (!result) LOGP(DDB, LOGL_ERROR, "Failed to create Subscriber by IMSI.\n"); subscr->net = net; subscr->id = dbi_conn_sequence_last(conn, NULL); + subscr->authorized = authorized; strncpy(subscr->imsi, imsi, GSM_IMSI_LENGTH-1); dbi_result_free(result); LOGP(DDB, LOGL_INFO, "New Subscriber: ID %llu, IMSI %s\n", subscr->id, subscr->imsi); diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index d81dab9..8f8eaa9 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -241,6 +241,8 @@ static int authorize_subscriber(struct gsm_loc_updating_operation *loc, return (subscriber->flags & GSM_SUBSCRIBER_FIRST_CONTACT); case GSM_AUTH_POLICY_ACCEPT_ALL: return 1; + case GSM_AUTH_POLICY_BLACK_LIST: + return subscriber->authorized; default: return 0; } -- 1.7.9.5
--- openbsc/tests/vty_test_runner.py | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) mode change 100644 => 100755 openbsc/tests/vty_test_runner.py diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py old mode 100644 new mode 100755 index ab9670c..3594129 --- a/openbsc/tests/vty_test_runner.py +++ b/openbsc/tests/vty_test_runner.py @@ -169,6 +169,42 @@ class TestVTYNITB(TestVTYGenericBSC): self.assertEquals(res.find('periodic location update 60'), -1) self.assert_(res.find('no periodic location update') > 0) + def testAuthPolicy (self): + self.vty.enable() + self.vty.command("configure terminal") + self.vty.command("network") + + # Test invalid input + self.vty.verify("auth policy", ['% Command incomplete.']) + + # Enable auth policy closed + self.vty.verify("auth policy closed", ['']) + + # Verify settings + res = self.vty.command("write terminal") + self.assert_(res.find('auth policy closed') > 0) + + # Enable auth policy accept-all + self.vty.verify("auth policy accept-all", ['']) + + # Verify settings + res = self.vty.command("write terminal") + self.assert_(res.find('auth policy accept-all') > 0) + + # Enable auth policy token + self.vty.verify("auth policy token", ['']) + + # Verify settings + res = self.vty.command("write terminal") + self.assert_(res.find('auth policy token') > 0) + + # Enable auth policy black-list + self.vty.verify("auth policy black-list", ['']) + + # Verify settings + res = self.vty.command("write terminal") + self.assert_(res.find('auth policy black-list') > 0) + class TestVTYBSC(TestVTYGenericBSC): def vty_command(self): -- 1.7.9.5
--- openbsc/src/libbsc/bsc_vty.c | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/openbsc/src/libbsc/bsc_vty.c b/openbsc/src/libbsc/bsc_vty.c index 7a89ca6..a06aa90 100644 --- a/openbsc/src/libbsc/bsc_vty.c +++ b/openbsc/src/libbsc/bsc_vty.c @@ -539,6 +539,14 @@ static void config_write_bts_single(struct vty *vty, struct gsm_bts *bts) vty_out(vty, " cell barred 1%s", VTY_NEWLINE); if ((bts->si_common.rach_control.t2 & 0x4) == 0) vty_out(vty, " rach emergency call allowed 1%s", VTY_NEWLINE); + if ((bts->si_common.rach_control.t3) != 0) + for (i = 0; i < 8; i++) + if (bts->si_common.rach_control.t3 & (0x1 << i)) + vty_out(vty, " rach access-control-class %d barred%s", i, VTY_NEWLINE); + if ((bts->si_common.rach_control.t2 & 0xfb) != 0) + for (i = 0; i < 8; i++) + if ((i != 2) && (bts->si_common.rach_control.t2 & (0x1 << i))) + vty_out(vty, " rach access-control-class %d barred%s", i+8, VTY_NEWLINE); for (i = SYSINFO_TYPE_1; i < _MAX_SYSINFO_TYPE; i++) { if (bts->si_mode_static & (1 << i)) { vty_out(vty, " system-information %s mode static%s", @@ -1899,6 +1907,51 @@ DEFUN(cfg_bts_rach_ec_allowed, cfg_bts_rach_ec_allowed_cmd, return CMD_SUCCESS; } +DEFUN(cfg_bts_rach_ac_class, cfg_bts_rach_ac_class_cmd, + "rach access-control-class (0|1|2|3|4|5|6|7|8|9|11|12|13|14|15) (barred|allowed)", + RACH_STR + "Set access control class\n" + "Access control class 0\n" + "Access control class 1\n" + "Access control class 2\n" + "Access control class 3\n" + "Access control class 4\n" + "Access control class 5\n" + "Access control class 6\n" + "Access control class 7\n" + "Access control class 8\n" + "Access control class 9\n" + "Access control class 11 for PLMN use\n" + "Access control class 12 for security services\n" + "Access control class 13 for public utilities (e.g. water/gas suppliers)\n" + "Access control class 14 for emergency services\n" + "Access control class 15 for PLMN staff\n" + "barred to use access control class\n" + "allowed to use access control class\n") +{ + struct gsm_bts *bts = vty->index; + + uint8_t control_class; + uint8_t allowed = 0; + + if (strcmp(argv[1],"allowed") == 0) + allowed = 1; + + control_class = atoi(argv[0]); + if (control_class < 8) + if (allowed) + bts->si_common.rach_control.t3 &= ~(0x1 << control_class); + else + bts->si_common.rach_control.t3 |= (0x1 << control_class); + else + if (allowed) + bts->si_common.rach_control.t2 &= ~(0x1 << (control_class - 8)); + else + bts->si_common.rach_control.t2 |= (0x1 << (control_class - 8)); + + return CMD_SUCCESS; +} + DEFUN(cfg_bts_ms_max_power, cfg_bts_ms_max_power_cmd, "ms max power <0-40>", "MS Options\n" @@ -3066,6 +3119,7 @@ int bsc_vty_init(const struct log_info *cat) install_element(BTS_NODE, &cfg_bts_rach_nm_ldavg_cmd); install_element(BTS_NODE, &cfg_bts_cell_barred_cmd); install_element(BTS_NODE, &cfg_bts_rach_ec_allowed_cmd); + install_element(BTS_NODE, &cfg_bts_rach_ac_class_cmd); install_element(BTS_NODE, &cfg_bts_ms_max_power_cmd); install_element(BTS_NODE, &cfg_bts_per_loc_upd_cmd); install_element(BTS_NODE, &cfg_bts_no_per_loc_upd_cmd); -- 1.7.9.5
--- openbsc/tests/vty_test_runner.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/openbsc/tests/vty_test_runner.py b/openbsc/tests/vty_test_runner.py index 3594129..1e25a91 100755 --- a/openbsc/tests/vty_test_runner.py +++ b/openbsc/tests/vty_test_runner.py @@ -205,6 +205,41 @@ class TestVTYNITB(TestVTYGenericBSC): res = self.vty.command("write terminal") self.assert_(res.find('auth policy black-list') > 0) + def testRachAccessControlClass(self): + self.vty.enable() + self.vty.command("configure terminal") + self.vty.command("network") + self.vty.command("bts 0") + + # Test invalid input + self.vty.verify("rach access-control-class", ['% Command incomplete.']) + self.vty.verify("rach access-control-class 1", ['% Command incomplete.']) + self.vty.verify("rach access-control-class -1", ['% Unknown command.']) + self.vty.verify("rach access-control-class 10", ['% Unknown command.']) + self.vty.verify("rach access-control-class 16", ['% Unknown command.']) + + # Barred rach access control classes + for classNum in range(16): + if classNum!=10: + self.vty.verify("rach access-control-class " + str(classNum) + " barred", ['']) + + # Verify settings + res = self.vty.command("write terminal") + for classNum in range(16): + if classNum!=10: + self.assert_(res.find("rach access-control-class " + str(classNum) + " barred") > 0) + + # Allowed rach access control classes + for classNum in range(16): + if classNum!=10: + self.vty.verify("rach access-control-class " + str(classNum) + " allowed", ['']) + + # Verify settings + res = self.vty.command("write terminal") + for classNum in range(16): + if classNum!=10: + self.assertEquals(res.find("rach access-control-class " + str(classNum) + " barred"), -1) + class TestVTYBSC(TestVTYGenericBSC): def vty_command(self): -- 1.7.9.5
On Mon, Sep 16, 2013 at 01:13:02PM +0400, Ivan Kluchnikov wrote:
+ + if (net->auth_policy == GSM_AUTH_POLICY_BLACK_LIST) + authorized = 1;
this causes a segfault in the tests (as there is no network that we pass). E.g. make check is failing. On second thought I wonder about the semantic of this patch. E.g. if I run an accept-all network.. and then switch to a closed policy. The authorized field will still be 0. But with this change I can not easily change. Have you considered using an enum like enum { AUTHORIZED_NOTSET, AUTHORIZED_ALLOWED, AUTHORIZES_BLACKLISTED, };
+ return subscriber->authorized;
return subscriber->authorized != AUTHORIZES_BLACKLISTED? doesn't look too bad and one avoids the using net inside the db code.
Hi Holger, 2013/9/18 Holger Hans Peter Freyther <holger@freyther.de>:
this causes a segfault in the tests (as there is no network that we pass). E.g. make check is failing.
Yes, I missed "make check", I will fix db test to support our case.
On second thought I wonder about the semantic of this patch. E.g. if I run an accept-all network.. and then switch to a closed policy. The authorized field will still be 0. But with this change I can not easily change.
For auth policy clarification: accept-all = accept MS with authorized = 1 and 0, for all new subscribers set authorized = 0 closed = accept MS with authorized = 1, MS with authorized = 0 and all new subscribers should be rejected black-list = accept MS with authorized = 1, MS with authorized = 0 should be rejected, for all new subscribers set authorized = 1 You can see, that "black-list" is like "closed", but in black-list mode we set authorized = 1 for all new subscribers. So the idea was to save meaning of authorized parameter, accept with authorized = 1 and reject with authorized = 0.
Have you considered using an enum like
enum { AUTHORIZED_NOTSET, AUTHORIZED_ALLOWED, AUTHORIZES_BLACKLISTED, };
+ return subscriber->authorized;
return subscriber->authorized != AUTHORIZES_BLACKLISTED?
doesn't look too bad and one avoids the using net inside the db code.
What's the problem to use net inside the db code? Moreover, net is used in db_create_subscriber() function. -- Regards, Ivan Kluchnikov. http://fairwaves.ru
On Wed, Sep 18, 2013 at 09:01:09PM +0400, Ivan Kluchnikov wrote:
On second thought I wonder about the semantic of this patch. E.g. if I run an accept-all network.. and then switch to a closed policy. The authorized field will still be 0. But with this change I can not easily change.
For auth policy clarification: accept-all = accept MS with authorized = 1 and 0, for all new subscribers set authorized = 0 closed = accept MS with authorized = 1, MS with authorized = 0 and all new subscribers should be rejected black-list = accept MS with authorized = 1, MS with authorized = 0 should be rejected, for all new subscribers set authorized = 1
You can see, that "black-list" is like "closed", but in black-list mode we set authorized = 1 for all new subscribers. So the idea was to save meaning of authorized parameter, accept with authorized = 1 and reject with authorized = 0.
My point was that. Currently I can do: 1.) accept-all policy... new subscribers will be allowed to register send/sms/added to the database but their actually authorized=1 2.) I decide to change to closed. All previous subscribers are not allowed in anymore. This means I can change policy without updating the database. I think it would be nice for the black-list too.
What's the problem to use net inside the db code? Moreover, net is used in db_create_subscriber() function.
It is a layering violation. The DB code should know little about the gsm_network. It should just save and restore records. We should assign subscriber->net outside of the code.
Hi Holger,
My point was that. Currently I can do:
1.) accept-all policy... new subscribers will be allowed to register send/sms/added to the database but their actually authorized=1
As I know, we set authorized=0 for all new subscribers by default "authorized INTEGER NOT NULL DEFAULT 0, "
2.) I decide to change to closed. All previous subscribers are not allowed in anymore.
This means I can change policy without updating the database.
Yes, this logic will work after my changes too.
I think it would be nice for the black-list too.
For black-list you can do: 1. accept-all policy. New subscribers will be allowed to register send/sms/added to the database with authorized=0 2. Change to black-list. All previous subscribers are not allowed in anymore. New subscribers will be allowed to register send/sms/added to the database with authorized=1 So the logic, which I described above, is what I really want to implement.
It is a layering violation. The DB code should know little about the gsm_network. It should just save and restore records. We should assign subscriber->net outside of the code.
I think the problem is that db_create_subscriber() function not only saves and restores records, but also creates subscriber. So I believe, that the right way is to add new layer "subscriber" and separate db_create_subscriber() function in two functions like create_subscriber() [subscriber layer] and db_set_subscriber() [db layer]. -- Regards, Ivan Kluchnikov. http://fairwaves.ru
On Thu, Sep 19, 2013 at 04:22:31PM +0400, Ivan Kluchnikov wrote:
Yes, this logic will work after my changes too.
We don't talk the same language here. With your change.. all subscribers that were allowed in during the blacklist mode are now authorized=1. So, if I want to change the policy, I will need to update the database.
I think the problem is that db_create_subscriber() function not only saves and restores records, but also creates subscriber. So I believe, that the right way is to add new layer "subscriber" and separate db_create_subscriber() function in two functions like create_subscriber() [subscriber layer] and db_set_subscriber() [db layer].
makes sense. I plan to clean that up.
Please call it "blacklist" without the dash. Splitting it into two words means a list of the colour black, which is very different from the term blacklist. :) Remember to update the commit message too. Thanks! //Peter
participants (4)
-
Holger Hans Peter Freyther -
Ivan Kluchnikov -
Ivan Kluchnikov -
Peter Stuge