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
-- To view, visit https://gerrit.osmocom.org/c/osmo-tetra/+/33943 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-tetra Gerrit-Branch: master Gerrit-Change-Id: I63bc712630ae5dbaa049c129d456f7aef5bda863 Gerrit-Change-Number: 33943 Gerrit-PatchSet: 4 Gerrit-Owner: wbokslag <w.bokslag@midnightblue.nl> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <laforge@osmocom.org> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Comment-Date: Thu, 27 Jul 2023 16:16:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge@osmocom.org> Gerrit-MessageType: comment