From bcc8b075133c6caf66c2cfd1e59a28bfe7aaa753 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 1 Sep 2020 15:56:50 +0200 Subject: [PATCH 1/6] fix(auth): Include state in authentication requests --- Cargo.lock | 1 + py/sentry_relay/auth.py | 15 +- py/tests/test_auth.py | 44 +++--- relay-auth/Cargo.toml | 1 + relay-auth/src/lib.rs | 215 +++++++++++++++++++++++----- relay-cabi/Cargo.lock | 1 + relay-cabi/include/relay.h | 12 +- relay-cabi/src/auth.rs | 87 +++++------ relay-cabi/src/core.rs | 2 + relay-common/src/time.rs | 29 ++++ relay-server/src/actors/upstream.rs | 2 +- 11 files changed, 284 insertions(+), 125 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f8792fcfc3..096ccd4283 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2805,6 +2805,7 @@ dependencies = [ "chrono", "ed25519-dalek", "failure", + "hmac", "rand 0.6.5", "relay-common", "serde", diff --git a/py/sentry_relay/auth.py b/py/sentry_relay/auth.py index ba38edf9a6..6813525d83 100644 --- a/py/sentry_relay/auth.py +++ b/py/sentry_relay/auth.py @@ -18,7 +18,6 @@ "SecretKey", "generate_key_pair", "create_register_challenge", - "get_register_response_relay_id", "validate_register_response", "is_version_supported", ] @@ -91,34 +90,28 @@ def generate_relay_id(): return decode_uuid(rustcall(lib.relay_generate_relay_id)) -def create_register_challenge(data, signature, max_age=60 * 15): +def create_register_challenge(data, signature, secret, max_age=60): challenge_json = rustcall( lib.relay_create_register_challenge, make_buf(data), encode_str(signature), + encode_str(secret), max_age, ) challenge = json.loads(decode_str(challenge_json, free=True)) return { "relay_id": uuid.UUID(challenge["relay_id"]), - "public_key": PublicKey.parse(challenge["public_key"]), "token": challenge["token"], } -def get_register_response_relay_id(data): - return decode_uuid( - rustcall(lib.relay_get_register_response_relay_id, make_buf(data)) - ) - - -def validate_register_response(public_key, data, signature, max_age=60 * 15): +def validate_register_response(data, signature, secret, max_age=60): response_json = rustcall( lib.relay_validate_register_response, - public_key._objptr, make_buf(data), encode_str(signature), + encode_str(secret), max_age, ) diff --git a/py/tests/test_auth.py b/py/tests/test_auth.py index c0c3a768d9..f565f840ed 100644 --- a/py/tests/test_auth.py +++ b/py/tests/test_auth.py @@ -3,6 +3,17 @@ import pytest +UPSTREAM_SECRET = "secret" + +RELAY_ID = b"6b7d15b8-cee2-4354-9fee-dae7ef43e434" +RELAY_KEY = b"kMpGbydHZSvohzeMlghcWwHd8MkreKGzl_ncdkZSOMg" +REQUEST = b'{"relay_id":"6b7d15b8-cee2-4354-9fee-dae7ef43e434","public_key":"kMpGbydHZSvohzeMlghcWwHd8MkreKGzl_ncdkZSOMg","version":"20.8.0"}' +REQUEST_SIG = "JIwzIb3kuOaVwgq_DRuPpquGVIIu0plfpOSvz_ixzfw_RmdyHr35cJrna7Jg_uXqNHQbSP1Yj0-4X5Omk9jcBA.eyJ0IjoiMjAyMC0wOS0wMVQxMzozNzoxNC40Nzk0NjVaIn0" +TOKEN = "eyJ0aW1lc3RhbXAiOjE1OTg5Njc0MzQsInJlbGF5X2lkIjoiNmI3ZDE1YjgtY2VlMi00MzU0LTlmZWUtZGFlN2VmNDNlNDM0IiwicHVibGljX2tleSI6ImtNcEdieWRIWlN2b2h6ZU1sZ2hjV3dIZDhNa3JlS0d6bF9uY2RrWlNPTWciLCJyYW5kIjoiLUViNG9Hal80dUZYOUNRRzFBVmdqTjRmdGxaNU9DSFlNOFl2d1podmlyVXhUY0tFSWYtQzhHaldsZmgwQTNlMzYxWE01dVh0RHhvN00tbWhZeXpWUWcifQ:KJUDXlwvibKNQmex-_Cu1U0FArlmoDkyqP7bYIDGrLXudfjGfCjH-UjNsUHWVDnbM28YdQ-R2MBSyF51aRLQcw" +RESPONSE = b'{"relay_id":"6b7d15b8-cee2-4354-9fee-dae7ef43e434","token":"eyJ0aW1lc3RhbXAiOjE1OTg5Njc0MzQsInJlbGF5X2lkIjoiNmI3ZDE1YjgtY2VlMi00MzU0LTlmZWUtZGFlN2VmNDNlNDM0IiwicHVibGljX2tleSI6ImtNcEdieWRIWlN2b2h6ZU1sZ2hjV3dIZDhNa3JlS0d6bF9uY2RrWlNPTWciLCJyYW5kIjoiLUViNG9Hal80dUZYOUNRRzFBVmdqTjRmdGxaNU9DSFlNOFl2d1podmlyVXhUY0tFSWYtQzhHaldsZmgwQTNlMzYxWE01dVh0RHhvN00tbWhZeXpWUWcifQ:KJUDXlwvibKNQmex-_Cu1U0FArlmoDkyqP7bYIDGrLXudfjGfCjH-UjNsUHWVDnbM28YdQ-R2MBSyF51aRLQcw"}' +RESPONSE_SIG = "HUp3eybT_5AmRJ_QzutfvStKTeE-cgD_reLPjIf4OpoOJT_Hln8ThrFqGyT_C6P8qF1LHbFLcrYFvQy4iNaqAQ.eyJ0IjoiMjAyMC0wOS0wMVQxMzozNzoxNC40ODEwNTNaIn0" + + def test_basic_key_functions(): sk, pk = sentry_relay.generate_key_pair() signature = sk.sign(b"some secret data") @@ -17,43 +28,34 @@ def test_basic_key_functions(): def test_challenge_response(): resp = sentry_relay.create_register_challenge( - b'{"relay_id":"95dc7c80-6db7-4505-8969-3a0927bfb85d","public_key":"KXxwPvbhadLYTglsiGnQe2lxKLCT4VB2qEDd-OQVLbQ"}', - "EQXKqDYLei5XhDucMDIR3n1khdcOqGWmUWDYhcnvi-OBkW92qfcAMSjSn8xPYDmkB2kLnNNsaFeBx1VifD3TCw.eyJ0IjoiMjAxOC0wMy0wMVQwOTo0NjowNS41NDA0NzdaIn0", - max_age=0xFFFFFFFF, + REQUEST, REQUEST_SIG, UPSTREAM_SECRET, max_age=0, ) - assert str(resp["public_key"]) == "KXxwPvbhadLYTglsiGnQe2lxKLCT4VB2qEDd-OQVLbQ" - assert resp["relay_id"] == uuid.UUID("95dc7c80-6db7-4505-8969-3a0927bfb85d") + assert resp["relay_id"] == uuid.UUID(RELAY_ID.decode("utf8")) assert len(resp["token"]) > 40 + assert ":" in resp["token"] def test_challenge_response_validation_errors(): with pytest.raises(sentry_relay.UnpackErrorSignatureExpired): sentry_relay.create_register_challenge( - b'{"relay_id":"95dc7c80-6db7-4505-8969-3a0927bfb85d","public_key":"KXxwPvbhadLYTglsiGnQe2lxKLCT4VB2qEDd-OQVLbQ"}', - "EQXKqDYLei5XhDucMDIR3n1khdcOqGWmUWDYhcnvi-OBkW92qfcAMSjSn8xPYDmkB2kLnNNsaFeBx1VifD3TCw.eyJ0IjoiMjAxOC0wMy0wMVQwOTo0NjowNS41NDA0NzdaIn0", - max_age=1, + REQUEST, REQUEST_SIG, UPSTREAM_SECRET, max_age=1, ) with pytest.raises(sentry_relay.UnpackErrorBadPayload): sentry_relay.create_register_challenge( - b'{"relay_id":"95dc7c80-6db7-4505-8969-3a0927bfb85d","public_key":"KXxwPvbhadLYTglsiGnQe2lxKLCT4VB2qEDd-OQVLbQ"}glumpat', - "EQXKqDYLei5XhDucMDIR3n1khdcOqGWmUWDYhcnvi-OBkW92qfcAMSjSn8xPYDmkB2kLnNNsaFeBx1VifD3TCw.eyJ0IjoiMjAxOC0wMy0wMVQwOTo0NjowNS41NDA0NzdaIn0", - max_age=1, + REQUEST + b"__broken", REQUEST_SIG, UPSTREAM_SECRET, max_age=0, + ) + with pytest.raises(sentry_relay.UnpackErrorBadSignature): + sentry_relay.create_register_challenge( + REQUEST, REQUEST_SIG + "__broken", UPSTREAM_SECRET, max_age=0, ) def test_register_response(): - pk = sentry_relay.PublicKey.parse("sFTtnMGut3xR_EqP1hSmyfBc6590wDQzHFGWj5nEG18") resp = sentry_relay.validate_register_response( - pk, - b'{"relay_id":"2ffe6ba6-3a27-4936-b30f-d6944a4f1216","token":"iiWGyrgBZDOOclHjnQILU6zHL1Mjl-yXUpjHOIaArowhrZ2djSUkzPuH_l7UF6sKYpbKD4C2nZWCBhuULLJE-w"}', - "uLvKHrTtFohGVMLDxlMZythEXmTJTx8DCT2VwZ_x5Aw0UzTzoastrn2tFy4I8jeTYzS-N8D-PZ_khfVzfFZHBg.eyJ0IjoiMjAxOC0wMy0wMVQwOTo0ODo1OC41ODMzNTRaIn0", - max_age=0xFFFFFFFF, - ) - assert ( - resp["token"] - == "iiWGyrgBZDOOclHjnQILU6zHL1Mjl-yXUpjHOIaArowhrZ2djSUkzPuH_l7UF6sKYpbKD4C2nZWCBhuULLJE-w" + RESPONSE, RESPONSE_SIG, UPSTREAM_SECRET, max_age=0, ) - assert resp["relay_id"] == uuid.UUID("2ffe6ba6-3a27-4936-b30f-d6944a4f1216") + assert resp["token"] == TOKEN + assert resp["relay_id"] == uuid.UUID(RELAY_ID.decode("utf8")) def test_is_version_supported(): diff --git a/relay-auth/Cargo.toml b/relay-auth/Cargo.toml index 9592bd2633..bd9a86121c 100644 --- a/relay-auth/Cargo.toml +++ b/relay-auth/Cargo.toml @@ -15,6 +15,7 @@ base64 = "0.10.1" chrono = "0.4.11" ed25519-dalek = "0.9.1" failure = "0.1.8" +hmac = "0.7.1" rand = "0.6.5" relay-common = { path = "../relay-common" } serde = { version = "1.0.114", features = ["derive"] } diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index 0a2813a7f0..9e35077676 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -2,13 +2,14 @@ use std::borrow::Cow; use std::fmt; use std::str::FromStr; -use chrono::{DateTime, Duration, Utc}; +use chrono::{DateTime, Duration, TimeZone, Utc}; use failure::Fail; +use hmac::{Hmac, Mac}; use rand::{rngs::OsRng, thread_rng, RngCore}; use serde::{de::DeserializeOwned, Deserialize, Serialize}; use sha2::Sha512; -use relay_common::Uuid; +use relay_common::{UnixTimestamp, Uuid}; include!(concat!(env!("OUT_DIR"), "/constants.gen.rs")); @@ -110,10 +111,10 @@ impl Serialize for RelayVersion { /// Raised if a key could not be parsed. #[derive(Debug, Fail, PartialEq, Eq, Hash)] pub enum KeyParseError { - /// Invalid key encoding + /// Invalid key encoding. #[fail(display = "bad key encoding")] BadEncoding, - /// Invalid key data + /// Invalid key data. #[fail(display = "bad key data")] BadKey, } @@ -124,6 +125,9 @@ pub enum UnpackError { /// Raised if the signature is invalid. #[fail(display = "invalid signature on data")] BadSignature, + /// Invalid key encoding. + #[fail(display = "bad key encoding")] + BadEncoding, /// Raised if deserializing of data failed. #[fail(display = "could not deserialize payload")] BadPayload(#[cause] serde_json::Error), @@ -424,6 +428,127 @@ pub fn generate_key_pair() -> (SecretKey, PublicKey) { (SecretKey { inner: kp }, PublicKey { inner: pk }) } +/// An encoded and signed `ChallengeState`. +/// +/// This signature can be used by the upstream server to ensure that the downstream client did not +/// tamper with the token without keeping state between requests. For more information, see +/// `ChallengeState`. +/// +/// The format and contents of `SignedChallengeState` are intentionally opaque. Downstream clients +/// do not need to interpret it, and the upstream can change its contents at any time. Parsing and +/// validation is only performed on the upstream. +/// +/// In the current implementation, the serialized state has the format `{state}:{signature}`, where +/// each component is: +/// - `state`: A URL-safe base64 encoding of the JSON serialized `ChallengeState`. +/// - `signature`: A URL-safe base64 encoding of the SHA512 HMAC of the encoded state. +/// +/// To create a signed challenge, use `RegisterChallenge::sign`. To validate the signature and read +/// the state, use `SignedRegisterChallenge::unpack`. In both cases, a secret for signing has to be +/// supplied. +#[derive(Clone, Debug, Deserialize, Serialize)] +pub struct SignedChallengeState(String); + +impl SignedChallengeState { + /// Creates an Hmac instance for signing the `ChallengeState`. + fn mac(secret: &[u8]) -> Hmac { + Hmac::new_varkey(secret).expect("HMAC takes variable keys") + } + + /// Signs the given `ChallengeState` and serializes it into a single string. + fn sign(state: ChallengeState, secret: &[u8]) -> Self { + let json = serde_json::to_string(&state).expect("relay challenge state serializes to JSON"); + let token = base64::encode_config(&json, base64::URL_SAFE_NO_PAD); + + let mut mac = Self::mac(secret); + mac.input(token.as_bytes()); + let signature = base64::encode_config(&mac.result().code(), base64::URL_SAFE_NO_PAD); + + Self(format!("{}:{}", token, signature)) + } + + /// Splits the signed state into the encoded state and encoded signature. + fn split(&self) -> (&str, &str) { + let mut split = self.as_str().splitn(2, ':'); + (split.next().unwrap_or(""), split.next().unwrap_or("")) + } + + /// Returns the string representation of the token. + pub fn as_str(&self) -> &str { + self.0.as_str() + } + + /// Unpacks the encoded state and validates the signature. + /// + /// If `max_age` is specified, then the timestamp in the state is validated against the current + /// time stamp. If the stored timestamp is too old, `UnpackError::SignatureExpired` is returned. + pub fn unpack( + &self, + secret: &[u8], + max_age: Option, + ) -> Result { + let (token, signature) = self.split(); + let code = base64::decode_config(signature, base64::URL_SAFE_NO_PAD) + .map_err(|_| UnpackError::BadEncoding)?; + + let mut mac = Self::mac(secret); + mac.input(token.as_bytes()); + mac.verify(&code).map_err(|_| UnpackError::BadSignature)?; + + let json = base64::decode_config(token, base64::URL_SAFE_NO_PAD) + .map_err(|_| UnpackError::BadEncoding)?; + let state = + serde_json::from_slice::(&json).map_err(UnpackError::BadPayload)?; + + if let Some(max_age) = max_age { + let secs = state.timestamp().as_secs() as i64; + if Utc.timestamp(secs, 0) + max_age < Utc::now() { + return Err(UnpackError::SignatureExpired); + } + } + + Ok(state) + } +} + +/// A state structure containing relevant information from `RegisterRequest`. +/// +/// This struct is used to carry over information between the downstream register request and +/// register response. In addition to identifying information, it contains a random bit to avoid +/// replay attacks. +#[derive(Clone, Deserialize, Serialize)] +pub struct ChallengeState { + timestamp: UnixTimestamp, + relay_id: RelayId, + public_key: PublicKey, + rand: String, +} + +impl ChallengeState { + /// Returns the timestamp at which the challenge was created. + pub fn timestamp(&self) -> UnixTimestamp { + self.timestamp + } + + /// Returns the identifier of the requesting downstream Relay. + pub fn relay_id(&self) -> RelayId { + self.relay_id + } + + /// Returns the public key of the requesting downstream Relay. + pub fn public_key(&self) -> &PublicKey { + &self.public_key + } +} + +/// Generates a new random token for the challenge state. +fn random_token() -> String { + let mut rng = thread_rng(); + let mut bytes = vec![0u8; 64]; + rng.fill_bytes(&mut bytes); + base64::encode_config(&bytes, base64::URL_SAFE_NO_PAD) +} + /// Represents a challenge request. /// /// This is created if the relay signs in for the first time. The server needs @@ -473,14 +598,17 @@ impl RegisterRequest { } /// Creates a register challenge for this request. - pub fn create_challenge(&self) -> RegisterChallenge { - let mut rng = thread_rng(); - let mut bytes = vec![0u8; 64]; - rng.fill_bytes(&mut bytes); - let token = base64::encode_config(&bytes, base64::URL_SAFE_NO_PAD); + pub fn into_challenge(self, secret: &[u8]) -> RegisterChallenge { + let state = ChallengeState { + timestamp: UnixTimestamp::now(), + relay_id: self.relay_id, + public_key: self.public_key, + rand: random_token(), + }; + RegisterChallenge { relay_id: self.relay_id, - token, + token: SignedChallengeState::sign(state, secret), } } } @@ -489,7 +617,7 @@ impl RegisterRequest { #[derive(Serialize, Deserialize, Debug)] pub struct RegisterChallenge { relay_id: RelayId, - token: String, + token: SignedChallengeState, } impl RegisterChallenge { @@ -500,14 +628,14 @@ impl RegisterChallenge { /// Returns the token that needs signing. pub fn token(&self) -> &str { - &self.token + self.token.as_str() } /// Creates a register response. - pub fn create_response(&self) -> RegisterResponse { + pub fn into_response(self) -> RegisterResponse { RegisterResponse { relay_id: self.relay_id, - token: self.token.clone(), + token: self.token, } } } @@ -516,13 +644,26 @@ impl RegisterChallenge { #[derive(Serialize, Deserialize, Debug)] pub struct RegisterResponse { relay_id: RelayId, - token: String, + token: SignedChallengeState, } impl RegisterResponse { - /// Loads the register response without validating. - pub fn unpack_unsafe(data: &[u8]) -> Result { - serde_json::from_slice(data).map_err(UnpackError::BadPayload) + /// Unpacks the register response and validates signatures. + pub fn unpack( + data: &[u8], + signature: &str, + secret: &[u8], + max_age: Option, + ) -> Result { + let response: Self = serde_json::from_slice(data).map_err(UnpackError::BadPayload)?; + let state = response.token.unpack(secret, max_age)?; + let public_key = state.public_key(); + + if !public_key.verify_timestamp(data, signature, max_age) { + return Err(UnpackError::BadSignature); + } + + Ok(response) } /// Returns the relay ID of the registering relay. @@ -532,7 +673,7 @@ impl RegisterResponse { /// Returns the token that needs signing. pub fn token(&self) -> &str { - &self.token + self.token.as_str() } } @@ -614,32 +755,40 @@ fn test_registration() { let (sk, pk) = generate_key_pair(); // create a register request - let reg_req = RegisterRequest::new(&relay_id, &pk); + let request = RegisterRequest::new(&relay_id, &pk); // sign it - let (reg_req_bytes, reg_req_sig) = sk.pack(®_req); + let (request_bytes, request_sig) = sk.pack(&request); // attempt to get the data through bootstrap unpacking. - let reg_req = - RegisterRequest::bootstrap_unpack(®_req_bytes, ®_req_sig, Some(max_age)).unwrap(); - assert_eq!(reg_req.relay_id(), &relay_id); - assert_eq!(reg_req.public_key(), &pk); + let request = + RegisterRequest::bootstrap_unpack(&request_bytes, &request_sig, Some(max_age)).unwrap(); + assert_eq!(request.relay_id(), &relay_id); + assert_eq!(request.public_key(), &pk); + + let upstream_secret = b"secret"; // create a challenge - let challenge = reg_req.create_challenge(); + let challenge = request.into_challenge(upstream_secret); + let challenge_token = challenge.token().to_owned(); assert_eq!(challenge.relay_id(), &relay_id); assert!(challenge.token().len() > 40); // create a response from the challenge - let reg_resp = challenge.create_response(); + let response = challenge.into_response(); // sign and unsign it - let (reg_resp_bytes, reg_resp_sig) = sk.pack(®_resp); - let reg_resp: RegisterResponse = pk - .unpack(®_resp_bytes, ®_resp_sig, Some(max_age)) - .unwrap(); - assert_eq!(reg_resp.relay_id(), &relay_id); - assert_eq!(reg_resp.token(), challenge.token()); + let (response_bytes, response_sig) = sk.pack(&response); + let response = RegisterResponse::unpack( + &response_bytes, + &response_sig, + upstream_secret, + Some(max_age), + ) + .unwrap(); + + assert_eq!(response.relay_id(), &relay_id); + assert_eq!(response.token(), challenge_token); } #[test] diff --git a/relay-cabi/Cargo.lock b/relay-cabi/Cargo.lock index 8f0a9eba34..35e388ec92 100644 --- a/relay-cabi/Cargo.lock +++ b/relay-cabi/Cargo.lock @@ -1094,6 +1094,7 @@ dependencies = [ "chrono", "ed25519-dalek", "failure", + "hmac", "rand 0.6.5", "relay-common", "serde", diff --git a/relay-cabi/include/relay.h b/relay-cabi/include/relay.h index 4ca16597ef..ab71c8fbaf 100644 --- a/relay-cabi/include/relay.h +++ b/relay-cabi/include/relay.h @@ -71,6 +71,7 @@ enum RelayErrorCode { RELAY_ERROR_CODE_UNPACK_ERROR_BAD_SIGNATURE = 1003, RELAY_ERROR_CODE_UNPACK_ERROR_BAD_PAYLOAD = 1004, RELAY_ERROR_CODE_UNPACK_ERROR_SIGNATURE_EXPIRED = 1005, + RELAY_ERROR_CODE_UNPACK_ERROR_BAD_ENCODING = 1006, RELAY_ERROR_CODE_PROCESSING_ERROR_INVALID_TRANSACTION = 2001, RELAY_ERROR_CODE_PROCESSING_ERROR_INVALID_GEO_IP = 2002, RELAY_ERROR_CODE_INVALID_RELEASE_ERROR_TOO_LONG = 3001, @@ -279,6 +280,7 @@ RelayStr relay_convert_datascrubbing_config(const RelayStr *config); */ RelayStr relay_create_register_challenge(const RelayBuf *data, const RelayStr *signature, + const RelayStr *secret, uint32_t max_age); /** @@ -335,12 +337,6 @@ void relay_geoip_lookup_free(RelayGeoIpLookup *lookup); RelayGeoIpLookup *relay_geoip_lookup_new(const char *path); -/** - * Given just the data from a register response returns the - * conained relay id without validating the signature. - */ -RelayUuid relay_get_register_response_relay_id(const RelayBuf *data); - /** * Initializes the library */ @@ -476,9 +472,9 @@ RelayStr relay_validate_pii_config(const RelayStr *value); /** * Validates a register response. */ -RelayStr relay_validate_register_response(const RelayPublicKey *pk, - const RelayBuf *data, +RelayStr relay_validate_register_response(const RelayBuf *data, const RelayStr *signature, + const RelayStr *secret, uint32_t max_age); /** diff --git a/relay-cabi/src/auth.rs b/relay-cabi/src/auth.rs index a6fc0cd547..7c4b76d838 100644 --- a/relay-cabi/src/auth.rs +++ b/relay-cabi/src/auth.rs @@ -1,11 +1,9 @@ use chrono::Duration; -use serde::Serialize; use relay_auth::{ generate_key_pair, generate_relay_id, PublicKey, RegisterRequest, RegisterResponse, RelayVersion, SecretKey, }; -use relay_common::Uuid; use crate::core::{RelayBuf, RelayStr, RelayUuid}; @@ -131,64 +129,51 @@ ffi_fn! { } } -#[derive(Serialize)] -struct RelayChallengeResult { - pub relay_id: Uuid, - pub public_key: PublicKey, - pub token: String, -} - ffi_fn! { /// Creates a challenge from a register request and returns JSON. - unsafe fn relay_create_register_challenge(data: *const RelayBuf, - signature: *const RelayStr, - max_age: u32) - -> Result - { - let max_age = Duration::seconds(i64::from(max_age)); + unsafe fn relay_create_register_challenge( + data: *const RelayBuf, + signature: *const RelayStr, + secret: *const RelayStr, + max_age: u32, + ) -> Result { + let max_age = match max_age { + 0 => None, + m => Some(Duration::seconds(i64::from(m))), + }; + let req = RegisterRequest::bootstrap_unpack( - (*data).as_bytes(), (*signature).as_str(), Some(max_age))?; - let challenge = req.create_challenge(); - Ok(RelayStr::from_string(serde_json::to_string(&RelayChallengeResult { - relay_id: *req.relay_id(), - public_key: req.public_key().clone(), - token: challenge.token().to_string(), - })?)) - } -} + (*data).as_bytes(), + (*signature).as_str(), + max_age, + )?; -ffi_fn! { - /// Given just the data from a register response returns the - /// conained relay id without validating the signature. - unsafe fn relay_get_register_response_relay_id(data: *const RelayBuf) - -> Result - { - Ok(RelayUuid::new(*RegisterResponse::unpack_unsafe((*data).as_bytes())?.relay_id())) + let challenge = req.into_challenge((*secret).as_str().as_bytes()); + Ok(RelayStr::from_string(serde_json::to_string(&challenge)?)) } } -#[derive(Serialize)] -struct RelayRegisterResponse { - pub relay_id: Uuid, - pub token: String, -} - ffi_fn! { /// Validates a register response. - unsafe fn relay_validate_register_response(pk: *const RelayPublicKey, - data: *const RelayBuf, - signature: *const RelayStr, - max_age: u32) - -> Result - { - let max_age = Duration::seconds(i64::from(max_age)); - let pk = &*(pk as *const PublicKey); - let reg_resp: RegisterResponse = pk.unpack( - (*data).as_bytes(), (*signature).as_str(), Some(max_age))?; - Ok(RelayStr::from_string(serde_json::to_string(&RelayRegisterResponse { - relay_id: *reg_resp.relay_id(), - token: reg_resp.token().to_string(), - })?)) + unsafe fn relay_validate_register_response( + data: *const RelayBuf, + signature: *const RelayStr, + secret: *const RelayStr, + max_age: u32, + ) -> Result { + let max_age = match max_age { + 0 => None, + m => Some(Duration::seconds(i64::from(m))), + }; + + let response = RegisterResponse::unpack( + (*data).as_bytes(), + (*signature).as_str(), + (*secret).as_str().as_bytes(), + max_age, + )?; + + Ok(RelayStr::from_string(serde_json::to_string(&response)?)) } } diff --git a/relay-cabi/src/core.rs b/relay-cabi/src/core.rs index 08e75975f2..b53441b6f8 100644 --- a/relay-cabi/src/core.rs +++ b/relay-cabi/src/core.rs @@ -32,6 +32,7 @@ pub enum RelayErrorCode { UnpackErrorBadSignature = 1003, UnpackErrorBadPayload = 1004, UnpackErrorSignatureExpired = 1005, + UnpackErrorBadEncoding = 1006, // relay_general::types::annotated::ProcessingAction ProcessingErrorInvalidTransaction = 2001, @@ -67,6 +68,7 @@ impl RelayErrorCode { UnpackError::BadSignature => RelayErrorCode::UnpackErrorBadSignature, UnpackError::BadPayload(..) => RelayErrorCode::UnpackErrorBadPayload, UnpackError::SignatureExpired => RelayErrorCode::UnpackErrorSignatureExpired, + UnpackError::BadEncoding => RelayErrorCode::UnpackErrorBadEncoding, }; } if let Some(err) = cause.downcast_ref::() { diff --git a/relay-common/src/time.rs b/relay-common/src/time.rs index dd6cbb439b..aa60741ac9 100644 --- a/relay-common/src/time.rs +++ b/relay-common/src/time.rs @@ -3,6 +3,8 @@ use std::fmt; use std::time::{Duration, Instant, SystemTime}; +use serde::{Deserialize, Serialize}; + /// Converts an `Instant` into a `SystemTime`. pub fn instant_to_system_time(instant: Instant) -> SystemTime { SystemTime::now() - instant.elapsed() @@ -64,6 +66,14 @@ impl fmt::Debug for UnixTimestamp { } } +impl std::ops::Add for UnixTimestamp { + type Output = Self; + + fn add(self, rhs: Duration) -> Self::Output { + Self(self.0 + rhs) + } +} + impl std::ops::Sub for UnixTimestamp { type Output = Duration; @@ -71,3 +81,22 @@ impl std::ops::Sub for UnixTimestamp { self.0 - rhs.0 } } + +impl Serialize for UnixTimestamp { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_u64(self.0.as_secs()) + } +} + +impl<'de> Deserialize<'de> for UnixTimestamp { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let secs = u64::deserialize(deserializer)?; + Ok(Self::from_secs(secs)) + } +} diff --git a/relay-server/src/actors/upstream.rs b/relay-server/src/actors/upstream.rs index 05008b9471..3982817391 100644 --- a/relay-server/src/actors/upstream.rs +++ b/relay-server/src/actors/upstream.rs @@ -495,7 +495,7 @@ impl Handler for UpstreamRelay { .into_actor(self) .and_then(|challenge, slf, ctx| { log::debug!("got register challenge (token = {})", challenge.token()); - let challenge_response = challenge.create_response(); + let challenge_response = challenge.into_response(); log::debug!("sending register challenge response"); let fut = slf.send_query(challenge_response).into_actor(slf); From 14c544190f9ee0bddd1de0e0ee5f6c0ecb647cf3 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 1 Sep 2020 16:06:34 +0200 Subject: [PATCH 2/6] ref: Rename ChallengeState to RegisterState --- relay-auth/src/lib.rs | 49 +++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index 9e35077676..74b6009c34 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -428,36 +428,36 @@ pub fn generate_key_pair() -> (SecretKey, PublicKey) { (SecretKey { inner: kp }, PublicKey { inner: pk }) } -/// An encoded and signed `ChallengeState`. +/// An encoded and signed `RegisterState`. /// /// This signature can be used by the upstream server to ensure that the downstream client did not /// tamper with the token without keeping state between requests. For more information, see -/// `ChallengeState`. +/// `RegisterState`. /// -/// The format and contents of `SignedChallengeState` are intentionally opaque. Downstream clients +/// The format and contents of `SignedRegisterState` are intentionally opaque. Downstream clients /// do not need to interpret it, and the upstream can change its contents at any time. Parsing and /// validation is only performed on the upstream. /// /// In the current implementation, the serialized state has the format `{state}:{signature}`, where /// each component is: -/// - `state`: A URL-safe base64 encoding of the JSON serialized `ChallengeState`. +/// - `state`: A URL-safe base64 encoding of the JSON serialized `RegisterState`. /// - `signature`: A URL-safe base64 encoding of the SHA512 HMAC of the encoded state. /// -/// To create a signed challenge, use `RegisterChallenge::sign`. To validate the signature and read +/// To create a signed state, use `RegisterChallenge::sign`. To validate the signature and read /// the state, use `SignedRegisterChallenge::unpack`. In both cases, a secret for signing has to be /// supplied. #[derive(Clone, Debug, Deserialize, Serialize)] -pub struct SignedChallengeState(String); +pub struct SignedRegisterState(String); -impl SignedChallengeState { - /// Creates an Hmac instance for signing the `ChallengeState`. +impl SignedRegisterState { + /// Creates an Hmac instance for signing the `RegisterState`. fn mac(secret: &[u8]) -> Hmac { Hmac::new_varkey(secret).expect("HMAC takes variable keys") } - /// Signs the given `ChallengeState` and serializes it into a single string. - fn sign(state: ChallengeState, secret: &[u8]) -> Self { - let json = serde_json::to_string(&state).expect("relay challenge state serializes to JSON"); + /// Signs the given `RegisterState` and serializes it into a single string. + fn sign(state: RegisterState, secret: &[u8]) -> Self { + let json = serde_json::to_string(&state).expect("relay register state serializes to JSON"); let token = base64::encode_config(&json, base64::URL_SAFE_NO_PAD); let mut mac = Self::mac(secret); @@ -486,7 +486,7 @@ impl SignedChallengeState { &self, secret: &[u8], max_age: Option, - ) -> Result { + ) -> Result { let (token, signature) = self.split(); let code = base64::decode_config(signature, base64::URL_SAFE_NO_PAD) .map_err(|_| UnpackError::BadEncoding)?; @@ -498,7 +498,7 @@ impl SignedChallengeState { let json = base64::decode_config(token, base64::URL_SAFE_NO_PAD) .map_err(|_| UnpackError::BadEncoding)?; let state = - serde_json::from_slice::(&json).map_err(UnpackError::BadPayload)?; + serde_json::from_slice::(&json).map_err(UnpackError::BadPayload)?; if let Some(max_age) = max_age { let secs = state.timestamp().as_secs() as i64; @@ -517,14 +517,14 @@ impl SignedChallengeState { /// register response. In addition to identifying information, it contains a random bit to avoid /// replay attacks. #[derive(Clone, Deserialize, Serialize)] -pub struct ChallengeState { +pub struct RegisterState { timestamp: UnixTimestamp, relay_id: RelayId, public_key: PublicKey, rand: String, } -impl ChallengeState { +impl RegisterState { /// Returns the timestamp at which the challenge was created. pub fn timestamp(&self) -> UnixTimestamp { self.timestamp @@ -541,7 +541,7 @@ impl ChallengeState { } } -/// Generates a new random token for the challenge state. +/// Generates a new random token for the register state. fn random_token() -> String { let mut rng = thread_rng(); let mut bytes = vec![0u8; 64]; @@ -549,10 +549,10 @@ fn random_token() -> String { base64::encode_config(&bytes, base64::URL_SAFE_NO_PAD) } -/// Represents a challenge request. +/// Represents a request for registration with the upstream. /// /// This is created if the relay signs in for the first time. The server needs -/// to respond to this challenge with a unique token that is then used to sign +/// to respond to this request with a unique token that is then used to sign /// the response. #[derive(Serialize, Deserialize, Debug)] pub struct RegisterRequest { @@ -599,7 +599,7 @@ impl RegisterRequest { /// Creates a register challenge for this request. pub fn into_challenge(self, secret: &[u8]) -> RegisterChallenge { - let state = ChallengeState { + let state = RegisterState { timestamp: UnixTimestamp::now(), relay_id: self.relay_id, public_key: self.public_key, @@ -608,7 +608,7 @@ impl RegisterRequest { RegisterChallenge { relay_id: self.relay_id, - token: SignedChallengeState::sign(state, secret), + token: SignedRegisterState::sign(state, secret), } } } @@ -617,7 +617,7 @@ impl RegisterRequest { #[derive(Serialize, Deserialize, Debug)] pub struct RegisterChallenge { relay_id: RelayId, - token: SignedChallengeState, + token: SignedRegisterState, } impl RegisterChallenge { @@ -640,11 +640,14 @@ impl RegisterChallenge { } } -/// Represents a response to a register challenge +/// Represents a response to a register challenge. +/// +/// The response contains the same data as the register challenge. By signing this payload +/// successfully, this Relay authenticates with the upstream. #[derive(Serialize, Deserialize, Debug)] pub struct RegisterResponse { relay_id: RelayId, - token: SignedChallengeState, + token: SignedRegisterState, } impl RegisterResponse { From f3e1b8f59e2845e81771a1a0caeee53c6134d080 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 1 Sep 2020 16:18:17 +0200 Subject: [PATCH 3/6] meta: Changelog --- py/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/py/CHANGELOG.md b/py/CHANGELOG.md index a82bba24cf..16b5fa4e7f 100644 --- a/py/CHANGELOG.md +++ b/py/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- Updates the authentication mechanism by introducing a signed register state. Signatures of `create_register_challenge` and `validate_register_response` now take a mandatory `secret` parameter, and the public key is encoded into the state. ([#743](https://github.com/getsentry/relay/pull/743)) + ## 0.5.13 *Note: This accidentally got released as 0.15.13 as well, which has since been yanked.* From c3230bfd7f999a3d214054c274381bf38b9f37ec Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 1 Sep 2020 16:46:52 +0200 Subject: [PATCH 4/6] ref(cabi): Expose more information for Sentry --- py/sentry_relay/auth.py | 6 +++++- relay-auth/src/lib.rs | 24 +++++++++++++++--------- relay-cabi/src/auth.rs | 20 +++++++++++++++++--- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/py/sentry_relay/auth.py b/py/sentry_relay/auth.py index 6813525d83..03cf377ad8 100644 --- a/py/sentry_relay/auth.py +++ b/py/sentry_relay/auth.py @@ -116,7 +116,11 @@ def validate_register_response(data, signature, secret, max_age=60): ) response = json.loads(decode_str(response_json, free=True)) - return {"relay_id": uuid.UUID(response["relay_id"]), "token": response["token"]} + return { + "relay_id": uuid.UUID(response["relay_id"]), + "token": response["token"], + "public_key": response["public_key"], + } def is_version_supported(version): diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index 74b6009c34..e7f571fecd 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -511,6 +511,12 @@ impl SignedRegisterState { } } +impl fmt::Display for SignedRegisterState { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_str().fmt(f) + } +} + /// A state structure containing relevant information from `RegisterRequest`. /// /// This struct is used to carry over information between the downstream register request and @@ -588,8 +594,8 @@ impl RegisterRequest { } /// Returns the relay ID of the registering relay. - pub fn relay_id(&self) -> &RelayId { - &self.relay_id + pub fn relay_id(&self) -> RelayId { + self.relay_id } /// Returns the new public key of registering relay. @@ -657,7 +663,7 @@ impl RegisterResponse { signature: &str, secret: &[u8], max_age: Option, - ) -> Result { + ) -> Result<(Self, RegisterState), UnpackError> { let response: Self = serde_json::from_slice(data).map_err(UnpackError::BadPayload)?; let state = response.token.unpack(secret, max_age)?; let public_key = state.public_key(); @@ -666,12 +672,12 @@ impl RegisterResponse { return Err(UnpackError::BadSignature); } - Ok(response) + Ok((response, state)) } /// Returns the relay ID of the registering relay. - pub fn relay_id(&self) -> &RelayId { - &self.relay_id + pub fn relay_id(&self) -> RelayId { + self.relay_id } /// Returns the token that needs signing. @@ -766,7 +772,7 @@ fn test_registration() { // attempt to get the data through bootstrap unpacking. let request = RegisterRequest::bootstrap_unpack(&request_bytes, &request_sig, Some(max_age)).unwrap(); - assert_eq!(request.relay_id(), &relay_id); + assert_eq!(request.relay_id(), relay_id); assert_eq!(request.public_key(), &pk); let upstream_secret = b"secret"; @@ -782,7 +788,7 @@ fn test_registration() { // sign and unsign it let (response_bytes, response_sig) = sk.pack(&response); - let response = RegisterResponse::unpack( + let (response, _) = RegisterResponse::unpack( &response_bytes, &response_sig, upstream_secret, @@ -790,7 +796,7 @@ fn test_registration() { ) .unwrap(); - assert_eq!(response.relay_id(), &relay_id); + assert_eq!(response.relay_id(), relay_id); assert_eq!(response.token(), challenge_token); } diff --git a/relay-cabi/src/auth.rs b/relay-cabi/src/auth.rs index 7c4b76d838..5db5a4394f 100644 --- a/relay-cabi/src/auth.rs +++ b/relay-cabi/src/auth.rs @@ -1,7 +1,8 @@ use chrono::Duration; +use serde::Serialize; use relay_auth::{ - generate_key_pair, generate_relay_id, PublicKey, RegisterRequest, RegisterResponse, + generate_key_pair, generate_relay_id, PublicKey, RegisterRequest, RegisterResponse, RelayId, RelayVersion, SecretKey, }; @@ -153,6 +154,13 @@ ffi_fn! { } } +#[derive(Serialize)] +struct RelayRegisterResponse<'a> { + pub relay_id: RelayId, + pub token: &'a str, + pub public_key: &'a PublicKey, +} + ffi_fn! { /// Validates a register response. unsafe fn relay_validate_register_response( @@ -166,14 +174,20 @@ ffi_fn! { m => Some(Duration::seconds(i64::from(m))), }; - let response = RegisterResponse::unpack( + let (response, state) = RegisterResponse::unpack( (*data).as_bytes(), (*signature).as_str(), (*secret).as_str().as_bytes(), max_age, )?; - Ok(RelayStr::from_string(serde_json::to_string(&response)?)) + let relay_response = RelayRegisterResponse { + relay_id: response.relay_id(), + token: response.token(), + public_key: state.public_key(), + }; + + Ok(RelayStr::from_string(serde_json::to_string(&relay_response)?)) } } From c6bdd4c2534c77878b45b3eab558f77aa92f657f Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 1 Sep 2020 16:57:52 +0200 Subject: [PATCH 5/6] ref: Rename rand to nonce --- relay-auth/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index e7f571fecd..d17890a74c 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -527,7 +527,7 @@ pub struct RegisterState { timestamp: UnixTimestamp, relay_id: RelayId, public_key: PublicKey, - rand: String, + nonce: String, } impl RegisterState { @@ -548,7 +548,7 @@ impl RegisterState { } /// Generates a new random token for the register state. -fn random_token() -> String { +fn nonce() -> String { let mut rng = thread_rng(); let mut bytes = vec![0u8; 64]; rng.fill_bytes(&mut bytes); @@ -609,7 +609,7 @@ impl RegisterRequest { timestamp: UnixTimestamp::now(), relay_id: self.relay_id, public_key: self.public_key, - rand: random_token(), + nonce: nonce(), }; RegisterChallenge { From 870347b3e29ed533050bba9d1745122f448ead1f Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Tue, 1 Sep 2020 17:14:15 +0200 Subject: [PATCH 6/6] ref: Rename back --- relay-auth/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/relay-auth/src/lib.rs b/relay-auth/src/lib.rs index d17890a74c..4407317add 100644 --- a/relay-auth/src/lib.rs +++ b/relay-auth/src/lib.rs @@ -527,7 +527,7 @@ pub struct RegisterState { timestamp: UnixTimestamp, relay_id: RelayId, public_key: PublicKey, - nonce: String, + rand: String, } impl RegisterState { @@ -609,7 +609,7 @@ impl RegisterRequest { timestamp: UnixTimestamp::now(), relay_id: self.relay_id, public_key: self.public_key, - nonce: nonce(), + rand: nonce(), }; RegisterChallenge {