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

refactor: get_nullifier_keys cleanup #6451

Merged
merged 6 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
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
5 changes: 2 additions & 3 deletions docs/docs/aztec/aztec/concepts/accounts/keys.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,9 @@ This is a snippet of our Schnorr Account contract implementation, which uses Sch
Still, different accounts may use different signing schemes, may require multi-factor authentication, or _may not even use signing keys_ and instead rely on other authentication mechanisms. Read [how to write an account contract](/tutorials/tutorials/write_accounts_contract.md) for a full example of how to manage authentication.

Furthermore, and since signatures are fully abstracted, how the key is stored in the contract is abstracted as well and left to the developer of the account contract.
Below are a few ideas on how to store them, each with their pros and cons.
In the following section we describe a few ways how an account contract could be architected to store signing keys.

### Ways to store signing keys
Below we described a few ways how an account contract could be architected to obtain signing keys.
### Storing signing keys
benesjan marked this conversation as resolved.
Show resolved Hide resolved

#### Using a private note

Expand Down
2 changes: 1 addition & 1 deletion noir-projects/aztec-nr/address-note/src/address_note.nr
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use dep::aztec::{
grumpkin_point::GrumpkinPoint, hash::poseidon2_hash
},
note::{note_header::NoteHeader, note_interface::NoteInterface, utils::compute_note_hash_for_consumption},
oracle::unsafe_rand::unsafe_rand, keys::getters::get_nsk_app, context::PrivateContext
oracle::unsafe_rand::unsafe_rand, oracle::nullifier_keys::get_nsk_app, context::PrivateContext
};

global ADDRESS_NOTE_LEN: Field = 3;
Expand Down
38 changes: 18 additions & 20 deletions noir-projects/aztec-nr/aztec/src/context/private_context.nr
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use crate::{
context::{inputs::PrivateContextInputs, interface::ContextInterface},
keys::getters::get_nullifier_keys, messaging::process_l1_to_l2_message,
messaging::process_l1_to_l2_message,
hash::{hash_args_array, ArgsHasher, compute_encrypted_log_hash, compute_unencrypted_log_hash},
note::{note_interface::NoteInterface, utils::compute_note_hash_for_insertion},
oracle::{
nullifier_keys::NullifierKeys, arguments, returns,
nullifier_keys::get_nullifier_key_validation_request, arguments, returns,
call_private_function::call_private_function_internal, header::get_header_at,
logs::emit_encrypted_log, logs_traits::{LensForEncryptedLog, ToBytesForUnencryptedLog},
enqueue_public_function_call::{
Expand Down Expand Up @@ -71,7 +71,7 @@ struct PrivateContext {
encrypted_log_preimages_length: Field,
unencrypted_log_preimages_length: Field,

nullifier_key: Option<NullifierKeys>,
last_nullifier_key_validation_request: Option<NullifierKeyValidationRequest>,
}

impl ContextInterface for PrivateContext {
Expand Down Expand Up @@ -133,7 +133,7 @@ impl PrivateContext {
unencrypted_logs_hashes: BoundedVec::new(),
encrypted_log_preimages_length: 0,
unencrypted_log_preimages_length: 0,
nullifier_key: Option::none()
last_nullifier_key_validation_request: Option::none()
}
}

Expand Down Expand Up @@ -208,24 +208,22 @@ impl PrivateContext {
}

pub fn request_nsk_app(&mut self, npk_m_hash: Field) -> Field {
// A value of empty nullifier keys will fail the key validation request.
let cached_nullifier_keys = self.nullifier_key.unwrap_or(NullifierKeys::empty());
let cached_request = self.last_nullifier_key_validation_request.unwrap_or(NullifierKeyValidationRequest::empty());

let nullifier_keys = if cached_nullifier_keys.master_nullifier_public_key.hash() == npk_m_hash {
cached_nullifier_keys
if cached_request.master_nullifier_public_key.hash() == npk_m_hash {
// We get a match so the cached request is the latest one
cached_request.app_nullifier_secret_key
} else {
let fetched_nullifier_keys = get_nullifier_keys(npk_m_hash);
let request = NullifierKeyValidationRequest {
master_nullifier_public_key: fetched_nullifier_keys.master_nullifier_public_key,
app_nullifier_secret_key: fetched_nullifier_keys.app_nullifier_secret_key
};
// We didn't get a match meaning the cached result is stale. We fetch new values from oracle and instruct
// protocol circuits to validate them by storing the validation request in context.
let request = get_nullifier_key_validation_request(npk_m_hash);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not care that the oracle can inject an arbitrary nullifier key validation request, even if it does not correspond to the npk_m_hash ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was non-sense. Fixed in 776538f

Thanks for pointing out

// We constrain that the npk_m_hash matches the one in the request (otherwise we could get an arbitrary
// valid key request and not the one corresponding to npk_m_hash).
assert(request.master_nullifier_public_key.hash() == npk_m_hash);
self.nullifier_key_validation_requests.push(request);
self.nullifier_key = Option::some(fetched_nullifier_keys);
fetched_nullifier_keys
};

assert_eq(nullifier_keys.master_nullifier_public_key.hash(), npk_m_hash);
benesjan marked this conversation as resolved.
Show resolved Hide resolved
nullifier_keys.app_nullifier_secret_key
self.last_nullifier_key_validation_request = Option::some(request);
request.app_nullifier_secret_key
}
}

// docs:start:context_message_portal
Expand Down Expand Up @@ -675,7 +673,7 @@ impl Empty for PrivateContext {
unencrypted_logs_hashes: BoundedVec::new(),
encrypted_log_preimages_length: 0,
unencrypted_log_preimages_length: 0,
nullifier_key: Option::none(),
last_nullifier_key_validation_request: Option::none(),
}
}
}
Expand Down
24 changes: 5 additions & 19 deletions noir-projects/aztec-nr/aztec/src/keys/getters.nr
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use dep::protocol_types::{address::AztecAddress, constants::CANONICAL_KEY_REGISTRY_ADDRESS, grumpkin_point::GrumpkinPoint};
use dep::protocol_types::{
abis::nullifier_key_validation_request::NullifierKeyValidationRequest, address::AztecAddress,
constants::CANONICAL_KEY_REGISTRY_ADDRESS, grumpkin_point::GrumpkinPoint
};
use crate::{
context::PrivateContext,
oracle::{
keys::get_public_keys_and_partial_address,
nullifier_keys::{NullifierKeys, get_nullifier_keys as get_nullifier_keys_oracle}
},
context::PrivateContext, oracle::{keys::get_public_keys_and_partial_address},
keys::public_keys::{PublicKeys, NULLIFIER_INDEX, INCOMING_INDEX},
state_vars::{
map::derive_storage_slot_in_map,
Expand Down Expand Up @@ -90,16 +89,3 @@ fn fetch_and_constrain_keys(address: AztecAddress) -> PublicKeys {

public_keys
}

// We get the full struct Nullifier Keys here
pub fn get_nullifier_keys(npk_m_hash: Field) -> NullifierKeys {
let nullifier_keys = get_nullifier_keys_oracle(npk_m_hash);
assert_eq(nullifier_keys.master_nullifier_public_key.hash(), npk_m_hash);

Copy link
Contributor Author

@benesjan benesjan May 16, 2024

Choose a reason for hiding this comment

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

This was just semi-constrained as the nsk_app was not checked. I initially though this is vulnerability but it turned out to not be case because we used this only in places where it was not necessary (Note::compute_nullifier_without_context).

nullifier_keys
}

// We are only getting the app_nullifier_secret_key here
pub fn get_nsk_app(npk_m_hash: Field) -> Field {
get_nullifier_keys(npk_m_hash).app_nullifier_secret_key
}
36 changes: 11 additions & 25 deletions noir-projects/aztec-nr/aztec/src/oracle/nullifier_keys.nr
Original file line number Diff line number Diff line change
@@ -1,35 +1,21 @@
use dep::protocol_types::{
address::AztecAddress, traits::Empty, grumpkin_point::GrumpkinPoint,
grumpkin_private_key::GrumpkinPrivateKey
};

// Nullifier keys pertaining to a specific account
struct NullifierKeys {
master_nullifier_public_key: GrumpkinPoint,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nuked this struct as it was the same thing as NullifierKeyValidationRequest in protocol circuits.

app_nullifier_secret_key: Field,
}

impl Empty for NullifierKeys {
fn empty() -> Self {
NullifierKeys {
master_nullifier_public_key: GrumpkinPoint::zero(),
app_nullifier_secret_key: 0
}
}
}
use dep::protocol_types::{grumpkin_point::GrumpkinPoint, abis::nullifier_key_validation_request::NullifierKeyValidationRequest};

#[oracle(getNullifierKeys)]
fn get_nullifier_keys_oracle(_npk_m_hash: Field) -> [Field; 3] {}
fn get_nullifier_key_validation_request_oracle(_npk_m_hash: Field) -> [Field; 3] {}

unconstrained fn get_nullifier_keys_internal(npk_m_hash: Field) -> NullifierKeys {
let result = get_nullifier_keys_oracle(npk_m_hash);
NullifierKeys {
unconstrained fn get_nullifier_key_validation_request_internal(npk_m_hash: Field) -> NullifierKeyValidationRequest {
let result = get_nullifier_key_validation_request_oracle(npk_m_hash);
NullifierKeyValidationRequest {
master_nullifier_public_key: GrumpkinPoint { x: result[0], y: result[1] },
app_nullifier_secret_key: result[2]
}
}

// We get the full struct Nullifier Keys here
pub fn get_nullifier_keys(npk_m_hash: Field) -> NullifierKeys {
get_nullifier_keys_internal(npk_m_hash)
pub fn get_nullifier_key_validation_request(npk_m_hash: Field) -> NullifierKeyValidationRequest {
get_nullifier_key_validation_request_internal(npk_m_hash)
}

pub fn get_nsk_app(npk_m_hash: Field) -> Field {
get_nullifier_key_validation_request_internal(npk_m_hash).app_nullifier_secret_key
}
2 changes: 1 addition & 1 deletion noir-projects/aztec-nr/value-note/src/value_note.nr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use dep::aztec::{
constants::GENERATOR_INDEX__NOTE_NULLIFIER, hash::poseidon2_hash
},
note::{note_header::NoteHeader, note_interface::NoteInterface, utils::compute_note_hash_for_consumption},
oracle::unsafe_rand::unsafe_rand, keys::getters::get_nsk_app, context::PrivateContext
oracle::unsafe_rand::unsafe_rand, oracle::nullifier_keys::get_nsk_app, context::PrivateContext
};

global VALUE_NOTE_LEN: Field = 3; // 3 plus a header.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use dep::aztec::prelude::{AztecAddress, PrivateContext, NoteHeader, NoteInterfac
use dep::aztec::{
keys::getters::get_ivpk_m,
protocol_types::{constants::GENERATOR_INDEX__NOTE_NULLIFIER, grumpkin_point::GrumpkinPoint, hash::poseidon2_hash},
note::utils::compute_note_hash_for_consumption, keys::getters::get_nsk_app
note::utils::compute_note_hash_for_consumption, oracle::nullifier_keys::get_nsk_app
};

global SUBSCRIPTION_NOTE_LEN: Field = 3;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use dep::aztec::prelude::{AztecAddress, NoteInterface, NoteHeader, PrivateContext};
use dep::aztec::{
keys::getters::get_ivpk_m, note::{utils::compute_note_hash_for_consumption},
keys::getters::get_nsk_app,
oracle::nullifier_keys::get_nsk_app,
protocol_types::{
traits::Empty, grumpkin_point::GrumpkinPoint, constants::GENERATOR_INDEX__NOTE_NULLIFIER,
hash::poseidon2_hash
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use dep::aztec::prelude::{AztecAddress, FunctionSelector, NoteHeader, NoteInterf

use dep::aztec::{
keys::getters::get_ivpk_m, note::utils::compute_note_hash_for_consumption,
keys::getters::get_nsk_app,
oracle::nullifier_keys::get_nsk_app,
protocol_types::{constants::GENERATOR_INDEX__NOTE_NULLIFIER, grumpkin_point::GrumpkinPoint, hash::poseidon2_hash}
};

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use dep::aztec::prelude::{AztecAddress, NoteHeader, NoteInterface, PrivateContext};
use dep::aztec::{
note::utils::compute_note_hash_for_consumption, keys::getters::get_nsk_app,
note::utils::compute_note_hash_for_consumption, oracle::nullifier_keys::get_nsk_app,
protocol_types::{constants::GENERATOR_INDEX__NOTE_NULLIFIER, grumpkin_point::GrumpkinPoint, hash::poseidon2_hash}
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use dep::aztec::{
prelude::{AztecAddress, NoteHeader, NoteInterface, PrivateContext},
protocol_types::{constants::GENERATOR_INDEX__NOTE_NULLIFIER, grumpkin_point::GrumpkinPoint, hash::poseidon2_hash},
note::utils::compute_note_hash_for_consumption, oracle::unsafe_rand::unsafe_rand,
keys::getters::get_nsk_app
oracle::nullifier_keys::get_nsk_app
};

trait OwnedNote {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use dep::aztec::{
prelude::{AztecAddress, NoteHeader, NoteInterface, PrivateContext},
protocol_types::{constants::GENERATOR_INDEX__NOTE_NULLIFIER, grumpkin_point::GrumpkinPoint, hash::poseidon2_hash},
note::utils::compute_note_hash_for_consumption, oracle::unsafe_rand::unsafe_rand,
keys::getters::get_nsk_app
oracle::nullifier_keys::get_nsk_app
};

trait OwnedNote {
Expand Down
Loading