Attention is currently required from: Hoernchen.

dexter has posted comments on this change by Hoernchen. ( https://gerrit.osmocom.org/c/pysim/+/40464?usp=email )

Change subject: smdpp: add proper tls support, cert generation FOR TESTING ......................................................................

Patch Set 5: Code-Review+1 Verified+1

(8 comments)

Patchset:

PS5: I have tried the patchset and it works with SSL and with --nossl + nginx. I have tried the generate_smdp_certs.py script. It generates certificates but they don't work. I guess this is expected as it is intended only for testing.

File contrib/generate_smdpp_certs.py:

https://gerrit.osmocom.org/c/pysim/+/40464/comment/bb007d48_299cdf51?usp=ema... : PS5, Line 2: To me this looks like an independent tool. Maybe putting it in a separate patch would make sense?

File osmo-smdpp.py:

https://gerrit.osmocom.org/c/pysim/+/40464/comment/a83bdf3b_75f41048?usp=ema... : PS5, Line 26: from pathlib import Path You have moved a lot of those imports from another code location. Maybe it would make sense to first move the imports in a patch before this one and then do the modifications?

https://gerrit.osmocom.org/c/pysim/+/40464/comment/9285f4f8_bc9ab8b4?usp=ema... : PS5, Line 53: We now support SSL and we have means to point to different certificate sub directories. Maybe we should make those two values configurable as well?

https://gerrit.osmocom.org/c/pysim/+/40464/comment/b4e22531_288cac31?usp=ema... : PS5, Line 586: parser.add_argument("-p", "--port", help="TCP port to bind HTTP to", default=8000) Maybe 8443, since we now support SSL?

https://gerrit.osmocom.org/c/pysim/+/40464/comment/1718c6f7_858f8dfb?usp=ema... : PS5, Line 587: parser.add_argument("-c", "--certpath", help=f"cert subdir relative to {DATA_DIR}", default="certs") I think --certpath is a bit irritating as one may thing that it is possible to provide an absolute path to a random location in the file system, which actually not the case. The option describes a sub directory in smdp-data. Maybe something like --certdir would be more suitable?

https://gerrit.osmocom.org/c/pysim/+/40464/comment/e4e6df34_a36a4e8d?usp=ema... : PS5, Line 588: parser.add_argument("-s", "--nossl", help="do NOT use ssl", action='store_true', default=False) I wonder if this may break anything when osmo-smdpp suddenly supports SSL by default. In any case I think that a --nossl option should be preferred over an --ssl option as SSL is the default and it should be on by default. So users will have to adopt.

https://gerrit.osmocom.org/c/pysim/+/40464/comment/a2dc15cc_5c8f1ae4?usp=ema... : PS5, Line 591: common_cert_path = os.path.join(DATA_DIR, args.certpath) (see above) Maybe having DATA_DIR as a commandline parameter would be a good idea.