Implement proper SQN #285
Draft
Implement proper SQN #285
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add the hashbang line to each service script and make them executable, so these can be executed directly from the terminal.
Configure ruff, the fast Python linter and code formatter, so developers can run "ruff check" and "ruff format --diff" in a pre-commit git hook to catch some errors early and to have consistent formatting. Set a line length that seems appropriate for pyhss and include the files that already pass all checks.
Before this patch we have multiple services trying to create the database and trying to upgrade the schema at the same time. (The logic for upgrading is currently just creating missing tables, I plan to improve this with future patches.) Instead of doing that, let only the main service, pyhss_hss, do this and let all other services wait until the database schema is ready.
Adding automatic database migrations from 1.0.0 to 1.0.1 is not feasible because the type of ims_subscriber.ifc_path was changed from VARCHAR(18) to VARCHAR(512), and this is not trivial to do with database type sqlite (there is no ALTER TABLE MODIFY). I don't think many people are running 1.0.0 from 2023, if any at all, so just document the changes that were made.
Version 1.0.2 has the same database scheme as 1.0.1.
The algo column is defined as:
algo = Column(
String(20),
default='3',
doc='2G Authentication Algorithm (1 = Comp128v1, 2 = Comp128v2, 3 = Comp128v3, All Other= 3G auth with 2g keys from 3G Milenage)',
)
Notably:
* It does not have nullable=False, meaning that NULL is allowed.
* The default of 3 is not set at CREATE TABLE time, for that
server_default would need to be used [1].
Handle the case that users may fill the database with a script that
leaves the algo field empty and therefore defaults to NULL.
Fix for:
TypeError: int() argument must be a string, a bytes-like object or a real number, not 'NoneType'
[1]: https://docs.sqlalchemy.org/en/20/core/defaults.html#sqlalchemy.schema.ColumnDefault
Prepare to implement the SQN counter as described in 3GPP TS 33.102 § C.1.1.2, instead of the currently used "sqn += 100" approach. According to the spec, SQN should look like this: SQN = SEQ | IND * SEQ is the number we increase with each new SQN. * IND is an array index. The upcoming patches will assign one IND to each client connected to PyHSS (MSC/VLR, SGSN, MME, ...). This is the same logic as in OsmoHLR. In order to do this, we need to know the subscriber-specific length of IND. § C.3.2 recommends the value 5, but it can be different. In the database, I use default=None instead of default=5, so we don't need to add migration logic for existing subscriber entries. Upcoming patches will check for None and use 5 in that case.
Ask connecting clients for the SERNR, just like OsmoHLR does it. This is in preparation for generating SQN from SEQ | IND, as IND will be assigned to each SERNR for GSUP (again following what OsmoHLR is doing).
Remove the workaround now that Get_Vectors_AuC_2g3g is properly increasing SEQ with each vector.
to fix this better, make a custom exception for subscriber not found and check against that, and only if there is a completely unexpected log the traceback.
Collaborator
Author
|
This is still a draft, but I've now added automatic database updates (will clean this up some more and make a separate PR) and storing SEQ numbers for each subscriber + IND combination. The latter also needs some more cleaning up, resolving some fixmes, etc., but from my testing with a 3g network it is already working as expected. |
Collaborator
|
Uuuh ... very nice. I'll definitly take a look at this as soon as I can. That's some great news 🎉 |
Collaborator
Author
Thanks 🙂 still no need for a thorough review yet, I'm currently splitting out and cleaning up the DB upgrade logic (for a separate PR), so there will be some more changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will fix #266. I've tested it with 3G and subscribers were able to attach.
There is still a lot to do before this is ready to merge, but sending the PR already in case somebody is curious about how I plan to implement this.
TODO:
CC @Takuto88 in case you want to have a look and have comments already, but a thorough review is not needed as this is still very WIP.