Attention is currently required from: pespin, Christian Amsüss, dexter. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/29033 )

Change subject: Add new pySim.ota library, implement SIM OTA crypto ......................................................................

Patch Set 6:

(9 comments)

File pySim/ota.py:

https://gerrit.osmocom.org/c/pysim/+/29033/comment/57493321_f2f0ac00 PS4, Line 44: S 102 225 Table 5 : ota_status_codes = bidict({ : 0x00: 'PoR OK', : 0x01: 'RC/CC/DS failed', : 0x02: 'CNTR low', : 0x03: 'CNTR high', : 0x04: 'CNTR blocked', : 0x05: 'Ciphering error', : 0x06: 'Unidentified security error', : 0x07: 'Insufficient memory', : 0x08: 'more time', : 0x09: 'TAR unknown', : 0x0a: 'Insufficient security level', : 0x0b: 'Actual Response in SMS-SUBMIT', # 31.115 : 0x0c: 'Actual Response in USSD', # 31.115 : })

This bidict is redundant with ResponseStatus, and not used anywhere. […]

yes, that can be removed. It goes back to some very old first attempts of implementing this months if not years ago.

https://gerrit.osmocom.org/c/pysim/+/29033/comment/dcfd2648_d6a0b5e4 PS4, Line 120: algo_auth: str, kid_idx: int, kid: bytes, cntr: int = 0):

I'm very suspicious of the counter having zero as a default; the symmetric algorithm is already init […]

I understand the point. the rationale here was that in 99% of all current use cases we won't be using the counter, as we don't have any central database/backend where we would store the current per-card OTA counter. Also, 99% of the sysmoISIM-SJA2 users (which are probably >95% of our overall user base for this feature) have cards with a MSL not requiring counters, as they are used in a research/development setting and if you have multiple different programs talking OTA over time without any centralized counter storage...

File pySim/ota.py:

https://gerrit.osmocom.org/c/pysim/+/29033/comment/ac77469f_e7816b5b PS5, Line 347: apdu += otak.crypt._get_padding(len_cipher, otak.crypt.blocksize)

This should also set pad_cnt. […]

your patch was cherry-picked + squashed, thanks.

https://gerrit.osmocom.org/c/pysim/+/29033/comment/ebf6a889_70f16a20 PS5, Line 402: def decode_resp(self, otak: OtaKeyset, spi: dict, data: bytes) -> bytes:

`-> bytes` is currently inaccurate, should be `CompactRemoteResp` (but see below)

Done

https://gerrit.osmocom.org/c/pysim/+/29033/comment/934b91bf_385bd7e8 PS5, Line 416: res = self.SmsResponsePacket.parse(remainder)

Do we trust this parsing step enought, to […]

I'm not sure I understand you here. Why would it be bad if that call would raise some exception, which in turn propagetes further upt to the caller of decode_resp() ?

https://gerrit.osmocom.org/c/pysim/+/29033/comment/c79cdf29_b9e4bdb9 PS5, Line 443: cc = otak.auth.check_sig(temp_data, res['cc_rc'])

Needless assign; check_sig is merely called for the exception it'd raise.

ack, fixing it in my next version.

https://gerrit.osmocom.org/c/pysim/+/29033/comment/85d113f1_874c01f6 PS5, Line 452: return dec

There will need to be an API change later here, as in cases of `res. […]

Done

File pySim/sms.py:

https://gerrit.osmocom.org/c/pysim/+/29033/comment/a109665c_65c7f816 PS5, Line 30: ie_c = Struct('offset'/Tell, 'iei'/Int8ub, 'length'/Int8ub, 'data'/Bytes(this.length))

There is an updated version of this in your patch series, which is easier to understand and removes […]

updated version pushed.

https://gerrit.osmocom.org/c/pysim/+/29033/comment/00bc2456_49a7dd02 PS5, Line 39: def __str__(self) -> str:

I think this would be more useful as `__repr__`, especially as it does produce an expression that'd […]

squashed your related patch into my patch