Skip to content
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

feat(hybridcloud) Add shared secret and signatures to RPC requests #52842

Merged
merged 8 commits into from
Jul 18, 2023

Conversation

markstory
Copy link
Member

When doing RPC requests we should have authentication on requests. I've gone with a simple request signature HMAC verfication as it felt like the simplest solution that was tamper & forgery resistant. Previously we had planned on using mTLS as authentication between regions. Setting up a cerfiticate authority and managing certificates is more scope than we presently have capacity for.

I'm anticipating us needing to do key rotation, or signature upgrades in the future and have accounted for both in the current design.

Refs HC-730

When doing RPC requests we should have authentication on requests. I've
gone with a simple request signature HMAC verfication as it felt like
the simplest solution that was tamper & forgery resistant. Previously
we had planned on using mTLS as authentication between regions. Setting
up a cerfiticate authority and managing certificates is more scope than
we presently have capacity for.

I'm anticipating us needing to do key rotation, or signature upgrades in
the future and have accounted for both in the current design.

Refs HC-730
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 13, 2023
@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #52842 (97b14a6) into master (9ef3ba5) will decrease coverage by 0.01%.
The diff coverage is 89.65%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #52842      +/-   ##
==========================================
- Coverage   79.48%   79.48%   -0.01%     
==========================================
  Files        4935     4935              
  Lines      207668   207762      +94     
  Branches    35453    35470      +17     
==========================================
+ Hits       165069   165133      +64     
- Misses      37570    37601      +31     
+ Partials     5029     5028       -1     
Impacted Files Coverage Δ
...c/app/components/events/interfaces/nativeFrame.tsx 55.22% <ø> (ø)
static/app/utils/fields/index.ts 62.50% <ø> (ø)
src/sentry/conf/server.py 91.62% <60.00%> (-0.17%) ⬇️
src/sentry/services/hybrid_cloud/rpc.py 89.16% <88.23%> (-0.26%) ⬇️
src/sentry/api/authentication.py 82.41% <100.00%> (+1.35%) ⬆️
src/sentry/api/endpoints/rpc.py 100.00% <100.00%> (ø)

... and 54 files with indirect coverage changes

Using requests lets us use responses for mocks which is more consistent
with other network stubs that we have in the application.
@markstory markstory marked this pull request as ready for review July 14, 2023 18:24
@markstory markstory requested review from a team as code owners July 14, 2023 18:24
@markstory markstory merged commit 2678010 into master Jul 18, 2023
@markstory markstory deleted the feat-rpc-authentication branch July 18, 2023 21:39
@markstory markstory added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Jul 18, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: f9bc819

getsentry-bot added a commit that referenced this pull request Jul 18, 2023
…uests (#52842)"

This reverts commit 2678010.

Co-authored-by: markstory <24086+markstory@users.noreply.github.com>
markstory added a commit that referenced this pull request Jul 18, 2023
The changes in #52842 needed to be reverted because of a removed json
import. This reverts commit f9bc819.
and restores RPC authentication.
markstory added a commit that referenced this pull request Jul 19, 2023
…#53118)

The changes in #52842 needed to
be reverted because of a removed json import. This reverts commit
f9bc819
and restores RPC authentication.
@github-actions github-actions bot locked and limited conversation to collaborators Aug 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert Add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants