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

ref(server): Re-authenticate at regular intervals #731

Merged
merged 8 commits into from
Aug 27, 2020
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
**Features**:

- Support chunked formdata keys for event payloads on the Minidump endpoint. Since crashpad has a limit for the length of custom attributes, the sentry event payload can be split up into `sentry__1`, `sentry__2`, etc. ([#721](https://github.com/getsentry/relay/pull/721))
- Add periodic re-authentication with the upstream server, previously there was only one initial authentication. ([#731](https://github.com/getsentry/relay/pull/731))

**Internal**:

Expand Down
20 changes: 20 additions & 0 deletions relay-config/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,12 @@ struct Http {
max_retry_interval: u32,
/// The custom HTTP Host header to send to the upstream.
host_header: Option<String>,
/// The number of seconds after a successful authentication after which
/// a Relay tries to re-authenticate with the upstream server.
auth_interval: u64,
/// The number of seconds allowed for a Relay to re-authenticate after which
/// it is considered that re-authentication has failed.
auth_grace_period: u64,
}

impl Default for Http {
Expand All @@ -533,6 +539,8 @@ impl Default for Http {
connection_timeout: 3,
max_retry_interval: 60,
host_header: None,
auth_interval: 60,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we don't have all PRs impld and merged I would make the entire new behavior opt-in.

This comment was marked as spam.

auth_grace_period: 10,
}
}
}
Expand Down Expand Up @@ -1100,6 +1108,18 @@ impl Config {
self.values.relay.tls_identity_password.as_deref()
}

/// Returns the interval at which Realy should try to re-authenticate with the upstream
pub fn http_auth_interval(&self) -> Duration {
Duration::from_secs(self.values.http.auth_interval)
}

/// The maximum amount of time that a Relay is allowed to take to re-authenticate with
/// the upstream after which it is declared as un-authenticated (if it is not able to
/// authenticate).
pub fn http_auth_grace_period(&self) -> Duration {
Duration::from_secs(self.values.http.auth_grace_period)
}

/// Returns whether this Relay should emit outcomes.
///
/// This is `true` either if `outcomes.emit_outcomes` is explicitly enabled, or if this Relay is
Expand Down
46 changes: 38 additions & 8 deletions relay-server/src/actors/upstream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use std::collections::VecDeque;
use std::fmt;
use std::str;
use std::sync::Arc;
use std::time::Instant;

use ::actix::fut;
use ::actix::prelude::*;
Expand Down Expand Up @@ -75,12 +76,20 @@ pub enum UpstreamRequestError {
ChannelClosed,
}

impl UpstreamRequestError {
fn is_network_error(&self) -> bool {
match self {
Self::SendFailed(_) | Self::PayloadFailed(_) => true,
Self::ResponseError(code, _) => matches!(code.as_u16(), 502 | 503),
_ => false,
}
}
}

/// Represents the current auth state.
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
enum AuthState {
Unknown,
RegisterRequestChallenge,
RegisterChallengeResponse,
Registered,
Error,
}
Expand Down Expand Up @@ -195,6 +204,7 @@ struct UpstreamRequest {

pub struct UpstreamRelay {
backoff: RetryBackoff,
first_error: Option<Instant>,
config: Arc<Config>,
auth_state: AuthState,
max_inflight_requests: usize,
Expand Down Expand Up @@ -257,11 +267,12 @@ impl UpstreamRelay {
UpstreamRelay {
backoff: RetryBackoff::new(config.http_max_retry_interval()),
max_inflight_requests: config.max_concurrent_requests(),
config,
auth_state: AuthState::Unknown,
num_inflight_requests: 0,
high_prio_requests: VecDeque::new(),
low_prio_requests: VecDeque::new(),
first_error: None,
config,
}
}

Expand All @@ -273,6 +284,17 @@ impl UpstreamRelay {
}
}

fn handle_network_error(&mut self) {
let now = Instant::now();
let first_error = *self.first_error.get_or_insert(now);
if first_error + self.config.http_auth_grace_period() > now {
return;
}

self.auth_state = AuthState::Error;
// TODO: initiate authentication loop if not already running
}

fn send_request(
&mut self,
request: UpstreamRequest,
Expand Down Expand Up @@ -451,7 +473,7 @@ impl Message for Authenticate {
/// message was successfully handled).
///
/// **Note:** Relay has retry functionality, outside this actor, that periodically sends Authenticate
/// messages until successful Authentication with the upstream server was achieved.
/// messages until successful Authentication with the upstream server was achieved.
impl Handler<Authenticate> for UpstreamRelay {
type Result = ResponseActFuture<Self, (), ()>;

Expand All @@ -466,29 +488,37 @@ impl Handler<Authenticate> for UpstreamRelay {
self.config.upstream_descriptor()
);

self.auth_state = AuthState::RegisterRequestChallenge;
let request = RegisterRequest::new(&credentials.id, &credentials.public_key);

let future = self
.send_query(request)
.into_actor(self)
.and_then(|challenge, slf, ctx| {
log::debug!("got register challenge (token = {})", challenge.token());
slf.auth_state = AuthState::RegisterChallengeResponse;
let challenge_response = challenge.create_response();

log::debug!("sending register challenge response");
let fut = slf.send_query(challenge_response).into_actor(slf);
ctx.notify(PumpHttpMessageQueue);
fut
})
.map(|_, slf, _ctx| {
.map(|_, slf, ctx| {
log::info!("relay successfully registered with upstream");
slf.auth_state = AuthState::Registered;

slf.backoff.reset();
slf.first_error = None;

ctx.notify_later(Authenticate, slf.config.http_auth_interval());
})
.map_err(|err, slf, ctx| {
log::error!("authentication encountered error: {}", LogError(&err));
slf.auth_state = AuthState::Error;

if err.is_network_error() {
slf.handle_network_error();
} else {
slf.auth_state = AuthState::Error;
jan-auer marked this conversation as resolved.
Show resolved Hide resolved
}

// Do not retry client errors including authentication failures since client errors
// are usually permanent. This allows the upstream to reject unsupported Relays
Expand Down