Attention is currently required from: laforge.

wbokslag has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-tetra/+/33943 )

Change subject: Added first steps towards tetra crypto support ......................................................................

Patch Set 4:

(9 comments)

Patchset:

PS4: Thanks for the remarks, I simplified some stuff, and am waiting for your opinion on merging tetra_crypto_state into tetra_mac_state while keeping tetra_crypto_database a global.

File src/crypto/tetra_crypto.h:

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/a653f2ce_7e63b875 PS3, Line 47: mcc

it's been a long time since I read TETRA specs. […]

ETSI EN 300 392-1 V1.4.1 Clause 7.2.5 states:

The MNC is coded in binary using 14 bits. The range of MNC shall be limited to four decimal digits i.e. the maximum value is 9 999 decimal or 270F in hexadecimal format.

This does not in any way seem to be a character based representation so I think we're good here.

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/01b4abdd_13b0be75 PS3, Line 54: int is_gssi_range; : int start; : int end;

are there situations where those integers really can be negative?

Since I'm not really using address ranges I've decided to remove them from this patch. We can re-introduce those when needed.

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/82e93cb4_6ba25440 PS3, Line 61: mnc

is there a reeason to use int for mnc/mcc in 'struct network_info' but uint32_t here?

You're right, I changed them to uint32_t in the network info struct

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/7c6b69cd_98946b5a PS3, Line 71: int num_keys;

I guess all of these could be unsigned as they simply count the number of list elements? Using the […]

Done

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/07b52ca7_952ac2f5 PS3, Line 93: const char *tetra_get_key_type_name(int key_type);

didn't you define enums above for those types? Wondering why the argument type is int instead of the […]

Done

File src/crypto/tetra_crypto.c:

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/9b060a51_c0b8d002 PS3, Line 14: struct tet

I'd generally suggest to a void global variables for state that might exist multiple times (think of […]

I introduce two new global variables. How would you feel about merging the tetra_crypto_state into the tetra_mac_state? Since the MAC handles encryption, it would not be out of place.

The second question then remains the tetra_crypto_database which serves as a database that stores user-supplied network specifications and key material. I think this neither fits in the tetra_mac_state, nor does it seem appropriate to pass it around through all the functions that are involved.

To me, it seems okay to make the tetra_crypto_database a global since it is reasonable to assume one would want to use it across multiple tetra_mac_states. Agree?

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/92efdc1d_85f4d289 PS3, Line 82: calloc

we generally use talloc in the existing osmo-tetra code base, and it would make sense to not mix dif […]

Done

https://gerrit.osmocom.org/c/osmo-tetra/+/33943/comment/b5ee42d5_5893be8a PS3, Line 96: k->mcc, k->mnc, k->key_type);

we have something like OSMO_STRBUF_APPEND() in osmocom/core/utils. […]

I'll leave it as-is for now