feat(client): A few improvements to clients #188
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
Collaborator
samoehlert
commented
Nov 21, 2025
samoehlert
commented
- Autocreate a UUID if one is not passed by a client during registration (recent request)
- Display UUID on the admin page for a client (improve admin life)
- Rename hostname => client_name to prevent confusion (fix from the pentest)
- Make the UUID a unique thing to prevent a collision (fix from the pentest)
- Grab the IP from the registering client and use that to further verify a client (fix from the pentest)
s/hostname/client_name because it was confusing. make UUID a unique field. add the ability to check that a client is bound to a specific IP.
There's probably a better way to do this. Definitely needs a look and think
… IP, and use it to further verify a client
samoehlert
added
bug
Collaborator
Author
|
Please look at the get_ip stuff, it feels fragile and maybe can be made more secure. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7d0386b |
…e error message we can live without that info for now
samoehlert
commented
Dec 17, 2025
samoehlert
commented
Dec 17, 2025
samoehlert
commented
Dec 17, 2025
…g and running tests It seems like we were hitting a subtle problem where it was using cached images which caused it to run the main code instead of my code.
…-security/SCRAM into topic/soehlert/client_improvements
… building and running tests" This reverts commit b42ae97.
…me instead of hostname apparently the failure was because there were changes on main that hadn't been pulled here and they were run causing failures
…endpoints for consistency's sake and explicitness
this lets people register if they add one to the registration POST. it was just set to autocreate one only. this is more flexible.
This lets us still use the autocreate UUID that we've done here
…ts with UUID, create clients without a UUID provided, do an idempotent create, and not leak client_names
…on with UUID and without (for our autocreate UUID code branch) also up our documentation game so we have much better info in DRF spectacular
also added pre commit config that hopefully doesnt break anything because i couldnt commit otherwise
…endpoints for consistency's sake and explicitness
this lets people register if they add one to the registration POST. it was just set to autocreate one only. this is more flexible.
This lets us still use the autocreate UUID that we've done here
…ts with UUID, create clients without a UUID provided, do an idempotent create, and not leak client_names
…on with UUID and without (for our autocreate UUID code branch) also up our documentation game so we have much better info in DRF spectacular
…ration tests (#157) This one was a doozie. Expired/deleted entries on one SCRAM instance were never unblocked on other instances because there is no mechanism for postgres to let django know that it needs to re-run stuff, only the active django re-runs stuff. Now, we get around that by calling process_updates and re-updating anything changed recently, however, before, the sync logic only looked at creation time (`when` field), missing any entries that were removed.. To fix this in a lot less dumb way than we used to, we now we use `django-simple-history` to detect _**any**_ entry changes and then go ahead and just reprocess them to what the DB says they should be doing. There is a lot in this PR, but the main changes are: - Split up `process_updates()` into: - `get_entries_to_process(cutoff_time)` - This is a function that we could reuse that simply finds all recently modified entries from other instances and returns them so you can Do Stuff® on em. - `reprocess_entries(entries)` - This actually "Does Stuff" by sending websocket messages to translators. We could eventually move this out of here and make it more generic to be used everywhere we Do A Websocket® but it needs to be a bit more dynamic to support future action types (see note below, not dealing with this now). - `_check_for_orphaned_history()` - This is a total edge case, but since we don't ever delete entries (our delete() override) I wanted to make sure that we catch and log any orphaned history objects that don't have a corresponding entry (say someone goes into the admin panel and actually "hard" deletes an entry, but the history stuff is still there). - Made sure to be efficient with DB calls by using `select_related()`,`.exclude()`, and `values_list()` on most of our django/db queries to make sure that we're relying on SQL to do the filtering (using built in django stuff obviously) instead of iterating on the python side (in the case of prod where we have a TON of entries.) - We used to rely on `msg.msg_type` from WebSocketMessage (which is always `translator_add`), but now we set that based on `entry.is_active` when we run the sync from DB to translator. Eventually though this needs to be more dynamic to support other action types, not sure how to solve that though. There are a few caveats here to keep in mind, but for now i think it's fine: - This breaks from the existing behave patterns, and adds a pattern where we connect directly to the containers running via compose and ignores the test database. I tried to seperate this out into full integration tests. This means that test data is left behind so we blow away everything in the DB to reset it before and after test runs. - It's possible that a docker health check could run while the behave integration tests are running. This is unlikely and I made the health check run less frequently just to help avoid that but... it's gross. For now, I think this gets us further, so it's still good. It also fixes things to use postgres instead of sqlite, which is a win overall. # Conflicts: # scram/route_manager/tests/acceptance/steps/common.py # scram/route_manager/tests/acceptance/steps/ip.py
also added pre commit config that hopefully doesnt break anything because i couldnt commit otherwise
how can there be any left i keep looking
Collaborator
crankynetman
left a comment
crankynetman
left a comment
There was a problem hiding this comment.
Looks really great, I especially love the OpenAPI stuff. I have a few questions inline about the IP stuff though.
Collaborator
There was a problem hiding this comment.
nit: Any reason to not do the two migrations in the same file?
Comment on lines
+245
to
+256
| # Check if the client's IP address is allow listed | ||
| if client.registered_from_ip: | ||
| request_ip = self.get_client_ip() | ||
| if client.registered_from_ip != request_ip: | ||
| logger.warning( | ||
| "Client %s attempted to authorize from unauthorized IP %s (expected %s)", | ||
| uuid, | ||
| request_ip, | ||
| client.registered_from_ip, | ||
| ) | ||
| msg = "Request from unauthorized IP address %s" | ||
| raise PermissionDenied(msg) |
Collaborator
There was a problem hiding this comment.
What's our plan for existing clients that have already been registered?
Right now, our existing clients will get denied with a null IP in the database so I think we should consider:
- null IP entries are allowed to auth for existing clients but then we populate the IP address after it tries to do something (seems like a potential avenue for someone adding their own IP.
- we manually update our databases to have the IP of our existing clients.
- we write a one time use management script to go and find the A/AAAA records for a host and add them during that one time use.
Collaborator
There was a problem hiding this comment.
Another thought... Say I'm a bad person and I'm trying to enumerate what's allowed from an IP standpoint or maybe trying to get information about what's behind a load balancer. Could I then maybe use this to grab the load-balancer's IP address and use that for something nefarious?
| uuid = models.UUIDField() | ||
| client_name = models.CharField(max_length=50, unique=True) | ||
| uuid = models.UUIDField(default=uuid_lib.uuid4, editable=False, unique=True) | ||
| registered_from_ip = models.GenericIPAddressField(null=True, blank=True) |
Collaborator
There was a problem hiding this comment.
How does this work for dual-stacked hosts? Sometimes they might connect from IPv4, sometimes from IPv6. Additionally, do we need to come up with a method for a client to change its IP automagically? If we don't, how will we handle hosts getting renumbered (or even how will we handle multiple v6 addresses on a host (privacy, ula, etc).
|
|
||
| # Check if the client's IP address is allow listed | ||
| if client.registered_from_ip: | ||
| request_ip = self.get_client_ip() |
Collaborator
There was a problem hiding this comment.
I'm not sure how tests are passing with this, but when I try running a POST to the entries API, I get a 500:
django-1 | File "/app/scram/route_manager/api/views.py", line 247, in check_client_authorization
django-1 | request_ip = self.get_client_ip()
django-1 | ^^^^^^^^^^^^^^^^^^
django-1 | AttributeError: 'EntryViewSet' object has no attribute 'get_client_ip'
I think this needs to be request_ip = get_client_ip(self.request)?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.