Attention is currently required from: dexter, laforge. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/35488?usp=email ) Change subject: Add pySim.esim.bsp module implementing BSP (BPP Protection Protocol) ...................................................................... Patch Set 4: (13 comments) Patchset: PS4: None of my comments are critical, mostly nitpicks. File pySim/esim/bsp.py: https://gerrit.osmocom.org/c/pysim/+/35488/comment/ba37f028_2a518555 PS4, Line 46: blocksize You can simply do a type declaration here: ``` blocksize: int ``` without assigning it to `None`. This is the usual way to declare a property, which is expected to be assigned either statically or by the `__init__()` function. https://gerrit.osmocom.org/c/pysim/+/35488/comment/827c94a6_06d8b2da PS4, Line 47: enum_name unused? https://gerrit.osmocom.org/c/pysim/+/35488/comment/ff19b899_9278e46b PS4, Line 60: __init__ can be removed? https://gerrit.osmocom.org/c/pysim/+/35488/comment/df6270e6_442a67f9 PS4, Line 95: abc.abstractmethod why commented out? https://gerrit.osmocom.org/c/pysim/+/35488/comment/63f83df4_b9453c92 PS4, Line 104: _get_padding Can we re-use the parent's implementation here? ``` return b'\x80' + super()._get_padding(in_len + 1, multiple, padding) ``` https://gerrit.osmocom.org/c/pysim/+/35488/comment/1465d984_6ced45e1 PS4, Line 124: _get_icv Can we pre-calculate the ICV once (e.g. in `__init__()`) and then just use it in `self._{en,de}crypt()`? Or is it supposed to change during the lifetime of a class instance? https://gerrit.osmocom.org/c/pysim/+/35488/comment/2ac9f9bd_96f1a281 PS4, Line 132: % (self.block_nr, b2h(data), b2h(icv JFYI: when using Python's logging, you can pass format string arguments directly to the logging function (without having to render the string using `"..." % (args...)`). This allows for lazy evaluation. https://gerrit.osmocom.org/c/pysim/+/35488/comment/53a536fa_c2a35705 PS4, Line 153: tag <= 255 This still permits negative values, maybe `assert tag in range(256)`? https://gerrit.osmocom.org/c/pysim/+/35488/comment/db5b289f_daa33ea0 PS4, Line 299: return b''.join(plaintext_list) Maybe use list-comprehension here? ``` return b''.join([self.demac_and_decrypt_one(x) for x in ciphertext_list]) ``` https://gerrit.osmocom.org/c/pysim/+/35488/comment/5575602b_efc53722 PS4, Line 310: return b''.join(plaintext_list) Maybe use list-comprehension here? ``` return b''.join([self.demac_only_one(x) for x in ciphertext_list]) ``` File tests/test_esim_bsp.py: https://gerrit.osmocom.org/c/pysim/+/35488/comment/72ff545f_1ab12b90 PS4, Line 62: segment0 = h2b('a048800102810101821a53494d616c6c69616e63652053616d706c652050726f66696c65830a89000123456789012341a506810084008b00a610060667810f010201060667810f010204b08201f8a0058000810101810667810f010201a207a105c60301020aa305a1038b010fa40c830a98001032547698103214a527a109820442210026800198831a61184f10a0000000871002ff33ff01890000010050045553494da682019ea10a8204422100258002022b831b8001019000800102a406830101950108800158a40683010a95010882010a8316800101a40683010195010880015aa40683010a95010882010f830b80015ba40683010a95010882011a830a800101900080015a970082011b8316800103a406830101950108800158a40683010a95010882010f8316800111a40683010195010880014aa40683010a95010882010f8321800103a406830101950108800158a40683010a950108840132a4068301019501088201048321800101a406830101950108800102a406830181950108800158a40683010a950108820104831b800101900080011aa406830101950108800140a40683010a95010882010a8310800101900080015aa40683010a95010882011583158001019000800118a40683010a95010880014297008201108310800101a40683010195010880015a97008201158316800113a406830101950108800148a40683010a95010882010f830b80015ea40683010a95010882011a83258001019000800102a010a406830101950108a406830102950108800158a40683010a950108a33fa0058000810102a13630118001018108303030303030303082020099300d800102810831323334353637383012800200818108313233343536373882020088a241a0058000810103a138a0363010800101810831323334ffffffff8201013010800102810830303030ffffffff820102301080010a810835363738ffffffff830101a182029ea0058000810104a18202933082028f62228202782183027ff18410a0000000871002ff33ff0189000001008b010ac60301810a62118202412183026f078b01028001098801388109082943019134876765621482044221002583026f068b010a8801b8c7022f06621a8202412183026f088b0105800121880140a507c00180c10207ff621a8202412183026f098b0105800121880148a507c00180c10207ff62168202412183026f318b0102800101880190a503c1010a62118202412183026f388b010280010e880120810d0a2e178ce73204000000000000621982044221001a83026f3b8b0108800202088800a504c10200ff62198204422100b083026f3c8b0105800206e08800a504c10200ff621282044221002683026f428b0105800126') not critical, but maybe use multi-line strings (`''' ... '''`) for those large samples? https://gerrit.osmocom.org/c/pysim/+/35488/comment/bfcba2bc_8748f3c1 PS4, Line 72: cosmetic: tabs vs spaces -- To view, visit https://gerrit.osmocom.org/c/pysim/+/35488?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: Ic461936f2e68e1e6f7faab33d06acf3063e261e7 Gerrit-Change-Number: 35488 Gerrit-PatchSet: 4 Gerrit-Owner: laforge <laforge@osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier@sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Attention: dexter <pmaier@sysmocom.de> Gerrit-Comment-Date: Sat, 06 Jan 2024 19:57:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment