[PATCH 3/3] Implement OAP for SGSN registration.

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.

Holger Freyther holger at freyther.de
Mon Sep 28 05:56:17 UTC 2015


> On 24 Sep 2015, at 13:44, Neels Hofmeyr <nhofmeyr at sysmocom.de> wrote:


Good Morning,

in general this is a bit too much to review in one piece.


> +struct gprs_ipa_client {
> +	struct ipa_client *ipac;

In general we subclass by embedding the parent class directly because then
you can do upwards casts very easily and can leave the ->data pointer for
something else?


> 
> +/* This is the config part for vty. It is essentially copied in gprs_oap_state,
> + * where values are copied over once the config is considered valid. The shared
> + * secret is converted from hex string to octet buffer, the sgsn_id is simply
> + * copied. Is this separation really necessary? */
> +struct gprs_oap_config {
> +	uint16_t sgsn_id;
> +	const char *shared_secret;
> +};

maybe we have two secrets to fill out K and OP(c) with different values?



> +
> +struct gprs_oap_state {
> +	enum {
> +		oap_uninitialized = 0,	// just allocated.
> +		oap_disabled,		// disabled by config.
> +		oap_config_error, // <-- TODO really?
> +		oap_initialized,	// shared_secret valid.
> +		oap_requested_challenge,
> +		oap_sent_challenge_result,
> +		oap_registered
> +	} state;

upper case like all our enums?


> +	uint16_t sgsn_id;
> +	uint8_t shared_secret[16];

good point. So here it is 16bytes and up there it is a const char *. Maybe use
uint8_t everywhere and then re-write the secrets as hexdump?

> 
> -/* (C) 2014 by Sysmocom s.f.m.c. GmbH
> +/* (C) 2015 by Sysmocom s.f.m.c. GmbH

Domain of copy-right. Is this 2014-2015, 2014,2015? Probably more like
2014,2015 here (yes, the bits from 2014 will expire at some point


> -			     encode_big_endian(pdp_info->pdp_type | 0xf000,
> -					       GPRS_GSUP_PDP_TYPE_SIZE));
> +			     gprs_encode_big_endian(pdp_info->pdp_type | 0xf000,
> +						    GPRS_GSUP_PDP_TYPE_SIZE));

how much effort would it be to split this part and the ipa->gsup client part
in two other commits and then adding OAP (even split up with message
encoding/decoding)?


> 
> +static void gprs_ipa_client_updown_cb(struct ipa_client *ipac, int up)
> +{
> +	struct gprs_ipa_client *gipac = ipac->data;
> +
> +	if (up && (gipac->oap.sgsn_id != 0)) {
> +		if (gprs_oap_register(gipac) < 0) {
> +			/* TODO: fail fatally */



			log an error at least.
> 
> +struct gprs_ipa_client *gprs_ipa_client_create(const char *ip_addr,
> +					       unsigned int tcp_port)
> +{
> 
> +	return gipac;
> +
> +failed:

	LOGL_ERROR?

> +	gprs_ipa_client_destroy(gipac);
> +	return NULL;
> +}
> +
> 

> +int gprs_oap_init(struct gprs_oap_config *config, struct gprs_oap_state *state)
> +{
> +	OSMO_ASSERT(state->state == oap_uninitialized);
> +
> +	if (config->sgsn_id == 0)
> +		goto disable;
> +
> +	if (!(config->shared_secret) || (strlen(config->shared_secret) == 0))
> +		goto disable;
> +
> +	/* this should probably happen in config parsing place?? */

yes ;) the VTY commands should create/parse hex


> +	int secret_len = osmo_hexparse(config->shared_secret,
> +				       state->shared_secret,
> +				       sizeof(state->shared_secret));
> 


> +	struct osmo_sub_auth_data auth = {
> +		.type		= OSMO_AUTH_TYPE_GSM,

								UMTS and not GSM ;)


> +		.algo		= OSMO_AUTH_ALG_MILENAGE,
> +	};
> +
> +	OSMO_ASSERT(sizeof(auth.u.umts.opc) == sizeof(state->shared_secret));
> +	OSMO_ASSERT(sizeof(auth.u.umts.k) == sizeof(state->shared_secret));
> +
> +	memcpy(auth.u.umts.opc, state->shared_secret, sizeof(auth.u.umts.opc));



> +	memcpy(auth.u.umts.k, state->shared_secret, sizeof(auth.u.umts.k));
> +	memcpy(auth.u.umts.k, state->shared_secret, sizeof(auth.u.umts.k));

one copy too many. What did you try to create? But yes k and opc from two diferent
keys would be great.



> 
> +int gprs_oap_register(struct gprs_ipa_client *gipac)
> +{
> +	struct gprs_oap_state *state = &gipac->oap;
> +
> +	OSMO_ASSERT(state);
> +	OSMO_ASSERT(state->sgsn_id);
> +
> +	struct msgb *msg = ipa_client_msgb_alloc();
> +
> +	struct gprs_oap_message oap_msg = {0};
> +	oap_msg.message_type = GPRS_OAP_MSGT_REGISTER_REQUEST;
> +	oap_msg.sgsn_id = state->sgsn_id;
> +
> +	gprs_oap_encode(msg, &oap_msg);
> +
> +	state->state = oap_requested_challenge;
> +	return gprs_ipa_client_send_oap(gipac, msg);
> +}

separating encoding and sending will allow us to unittest the
encoder/decoder separately

> 
> +	switch (oap_msg.message_type) {
> +	case GPRS_OAP_MSGT_CHALLENGE_REQUEST:


don’t do this in-place. Please call a function


> +int gprs_oap_decode(const uint8_t *const_data, size_t data_len,
> +		    struct gprs_oap_message *oap_msg)
> +{
> 

> +	static const struct gprs_oap_message empty_oap_message = {0};
> +
> +	*oap_msg = empty_oap_message;

why do we need to copy this? An alternative to memset? Taken from gsup
code?


> +/* Wishful thinking to generate a constant time compare */
> +int gprs_constant_time_cmp(const uint8_t *exp, const uint8_t *rel, const int count)
> +{

don’t copy and paste from the NAT but at least put it into a commonly used
function (e.g. somewhere in libcommom/)?


> +	if (ipac->updown_cb != NULL)
> +	      ipac->updown_cb(ipac, up);

Does it make sense to have the updown_cb be NULL?

> 
> -	if (hh->proto != IPAC_PROTO_OSMO)
> -		goto invalid;
> -
> -	if (!he || msgb_l2len(msg) < sizeof(*he) ||
> -	    he->proto != IPAC_PROTO_EXT_GSUP)
> +	if (!he || msgb_l2len(msg) < sizeof(*he))
> 		goto invalid;

I think you want to have the first hh->proto check here. As the second part will
use he which is only valid/there if it is IPAC_PROTO_OSMO?
> 

> +	// l2h is not sent over the wire, but for the test suite it makes sense
> +	// to make l2h point at the IPA message payload.

Please don’t use C99 comments, e.g. use /* */ But why do we add the comment
what changed from the previous implementation?

> );
> -	if (g_cfg->gsup_server_addr.sin_addr.s_addr)
> -		vty_out(vty, " gsup remote-ip %s%s",
> -			inet_ntoa(g_cfg->gsup_server_addr.sin_addr), VTY_NEWLINE);
> -	if (g_cfg->gsup_server_port)
> -		vty_out(vty, " gsup remote-port %d%s",
> -			g_cfg->gsup_server_port, VTY_NEWLINE);
> +	if (g_cfg->ipa_server_addr.sin_addr.s_addr)
> +		vty_out(vty, " ipa remote-ip %s%s",
> +			inet_ntoa(g_cfg->ipa_server_addr.sin_addr), VTY_NEWLINE);
> +	if (g_cfg->ipa_server_port)
> +		vty_out(vty, " ipa remote-port %d%s",
> +			g_cfg->ipa_server_port, VTY_NEWLINE);

“gsup” vs. “ipa”.

* If you change config files you need to provide an upgrade path from a config file
that is using “gsup”. E.g. by adding a DEFUN_DEPRECATED for the old parameter

* “ipa” is such a vague name. The SGSN might have several IPA connections to
different kind of systems. One might be used for GSUP. Another one might be for
sending telemetric data. We use this connection for GSUP so let’s keep the current
configuration settings.

And this you probably don’t want the s/gsup_client/gprs_ipa_client/ renaming as
well. As we might have multiple IPA connections in the future to different systems.
> 
> -/* Wishful thinking to generate a constant time compare */
> -static int constant_time_cmp(const uint8_t *exp, const uint8_t *rel, const int count)


ah ha, but then please don’t call it gprs_constant_time..


> 
> static int verify_key(struct bsc_connection *conn, struct bsc_config *conf, const uint8_t *key, const int keylen)
> {
> 
> 	if (vec.res_len != 8) {
> 		LOGP(DNAT, LOGL_ERROR, "Res length is wrong: %d for bsc nr %d\n",
> -			keylen, conf->nr);
> +			(int)vec.res_len, conf->nr);

separate commit. And what type is res_len? size_t? then please change the format
identifier.



> static void ipaccess_auth_bsc(struct tlv_parsed *tvp, struct bsc_connection *bsc)
> diff --git a/openbsc/tests/sgsn/Makefile.am b/openbsc/tests/sgsn/Makefile.am
> index ea29fce..3b45b3f 100644
> --- a/openbsc/tests/sgsn/Makefile.am
> +++ b/openbsc/tests/sgsn/Makefile.am

I am afraid you really need to split this change a bit more. My attention span is over
here.





More information about the OpenBSC mailing list