Port register, login and update services to the new ECS style #923
Conversation
Added new directories v1 and v2 to mirror the paths in the API with the file structure. Added vpn routes to its own vpn module. Created a v1 module because the path in probe_services.py in the monolith code is `/api/v1/login`, so we keep the same versioning and path definition
…es.py version in the monolith
…old api; Add a testing data json
| response : Response, | ||
| settings : Settings = Depends(get_settings), | ||
| ) -> ProbeLoginResponse: | ||
| global log |
There was a problem hiding this comment.
Is it necessary to make the log variable global?
There was a problem hiding this comment.
Not really, I will remove it. I was mixing it with the pattern used in the monolith (example here), but now that I look closer is very different
|
|
||
| return register_response | ||
|
|
||
| class ProbeUpdate(BaseModel): |
There was a problem hiding this comment.
The ProbeUpdate request contains some useful metadata that we could use for the purpose of the anonymous credential anomaly detection to establish a baseline for comparing the probe_cc and probe_asn inconsistencies (see: https://github.com/ooni/orchestra/blob/master/registry/registry/handler/registry.go#L25).
Basically what I'm thinking of is that, if we assume that the update requests are not going to be tampered with as much as the measurement submit ones, we can use this to determine which inconsistencies are due to our geoip being off compared to the probe one, vs those which are due to the measurement metadata being inconsistent due to malice.
We can do parsing and logging of this as a follow up PR though.
| # "audience" claim. The "issued at" claim is rounded up. | ||
| issued_at = int(time.time()) | ||
| payload = {"iat": issued_at, "aud": "probe_login"} | ||
| client_id = create_jwt(payload, key=settings.jwt_encryption_key) |
There was a problem hiding this comment.
We should take note of the fact the jwt_encryption_key is being shared for between probes and user authenticating via email in the auth service.
I don't think there is necessary a problem, given that the aud field is different, but we should make sure we don't forget to always check aud filed to avoid authentication bypasses.
There was a problem hiding this comment.
Following a discussion with @LDiazN we decided to actually change this to a dedicated JWT token which we generate and inject via terraform: https://github.com/ooni/devops/blob/main/tf/environments/dev/main.tf#L187
ooniapi/services/ooniprobe/src/ooniprobe/routers/v1/probe_services.py
Outdated
Show resolved
Hide resolved
|
This looks good. Here is a summary of what should be addressed prior to merge:
|
This PR is related to #907 and ports services for login, register and update to the new ECS development style, mostly copied and pasted from the monolith, including tests.
No host points to this services right now, so it shouldn't crash any existent probes or infrastructure.
You can try the new endpoints here