Attention is currently required from: dexter, laforge.

fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/36930?usp=email )

Change subject: CardKeyProvider: Implement support for column-based transport key encryption ......................................................................

Patch Set 1: Code-Review+1

(7 comments)

Patchset:

PS1: None of my comments are merge blockers, mostly cosmetics.

File contrib/csv-encrypt-columns.py:

https://gerrit.osmocom.org/c/pysim/+/36930/comment/23e659d5_7fcba862 PS1, Line 29: def dict_keys_to_upper(d: dict): typing hint: `-> dict:`

https://gerrit.osmocom.org/c/pysim/+/36930/comment/422e01a1_fe6d086e PS1, Line 37: def encrypt_col(self, colname:str, value: str): typing hint: `-> Hexstr:`?

https://gerrit.osmocom.org/c/pysim/+/36930/comment/b16674bf_85935630 PS1, Line 43: def encrypt(self): type hint: `-> None:`

https://gerrit.osmocom.org/c/pysim/+/36930/comment/561b5356_52ed67f2 PS1, Line 60: parser.add_argument('CSVFILE', help="CSV file name") Idea: you can do `type=argparse.FileType('r')`, so that argparse will `open()` the file for you and fail early if it does not exist. Not sure when it gets `close()d` though (perhaps automatically when the script finishes?). As a bonus, a special value `-` will automatically open `stdin`, but in that case the `self.filename + '.encr` logic above will no longer work.

Maybe generalize this even further, allowing to specify the output file and using `stdin` / `stdout` by default?

``` parser.add_argument('-i', metavar='FILE', type=argparse.FileType('r'), default=sys.stdin, help="Input CSV file to be encrypted") parser.add_argument('-o', metavar='FILE', type=argparse.FileType('w'), default=sys.stdout, help="Output CSV file") ```

https://gerrit.osmocom.org/c/pysim/+/36930/comment/11e2dd34_48cfb2c5 PS1, Line 61: default=[], I would remove `default` and put `required=True`, since you require at least one argument.

File pySim/card_key_provider.py:

https://gerrit.osmocom.org/c/pysim/+/36930/comment/4e88736f_0a5f7721 PS1, Line 129: str) -> str `Hexstr`?