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(BOUN-1236): implement secret tracking #2433

Open
wants to merge 1 commit into
base: or-salt-log-5
Choose a base branch
from
Open
Changes from all 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
183 changes: 181 additions & 2 deletions rs/boundary_node/anonymization/client/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
use std::time::SystemTime;
use std::{sync::Arc, time::SystemTime};

use anonymization_interface::{self as ifc};
use anyhow::{anyhow, Context};
use anyhow::{anyhow, Context, Error};
use async_trait::async_trait;
use candid::{Decode, Encode, Principal};
use ic_canister_client::Agent;
use rsa::{
pkcs1::{DecodeRsaPublicKey, EncodeRsaPublicKey},
rand_core::CryptoRngCore,
Pkcs1v15Encrypt, RsaPrivateKey, RsaPublicKey,
};

const SALT_SIZE: usize = 64;
const RSA_KEY_SIZE: usize = 2048;

#[derive(Debug, thiserror::Error)]
pub enum RegisterError {
Expand Down Expand Up @@ -251,3 +259,174 @@ impl Submit for Canister {
}
}
}

// Canister methods

pub struct CanisterMethods {
/// register method tied to canister
register: Arc<dyn Register>,

/// query method tied to canister
query: Arc<dyn Query>,

/// submit method tied to canister
submit: Arc<dyn Submit>,
}

impl From<Canister> for CanisterMethods {
fn from(value: Canister) -> Self {
Self {
register: Arc::new(value.clone()),
query: Arc::new(value.clone()),
submit: Arc::new(value.clone()),
}
}
}

// Client

#[async_trait]
pub trait Track: Sync + Send {
async fn track(&mut self, cb: impl Fn(Vec<u8>) + Send + Sync) -> Result<(), Error>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to make this trait immutable and simply return the salt, and have another struct e.g. PeriodicTracker, which would execute this track() in a loop?

}

pub struct Tracker {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: SaltTracker

/// rng for generating a salt when needed
rng: Box<dyn CryptoRngCore + Send + Sync>,

/// canister client for salt sharing
canister: CanisterMethods,

/// Ephemeral private key for identifying client
pkey: RsaPrivateKey,

/// Current value of the salt
cur: Option<Vec<u8>>,
}

impl Tracker {
pub fn new(
mut rng: Box<dyn CryptoRngCore + Send + Sync>,
canister: CanisterMethods,
) -> Result<Self, Error> {
// Generate private key
let pkey = RsaPrivateKey::new(&mut rng, RSA_KEY_SIZE)
.context("failed to generate rsa private key")?;

Ok(Self {
rng,
canister,
pkey,
cur: None,
})
}

fn vec_pubkey(&self) -> Vec<u8> {
self.pkey
.to_public_key()
.to_pkcs1_der()
.expect("failed to encode public-key")
.to_vec()
}
}

#[async_trait]
impl Track for Tracker {
async fn track(&mut self, cb: impl Fn(Vec<u8>) + Send + Sync) -> Result<(), Error> {
// Register public-key
loop {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider having delays and probably max retries.

if self
.canister
.register
.register(&self.vec_pubkey())
.await
.is_ok()
{
break;
}
}

loop {
Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to do one loop after the other, maybe use one loop then? Is it ok to get stuck in this loop forever?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function is relatively long, consider breaking it down into smaller ones. This would improve readability and make the function logic cleaner.

match self.canister.query.query().await {
// Ok means we got a new salt value
Ok(ct) => {
// Decrypt salt
let salt = match self.pkey.decrypt(
Pkcs1v15Encrypt, // padding
&ct, // ciphertext
) {
Ok(v) => v,

// Retry on failure
Err(_) => continue,
};

// Set value
self.cur = Some(salt.to_owned());

// Trigger callback
cb(salt);
}

// Leader means we're being asked to generate a salt
// and encrypt it for others
Err(QueryError::Leader(mode, pairs)) => {
let salt = match mode {
// Generate salt
LeaderMode::Bootstrap => {
let mut salt = vec![0u8; SALT_SIZE];
self.rng.fill_bytes(&mut salt);
salt
}

LeaderMode::Refresh => {
match &self.cur {
// Re-use existing salt
Some(salt) => salt.to_owned(),

// Do nothing
None => continue,
}
}
};

// Encrypt salt for each principal
let mut out = vec![];

for Pair(p, pk) in pairs {
// Parse public-key
let pubkey = match RsaPublicKey::from_pkcs1_der(&pk) {
Ok(v) => v,

// Skip invalid keys
Err(_) => continue,
};

// Encrypt salt for principal
let ct = match pubkey.encrypt(
&mut self.rng, // rng
Pkcs1v15Encrypt, // padding
&salt, // msg
) {
Ok(v) => v,

// Skip on failure
Err(_) => continue,
};

// Append to result
out.push(Pair(
p, // principal
ct, // ciphertext
));
}

// Submit encrypted salt values
let _ = self.canister.submit.submit(&out).await;
}

Err(_) => continue,
}
}
}
}