-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(preprod): Create artifact download endpoint + associated authentication code #93865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -550,3 +551,120 @@ def authenticate_token(self, request: Request, token: str) -> tuple[Any, Any]: | |||
sentry_sdk.get_isolation_scope().set_tag("rpc_auth", True) | |||
|
|||
return (AnonymousUser(), token) | |||
|
|||
|
|||
def compare_service_signature( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty much 1:1 rip of https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/seer_rpc.py#L60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the seer implementation was derived from the cross-region rpc auth. We should consolidate these implementations now that we have 3 of them, but that can happen after this merges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, so long as @mdtro signs off on this PR, I can then put another one up after this to merge all three until one
return False | ||
|
||
|
||
class ServiceRpcSignatureAuthentication(StandardAuthentication): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
shared_secret_setting_name = LAUNCHPAD_RPC_SHARED_SECRET_SETTING | ||
service_name = "Launchpad" | ||
sdk_tag_name = "launchpad_rpc_auth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could just create another version of this but for seer cc @jennmueng
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #93865 +/- ##
==========================================
+ Coverage 88.02% 88.06% +0.03%
==========================================
Files 10339 10338 -1
Lines 596639 598538 +1899
Branches 23171 23171
==========================================
+ Hits 525199 527098 +1899
Misses 70981 70981
Partials 459 459 |
9ce5c17
to
3bd37f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks like we can definitely share it with Seer once it's in. I'm definitely not the best person to review this though, @mdtro will be.
@@ -550,3 +551,120 @@ def authenticate_token(self, request: Request, token: str) -> tuple[Any, Any]: | |||
sentry_sdk.get_isolation_scope().set_tag("rpc_auth", True) | |||
|
|||
return (AnonymousUser(), token) | |||
|
|||
|
|||
def compare_service_signature( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the seer implementation was derived from the cross-region rpc auth. We should consolidate these implementations now that we have 3 of them, but that can happen after this merges.
class ProjectPreprodArtifactDownloadEndpoint(ProjectEndpoint): | ||
owner = ApiOwner.EMERGE_TOOLS | ||
publish_status = { | ||
"GET": ApiPublishStatus.EXPERIMENTAL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid marking this public in the future, as public endpoints become part of the customer facing API docs.
src/sentry/preprod/authentication.py
Outdated
LAUNCHPAD_RPC_SHARED_SECRET_SETTING = "LAUNCHPAD_RPC_SHARED_SECRET" | ||
|
||
|
||
@AuthenticationSiloLimit(SiloMode.CONTROL, SiloMode.REGION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AuthenticationSiloLimit(SiloMode.CONTROL, SiloMode.REGION) | |
@AuthenticationSiloLimit(SiloMode.REGION) |
I don't think you'll be sending requests to control silo.
state=PreprodArtifact.ArtifactState.UPLOADING, | ||
) | ||
|
||
url = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/files/preprodartifacts/{unprocessed_artifact.id}/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is an internal API. We could put the URLs under /internal
that will prevent these URLs from being reached from the frontend loadbalancers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token handling logic and endpoint looks good to me.
src/sentry/api/authentication.py
Outdated
url: str, | ||
body: bytes, | ||
signature: str, | ||
shared_secret_setting: list[str] | None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to allow None
here? It'd be safer to enforce a key explicitly being set.
We should also not allow shared_secret_setting = [""]
. This will effectively sign with an empty HMAC key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None isn't actually allow here. It's rejected
if not shared_secret_setting:
raise RpcAuthenticationSetupException(
f"Cannot validate {service_name} RPC request signatures without shared secret"
)
None is just an option to make the usage of the ServiceRpcSignatureAuthentication
class cleaner. I'll change it tho 👍 I also just updated the code to handle the empty HMAC key case
Suspect IssuesThis pull request was deployed and Sentry observed the following issues: Did you find this useful? React with a 👍 or 👎 |
…ication code (#93865) I implemented the authentication logic that will power the monolith <> launchpad specific endpoints. The code is exactly how seer currently has its auth HTTP calls implemented. I put it in a shared space so that once this lands, we could potentially have the seer team share this underlying logic too. That way we don't have two different implementations of the same thing As for "why this auth approach", I explored the different ways we currently have it implemented: 1. Relay: Public key + signature validation + IP allowlists 2. Cross-region RPC: Shared secret HMAC (RpcSignatureAuthentication) 3. Seer: Custom shared secret HMAC (SeerRpcSignatureAuthentication) 4. Codecov: JWT with shared signing secret 5. Taskbroker: gRPC interceptor with shared secrets I went with the #3 approach since our use case is pretty much identical to the Seer use case and the implementation seemed the most straightforward. Security folks though, please weigh in here! You know best I also created 1 of the 3 new endpoints that we need for the launchpad service. This one just allows our service to download the artifact file. I included it so that the full usage of this auth logic is apparent
I implemented the authentication logic that will power the monolith <> launchpad specific endpoints. The code is exactly how seer currently has its auth HTTP calls implemented. I put it in a shared space so that once this lands, we could potentially have the seer team share this underlying logic too. That way we don't have two different implementations of the same thing
As for "why this auth approach", I explored the different ways we currently have it implemented:
I went with the #3 approach since our use case is pretty much identical to the Seer use case and the implementation seemed the most straightforward.
Security folks though, please weigh in here! You know best
I also created 1 of the 3 new endpoints that we need for the launchpad service. This one just allows our service to download the artifact file. I included it so that the full usage of this auth logic is apparent