chore(auth): Exclude URL from auth check as it can differ between environments #1142
Conversation
ram-senth
force-pushed
the
anomaly_detection/exclude_url_from_auth
branch
from
b567ec8
to
17f1b48
Compare
src/seer/json_api.py
Outdated
| if nonce: | ||
| if is_valid(b"%s:%s" % (nonce.encode(), body), key, signature_data): | ||
| sentry_sdk.metrics.incr( | ||
| key="rpc_signature_auth_success", value=1, tags={"with_url": False} |
There was a problem hiding this comment.
Change the key and value to {"additional_payload": "nonce"}? Similarly change line 193 to {"additional_payload": "url"}?
There was a problem hiding this comment.
I can also make the change if you are ok with it.
There was a problem hiding this comment.
Have removed use of metrics API and changed the key-value pairs.
src/seer/json_api.py
Outdated
| @@ -88,7 +88,10 @@ def wrapper(config: AppConfig = injected) -> Any: | |||
| # Optional for now during rollout, make this required after rollout. | |||
| if auth_header.startswith("Rpcsignature "): | |||
| parts = auth_header.split() | |||
| if len(parts) != 2 or not compare_signature(request.url, raw_data, parts[1]): | |||
| sentry_sdk.metrics.incr(key="rpc_signature_auth_attempt", value=1) | |||
There was a problem hiding this comment.
metrics API is being deprecated, so you would want to switch it out for a span + span metric (e.g.
Lines 108 to 109 in f393fef
you can also do this to reference the current span
seer/src/seer/grouping/grouping.py
Line 312 in f393fef
There was a problem hiding this comment.
Used the second option to update existing span.
By using URL as part of the payload that is signed we are assuming that URL used by client will be same as the URL seen in the request payload on Seer end. This is not true as requests from Subscription processor has full UTL including server name while those from the UI do not.
This PR makes URL optional. This will be step 1 of the fix. We need to follow this up with two more changes: