20 Jun
2025
20 Jun
'25
3:32 p.m.
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