Paths

Table of Contentst

-
services/tunnelbroker/src/libcpp/src/Tools/
-
tunnelbroker/
-
src/
-
libcpp/
-
src/
-
Tools/
1/2
Tools.cpp

[services] Tunnelbroker - Skipping `deviceID` validation in tools function
ClosedPublic
Actions

Authored by • max on Feb 2 2023, 4:50 AM.

Details

Reviewers

• jon
bartek

Commits

rCOMM044b8239d2c4: [services] Tunnelbroker - Skipping `deviceID` validation in tools function

Summary

This diff adds skipping of the deviceID format validation in the validateDeviceID tools function by checking the sessions.disable_deviceid_validation config flag (from D6523).

We are disabling the validation of the deviceID format for using the Tunnelbroker as a Redis replacement, in this case, the deviceID format will be different and we should not validate it.

Linear task: ENG-2640

Test Plan

CI checks are successfully passed.
Manual testing:

Adding the disable_deviceid_validation flag to the [sessions] section of the config file results in always true returns from the validateDeviceID tools function.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Feb 2 2023, 4:50 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptFeb 2 2023, 4:50 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript

• max added a parent revision: D6523: [services] Tunnelbroker - Add disable `deviceID` format validation config parameter.Feb 2 2023, 4:50 AM

• max edited the summary of this revision. (Show Details)Feb 2 2023, 4:59 AM

• max edited the test plan for this revision. (Show Details)

• max added a reviewer: bartek.

• max added inline comments.

services/tunnelbroker/src/libcpp/src/Tools/Tools.cpp
30 ↗	(On Diff #21843)	Logging here in case we are skipping the validation is a good debugging point.

• max published this revision for review.Feb 2 2023, 5:00 AM

Harbormaster completed remote builds in B15999: Diff 21843.Feb 2 2023, 5:04 AM

• jon accepted this revision.Feb 2 2023, 10:18 AM

This revision is now accepted and ready to land.Feb 2 2023, 10:18 AM

ashoat added inline comments.Feb 2 2023, 11:44 AM

services/tunnelbroker/src/libcpp/src/Tools/Tools.cpp
32 ↗	(On Diff #21843)	`deviceID` is user-identifiable... this creates a log of all the times a given user has accessed Tunnelbroker. I'd rather we didn't implicitly create access logs like this. Is `LOG(INFO)` going to be active in production?

Removing logging.

• max marked an inline comment as done.Feb 15 2023, 4:00 PM

• max added inline comments.

services/tunnelbroker/src/libcpp/src/Tools/Tools.cpp
32 ↗	(On Diff #21843)	`deviceID` is user-identifiable... this creates a log of all the times a given user has accessed Tunnelbroker. I'd rather we didn't implicitly create access logs like this. Makes sense to remove this logging. Is `LOG(INFO)` going to be active in production? No, `ERROR` and `FATAL` are going to be printed in production.

Harbormaster completed remote builds in B16546: Diff 22640.Feb 15 2023, 4:10 PM

Closed by commit rCOMM044b8239d2c4: [services] Tunnelbroker - Skipping `deviceID` validation in tools function (authored by • max). · Explain WhyFeb 15 2023, 4:15 PM

This revision was automatically updated to reflect the committed changes.

• max marked an inline comment as done.

• max added a commit: rCOMM044b8239d2c4: [services] Tunnelbroker - Skipping `deviceID` validation in tools function.

Revision Contents
Changeset List

Path

Size

services/

tunnelbroker/

src/

libcpp/

src/

Tools/

Tools.cpp

4 lines

Diff 22641

View Options

services/tunnelbroker/src/libcpp/src/Tools/Tools.cpp

use derive_more::{Display, Error, From};		use derive_more::{Display, Error, From};
use futures_core::Stream;		use futures_core::Stream;
use futures_util::StreamExt;		use futures_util::{StreamExt, TryStreamExt};
use reqwest::Url;		use reqwest::{
		multipart::{Form, Part},
		Body, Url,
		};
use tracing::{debug, trace, warn};		use tracing::{debug, trace, warn};

// publicly re-export some reqwest types		// publicly re-export some reqwest types
pub use reqwest::Error as ReqwestError;		pub use reqwest::Error as ReqwestError;
pub use reqwest::StatusCode;		pub use reqwest::StatusCode;

#[derive(From, Error, Debug, Display)]		#[derive(From, Error, Debug, Display)]
pub enum BlobServiceError {		pub enum BlobServiceError {
▲ Show 20 Lines • Show All 91 Lines • ▼ Show 20 Lines	) -> BlobResult<impl Stream<Item = BlobResult<Vec<u8>>>> {
}		}

let error = handle_http_error(response.status());		let error = handle_http_error(response.status());
if let Ok(message) = response.text().await {		if let Ok(message) = response.text().await {
trace!("Error response message: {}", message);		trace!("Error response message: {}", message);
}		}
Err(error)		Err(error)
}		}

		/// Uploads a blob. Returns `BlobServiceError::AlreadyExists` if blob with given hash
		/// already exists.
		///
		/// # Example
		/// ```rust
		/// use std::io::{Error, ErrorKind};
		///
		/// let client =
		/// BlobServiceClient::new("http://localhost:50053".parse()?);
		///
		/// let stream = async_stream::stream! {
		/// yield Ok(vec![1, 2, 3]);
		/// yield Ok(vec![4, 5, 6]);
		/// yield Err(Error::new(ErrorKind::Other, "Oops"));
		/// };
		/// client.upload_blob(&blob_hash, stream).await?;
		/// ```
		pub async fn upload_blob<H, S>(
		&self,
		blob_hash: H,
		data_stream: S,
		) -> BlobResult<()>
		where
		H: Into<String>,
		S: futures_core::stream::TryStream + Send + Sync + 'static,
		S::Error: Into<Box<dyn std::error::Error + Send + Sync>>,
		Vec<u8>: From<S::Ok>,
		bartekAuthorUnsubmitted Done Inline Actions Requirements of the reqwest body stream: A stream of `Result<impl Into<Vec<u8>>, impl Into<Box<dyn Error>>>` bartek: Requirements of the reqwest body stream: A stream of `Result<impl Into<Vec<u8>>, impl…
		{
		debug!("Revoke holder request");
		michalUnsubmitted Not Done Inline Actions Wrong message? michal: Wrong message?
		let url = self.get_blob_url(None)?;

		let stream = data_stream.map_ok(Vec::from);
		let streaming_body = Body::wrap_stream(stream);
		let form = Form::new()
		.text("blob_hash", blob_hash.into())
		.part("blob_data", Part::stream(streaming_body));

		let response = self.http_client.put(url).multipart(form).send().await?;
		debug!("Response status: {}", response.status());

		if response.status().is_success() {
		trace!("Blob upload successful");
		return Ok(());
		}

		let error = handle_http_error(response.status());
		if let Ok(message) = response.text().await {
		trace!("Error response message: {}", message);
		}
		Err(error)
		}
}		}

// private helper methods		// private helper methods
impl BlobServiceClient {		impl BlobServiceClient {
fn get_blob_url(		fn get_blob_url(
&self,		&self,
blob_hash: Option<&str>,		blob_hash: Option<&str>,
) -> Result<Url, BlobServiceError> {		) -> Result<Url, BlobServiceError> {
Show All 24 Lines

[services] Tunnelbroker - Skipping `deviceID` validation in tools functionClosedPublicActions