ref: Generic HMAC service authentication class#106231
Conversation
This PR replaces the ServiceRpcSignatureAuthentication with a more generic HmacSignatureAuthentication class which attempts to provide a class to do hmac authentication which is not RPC specific in any way. The motivation for this change to enable new service (Synapse), which does not use RPC to be able to utilize this implementation. The main change involved here is making the service prefix (which was previously hardcoded to rpc0:) configurable. Since this class never did any parsing of request bodies and just did cryptographic validation on body bytes this should be a fairly easy/safe change to make.
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
michelletran-sentry
left a comment
michelletran-sentry
left a comment
There was a problem hiding this comment.
Changes generally LGTM except for the one comment below!
| ) | ||
|
|
||
| signature_input = body | ||
| if include_url_in_signature: |
There was a problem hiding this comment.
I'm not seeing a test that uses include_url_in_signature. Should we have a test for this route?
There was a problem hiding this comment.
good point, added a test with include_url_in_signature=True
markstory
left a comment
markstory
left a comment
There was a problem hiding this comment.
Looking good. I agree with Michelle that we should have one more test covering the URL in signature flow.
| - signature_prefix: str - prefix for the signature format (e.g., "rpc0", "service0"). The colon will be added automatically. Defaults to "rpc0" for backward compatibility. | ||
| - include_url_in_signature: bool - If True, signs "url:body". If False, signs only "body". Defaults to False for backward compatibility. |
There was a problem hiding this comment.
👍 Nice. This is a great way to capture the existing differences in the implementations we have right now.
| def generate_service_request_signature( | ||
| url_path: str, body: bytes, shared_secret_setting: list[str] | None, service_name: str | ||
| url_path: str, | ||
| body: bytes, | ||
| shared_secret_setting: list[str] | None, | ||
| service_name: str, | ||
| signature_prefix: str = "rpc0", | ||
| include_url_in_signature: bool = False, | ||
| ) -> str: |
There was a problem hiding this comment.
After this merges it would be good to merge this with the other signature generation function we have for sentry's region RPC system as well.
4 participants
This PR replaces the ServiceRpcSignatureAuthentication with a more generic HmacSignatureAuthentication class which attempts to provide a class to do hmac authentication which is not RPC specific in any way.
The motivation for this change to enable new service (Synapse), which does not use RPC to be able to utilize this implementation.
The are two main changes here:
rpc0:) configurable. Since this class never did any parsing of request bodies and just did cryptographic validation on body bytes this should be a fairly easy/safe change to makeDefault args are set for backward compatibility.
Note: the querystring is not included in the url that is signed. I didn't add this here to avoid more changes / encoding complexities.