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=em… : 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=em… : 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=em… : 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=em… : 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=em… : 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=em… : 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=em… : 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. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/40464?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I84b2666422b8ff565620f3827ef4d4d7635a21be Gerrit-Change-Number: 40464 Gerrit-PatchSet: 5 Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de> Gerrit-Comment-Date: Fri, 20 Jun 2025 13:32:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes