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

historical

/me replying to two mails in one

In summary, the questions I'd like answered, see below for details:
- using memcmp() is ok? (re constant_time_cmp())

On Mon, Sep 28, 2015 at 07:23:56AM +0200, Holger Freyther wrote:
> not just remove the above but provide a short general statement about what this
> protocol should provide.

done, I hope to satisfaction. (patch will follow)

> GSM MAP is a protocol, the MAP Proxy is a system to relay these. You could
> also opt for more generic terms like “Client”/“Server” “Consumer”/“Provider”

Changed to "client"/"server".

> Okay, I just said something else to Harald but adding (and not implementing)
> the following. If the server key matches but the SQN is wrong an AUTS will
> be included by the client?

Spec'd a Sync Request message, but will not implement yet.

> >   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));
> maybe we have two secrets to fill out K and OP(c) with different values?
&
> yes k and opc from two diferent keys would be great.

I can make it one double-length shared secret and use one half each?
Ah whatever, I'll add secret_k and secret_opc, who knows if we'd like to have
an operator specifc key one day...

On Mon, Sep 28, 2015 at 07:56:17AM +0200, Holger Freyther wrote:
> in general this is a bit too much to review in one piece.

you mean the patch is too large... ;)

> > +struct gprs_ipa_client {
> > +	struct ipa_client *ipac;
> 
> In general we subclass by embedding the parent class directly because then

Problem being, we get a pointer from the constructor:

    struct ipa_client *ipa_client_create(const char *ip_addr, [...]

(was gsup_client_create(), i.e. someone else wrote this.)
Should I refactor this to

    void ipa_client_create(struct ipa_client*, ...)

?? I'm trying to limit API refactoring efforts though.

> upper case like all our enums?

ACK

> > +	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?
&
> > +	/* this should probably happen in config parsing place?? */
> 
> yes ;) the VTY commands should create/parse hex

In a subsequent commit on the branch, the const char * has already disappeared
into the vty (patch not posted yet).

> > -/* (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

I have no idea about notation of copyright years... I'll write "2014,2015", then.

> > -			     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)?

Some effort. I had considered it, but decided against it in the end.
Slightly feels like unneccessary effort.  If you need smaller review
chunks though, it adds motivation to further split.

Not sure what you mean by "split with message encoding/decoding", like,
commit the oap_messages.c in a separate commit? IMHO it's already
separate.

> > +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.

Am not clear yet how an error in a protocol callback should propagate to
the main loop and barf there. Certainly something grave should happen.

> > +struct gprs_ipa_client *gprs_ipa_client_create(const char *ip_addr,
> > +					       unsigned int tcp_port)
> > +{
> > 
> > +	return gipac;
> > +
> > +failed:
> 
> 	LOGL_ERROR?

The only possible failure cause is from ipa_client_create(), so *that* one
should log an error instead. But that's "older" code again, and I'm trying to
stay on topic... should I add error logging to the older code??

> > +	struct osmo_sub_auth_data auth = {
> > +		.type		= OSMO_AUTH_TYPE_GSM,
> 
> 								UMTS and not GSM ;)

erm, well, "was just checking if you're paying attention" ^_^

> > +	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));

Seems I had an echo on the 'p' key...

> 
> 
> 
> > 
> > +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

gprs_oap_encode/_decode() can be called directly to test them?  You mean I
should have a separate create_oap_register_msg() function and call that from
gprs_oap_register()? IMHO that's overkill in this instance... The registration
message is part of a unit test already, I see no need to separate it further.
Let me know if I'm wrong.

> > +	switch (oap_msg.message_type) {
> > +	case GPRS_OAP_MSGT_CHALLENGE_REQUEST:
> 
> don’t do this in-place. Please call a function

Don't do what in-place? You mean factor the guts of "case: { ... } break;" out
to a new function?

BTW, I'd be faster grasping the context if you left the entire patch in the
reply... no filename, no linenr... :)

> > +	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?

Yes, code pasting syndrome. I have no idea, would have used memset().
Looks interesting though :P

> > +/* 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/)?

I picked the scope most closely wrapping the two users: the gprs subdir.
"gprs_utils" did sound like the proper kitchen sink for this...

Then again, why not use memcmp()?
(this is also basically code I copied from GSUP)

> > +	if (ipac->updown_cb != NULL)
> > +	      ipac->updown_cb(ipac, up);
> 
> Does it make sense to have the updown_cb be NULL?

If no action upon connecting is required, no cb is needed. Here we'd like to
call the gprs_oap_register() function once IPA is established, see
gprs_ipa_client_updown_cb(). For example, GSUP doesn't need an updown_cb.

Note, the "bare IPA client" timers etc. are handled one level deeper, in the
function that calls the ipac->updown_cb(): ipa_client_updown_cb().

> > 
> > -	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?

Hmmm. Is it? I'd love to assumed that there is always both a PROTO and
PROTO_EXT value available... ipaccess_head_ext is defined in ipaccess.h, so is
it really specific to 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 /* */

hrm. It's so much more effort :P
I often disagree on how people use commenting styles...
anyway, I'll submit to local rule. Is it: *never* use '//' ?

> But why do we add the comment
> what changed from the previous implementation?

Without the change, l2h pointed at the wrong index and the messages could not
be decoded in the test. Re-reading it now, though, makes me think that the
order may be wrong entirely.

Last time I concluded that this causes l2h to point at the proto_ext header
instead of the payload:

        ipa_prepend_header_ext(msg, proto);
        ipa_msg_push_header(msg, proto_ext)

Like: ipa_prepend_header_ext() prepends the proto_ext byte, and then
ipa_msg_push_header() again prepends the "Osmo" protocol header but makes l2h
point at the ipa_prepend_header_ext() part.

Only, just now I noticed that the proto and proto_ext seem to be swapped.

I'll take another look at it, wait for next patch mail...

> “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

good to know.

> * “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.

Was not aware of it, my name choice based on the naive assumption that there is
just the one IPA connection.

But we're also talking OAP on that IPA line.
Can I call it gprs_map_proxy_client, plz?

> > -/* 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..

wasn't me, was it. I just copied it around. I guess I'll use memcmp() and leave
constant_time_cmp() where it belongs: a dark corner of a static C file context.

> > 	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.

Hey, how did that sneak in here? That patch was committed separately on master.
I must have mixed up a branchpoint, this change was supposed to be separate.

> > 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.

come on, just adding entries to a Makefile? ;)
And the tests, true.

Admitted, I have enough to polish as it is.
I'll come back with smaller chunks, if the effort remains reasonable.

~Neels

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.osmocom.org/pipermail/openbsc/attachments/20150929/45635c29/attachment.bin>