| Message ID | 1459975492-74488-1-git-send-email-holger@freyther.de |
|---|---|
| State | Not Applicable |
| Headers | show |
On Wed, Apr 06, 2016 at 10:44:52PM +0200, Holger Hans Peter Freyther wrote: > + > + /* handle optional ciphering */ > + if (!alg || strcasecmp(alg, "none") == 0) > + db_sync_authinfo_for_subscr(NULL, subscr); > + else { > + struct gsm_auth_info ainfo = { 0, }; > + /* the verify should make sure that this is okay */ > + OSMO_ASSERT(alg); > + OSMO_ASSERT(ki); > + > + if (strcasecmp(alg, "xor") == 0) > + ainfo.auth_algo = AUTH_ALGO_XOR; > + else if (strcasecmp(alg, "comp128v1") == 0) > + ainfo.auth_algo = AUTH_ALGO_COMP128v1; > + > + rc = osmo_hexparse(ki, ainfo.a3a8_ki, sizeof(ainfo.a3a8_ki)); > + if (rc < 0) { > + subscr_put(subscr); > + talloc_free(tmp); > + cmd->reply = "Failed to parse KI"; > + return CTRL_CMD_ERROR; > + } > + > + ainfo.a3a8_ki_len = rc; > + db_sync_authinfo_for_subscr(&ainfo, subscr); > + rc = 0; > + } > + > + db_sync_lastauthtuple_for_subscr(NULL, subscr); You didn't bump to v2 because it is supposedly backwards compatible to subscriber-modify-v1 without auth info, right? But consider that a subscriber-modify-v1 will now clear out the auth info when the newly added parameters are omitted. So it's not strictly backwards compatible, right? With the previous v1 I could only modify IMSI and MSISDN and leave algo and KI untouched. After this patch I will always either have to pass algo+KI again or they will be cleared out. Also, this will always clear out the lastauthtuple, regardless of the auth info being changed or not. Even if I again pass the same algo+KI as were stored previously. OTOH clearing might be good when the IMSI has changed?? :P Maybe it's better / less complex to have a separate command for auth or bump to v2... > + r = self.do_set('subscriber-modify-v1', '2620345,445566,xor') > + self.assertEquals(r['mtype'], 'ERROR') > + self.assertEquals(r['error'], 'Value failed verification.') I'm not familiar with the ctrl interface, but couldn't the error message be more descriptive, like "missing KI argument"? log msg: > The algorithm and ki are considered optional but if a valid > and non none algorithm is passed, a KI must be passed as well. rather say 'not-none' or 'a valid algorithm other than "none"'. ~Neels
> On 07 Apr 2016, at 02:52, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote: > > > You didn't bump to v2 because it is supposedly backwards compatible to > subscriber-modify-v1 without auth info, right? right. The v1 testcase is still passing and the customer using this interface will not notice any change. > Also, this will always clear out the lastauthtuple, regardless of the auth info > being changed or not. Even if I again pass the same algo+KI as were stored > previously. OTOH clearing might be good when the IMSI has changed?? :P IMSI has changed == new subscriber. Also for a CTRL interface only case we could not have set the keys before, so clearing the auth tuples has no effect. But anyway, I have sent a v2 that moves this handling into an if so there is no semantic change for the current usage of the interface. > > Maybe it's better / less complex to have a separate command for auth or bump to > v2... I didn't want to open the can of deprecation and removing old commands yet. The v1 behavior is tested (the test should actually check the DB for the state or such but that is a different topic). I found it easier to add to the existing command and based on the ctrl test I am confident to not break the users of that interface. >> + r = self.do_set('subscriber-modify-v1', '2620345,445566,xor') >> + self.assertEquals(r['mtype'], 'ERROR') >> + self.assertEquals(r['error'], 'Value failed verification.') > > I'm not familiar with the ctrl interface, but couldn't the error message be > more descriptive, like "missing KI argument"?\ The CTRL command has a READ, VERIFY and WRITE method. VERIFY will be executed before WRITE. The VERIFY method can only return yes or no. We can add improving/extending the VERIFY to our backlog. > log msg: >> The algorithm and ki are considered optional but if a valid >> and non none algorithm is passed, a KI must be passed as well. > > rather say 'not-none' or 'a valid algorithm other than "none"'. fixed locally but that is not in the v2 patch holger
diff --git a/openbsc/src/libmsc/ctrl_commands.c b/openbsc/src/libmsc/ctrl_commands.c index e48c6a3..566240c 100644 --- a/openbsc/src/libmsc/ctrl_commands.c +++ b/openbsc/src/libmsc/ctrl_commands.c @@ -24,9 +24,25 @@ #include <openbsc/db.h> #include <openbsc/debug.h> +static bool alg_supported(const char *alg) +{ + /* + * TODO: share this with the vty_interface and extend to all + * algorithms supported by libosmocore now. Make it table based + * as well. + */ + if (strcasecmp(alg, "none") == 0) + return true; + if (strcasecmp(alg, "xor") == 0) + return true; + if (strcasecmp(alg, "comp128v1") == 0) + return true; + return false; +} + static int verify_subscriber_modify(struct ctrl_cmd *cmd, const char *value, void *d) { - char *tmp, *imsi, *msisdn, *saveptr = NULL; + char *tmp, *imsi, *msisdn, *alg, *ki, *saveptr = NULL; int rc = 0; tmp = talloc_strdup(cmd, value); @@ -35,6 +51,8 @@ static int verify_subscriber_modify(struct ctrl_cmd *cmd, const char *value, voi imsi = strtok_r(tmp, ",", &saveptr); msisdn = strtok_r(NULL, ",", &saveptr); + alg = strtok_r(NULL, ",", &saveptr); + ki = strtok_r(NULL, ",", &saveptr); if (!imsi || !msisdn) rc = 1; @@ -42,6 +60,12 @@ static int verify_subscriber_modify(struct ctrl_cmd *cmd, const char *value, voi rc = 1; else if (strlen(msisdn) >= GSM_EXTENSION_LENGTH) rc = 1; + else if (alg) { + if (!alg_supported(alg)) + rc = 1; + else if (strcasecmp(alg, "none") != 0 && !ki) + rc = 1; + } talloc_free(tmp); return rc; @@ -56,7 +80,7 @@ static int get_subscriber_modify(struct ctrl_cmd *cmd, void *data) static int set_subscriber_modify(struct ctrl_cmd *cmd, void *data) { struct gsm_network *net = cmd->node; - char *tmp, *imsi, *msisdn, *saveptr = NULL; + char *tmp, *imsi, *msisdn, *alg, *ki, *saveptr = NULL; struct gsm_subscriber* subscr; int rc; @@ -66,6 +90,8 @@ static int set_subscriber_modify(struct ctrl_cmd *cmd, void *data) imsi = strtok_r(tmp, ",", &saveptr); msisdn = strtok_r(NULL, ",", &saveptr); + alg = strtok_r(NULL, ",", &saveptr); + ki = strtok_r(NULL, ",", &saveptr); subscr = subscr_get_by_imsi(net->subscr_group, imsi); if (!subscr) @@ -80,6 +106,35 @@ static int set_subscriber_modify(struct ctrl_cmd *cmd, void *data) /* put it back to the db */ rc = db_sync_subscriber(subscr); db_subscriber_update(subscr); + + /* handle optional ciphering */ + if (!alg || strcasecmp(alg, "none") == 0) + db_sync_authinfo_for_subscr(NULL, subscr); + else { + struct gsm_auth_info ainfo = { 0, }; + /* the verify should make sure that this is okay */ + OSMO_ASSERT(alg); + OSMO_ASSERT(ki); + + if (strcasecmp(alg, "xor") == 0) + ainfo.auth_algo = AUTH_ALGO_XOR; + else if (strcasecmp(alg, "comp128v1") == 0) + ainfo.auth_algo = AUTH_ALGO_COMP128v1; + + rc = osmo_hexparse(ki, ainfo.a3a8_ki, sizeof(ainfo.a3a8_ki)); + if (rc < 0) { + subscr_put(subscr); + talloc_free(tmp); + cmd->reply = "Failed to parse KI"; + return CTRL_CMD_ERROR; + } + + ainfo.a3a8_ki_len = rc; + db_sync_authinfo_for_subscr(&ainfo, subscr); + rc = 0; + } + + db_sync_lastauthtuple_for_subscr(NULL, subscr); subscr_put(subscr); talloc_free(tmp); diff --git a/openbsc/tests/ctrl_test_runner.py b/openbsc/tests/ctrl_test_runner.py index 21850e3..7a12643 100644 --- a/openbsc/tests/ctrl_test_runner.py +++ b/openbsc/tests/ctrl_test_runner.py @@ -469,6 +469,33 @@ class TestCtrlNITB(TestCtrlBase): self.assertEquals(r['var'], 'number-of-bts') self.assertEquals(r['value'], '1') + def testSubscriberAddWithKi(self): + """Test that we can set the algorithm to none, xor, comp128v1""" + + r = self.do_set('subscriber-modify-v1', '2620345,445566') + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'subscriber-modify-v1') + self.assertEquals(r['value'], 'OK') + + r = self.do_set('subscriber-modify-v1', '2620345,445566,none') + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'subscriber-modify-v1') + self.assertEquals(r['value'], 'OK') + + r = self.do_set('subscriber-modify-v1', '2620345,445566,xor') + self.assertEquals(r['mtype'], 'ERROR') + self.assertEquals(r['error'], 'Value failed verification.') + + r = self.do_set('subscriber-modify-v1', '2620345,445566,comp128v1,00112233445566778899AABBCCDDEEFF') + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'subscriber-modify-v1') + self.assertEquals(r['value'], 'OK') + + r = self.do_set('subscriber-modify-v1', '2620345,445566,none') + self.assertEquals(r['mtype'], 'SET_REPLY') + self.assertEquals(r['var'], 'subscriber-modify-v1') + self.assertEquals(r['value'], 'OK') + def testSubscriberAddRemove(self): r = self.do_set('subscriber-modify-v1', '2620345,445566') self.assertEquals(r['mtype'], 'SET_REPLY')
From: Holger Hans Peter Freyther <holger@moiji-mobile.com> Extend the existing ctrl command to be able to specify the algorithm and Ki. In contrast to the VTY no size check is done. Together with the VTY this code only supports a small part of what is supported by libosmocore. The algorithm and ki are considered optional but if a valid and non none algorithm is passed, a KI must be passed as well. Extend the test coverage by passing the potential values. It is not verified that the KI/algorithm is stored. --- openbsc/src/libmsc/ctrl_commands.c | 59 ++++++++++++++++++++++++++++++++++++-- openbsc/tests/ctrl_test_runner.py | 27 +++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-)