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`?
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/36930?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: I13146a799448d03c681dc868aaa31eb78b7821ff
Gerrit-Change-Number: 36930
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 27 May 2024 20:10:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
