initial pepper service for keyless accounts #12158

zjma · 2024-02-22T03:21:43Z

Description

TODOs

Rename oidb/pepper to oidb/pepper-service
Rename oidb/ to keyless/

Test Plan

trunk-io · 2024-02-22T03:21:49Z

⏱️ 24h 48m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
windows-build	10h 46m	🟩 🟩 🟩 🟩 🟩 (+26 more)
rust-unit-tests	5h 55m	🟥 🟥 🟥 🟥 🟩 (+25 more)
run-tests-main-branch	3h 22m	🟥 🟥 🟥 🟥 🟥 (+26 more)
rust-lints	1h 32m	🟥 🟥 🟥 🟥 🟩 (+25 more)
check-dynamic-deps	1h 3m	🟩 🟩 🟩 🟩 🟩 (+26 more)
general-lints	1h 1m	🟩 🟩 🟩 🟩 🟩 (+25 more)
check	35m	🟩 🟩 🟩 🟩 🟩 (+26 more)
semgrep/ci	15m	🟩 🟩 🟩 🟩 🟩 (+26 more)
file_change_determinator	6m	🟩 🟩 🟩 🟩 🟩 (+26 more)
file_change_determinator	5m	🟩 🟩 🟩 🟩 🟩 (+26 more)
permission-check	4m	🟩 🟩 🟩 🟩 🟩 (+26 more)
permission-check	2m	🟩 🟩 🟩 🟩 🟩 (+26 more)
permission-check	1m	🟩 🟩 🟩 🟩 🟩 (+26 more)
permission-check	1m	🟩 🟩 🟩 🟩 🟩 (+26 more)

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
run-tests-main-branch	6m	4m
windows-build	12m	20m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

oidb/pepper/common/src/asymmetric_encryption/mod.rs

oidb/pepper/common/src/vuf/mod.rs

oidb/pepper/service/src/jwk.rs

gedigi · 2024-02-23T01:04:15Z

oidb/pepper/service/src/lib.rs

+pub type KeyID = String;
+
+/// The core processing logic of this pepper service.
+pub async fn process(request: PepperRequest) -> anyhow::Result<String> {


should there be any data validation (e.g. max/expected sizes) throughout this function? there's a lot of deserialization and parsing with user-controlled input which could potentially be expensive.

oidb/pepper/service/src/lib.rs

oidb/pepper/common/src/nonce_derivation/mod.rs

zjma · 2024-02-26T05:14:03Z

oidb/pepper/service/src/vuf_keys.rs

@@ -16,7 +17,7 @@ pub static VUF_SCHEME0_SK: Lazy<ark_bls12_381::Fr> = Lazy::new(|| {
    let vuf_key_hex =
        std::env::var("VRF_KEY_HEX").expect("VRF_KEY_HEX is required for pepper calculation");
    let sk_bytes = hex::decode(vuf_key_hex).expect("vrf_key_hex should be a valid hex string");
-    ark_bls12_381::Fr::deserialize_compressed(sk_bytes.as_slice()).unwrap()
+    ark_bls12_381::Fr::from_be_bytes_mod_order(sk_bytes.as_slice())


@heliuchuan let's not use *_mod_order as it accepts any byte string, ircc.
Reverse the byte string and use deserialize_compressed instead.

nvm, what's from envvar will be a 256-bit seed and we need to derive sk from the seed.

oidb/pepper/common/src/lib.rs

oidb/pepper/service/src/lib.rs

oidb/pepper/common/src/vrf/mod.rs

oidb/pepper/common/src/asymmetric_encryption/mod.rs

oidb/pepper/common/src/elgamal/curve25519.rs

aluon

The dev_setup.sh changes look good to me

aluon · 2024-02-26T19:57:19Z

scripts/dev_setup.sh

@@ -892,6 +893,9 @@ while getopts "btoprvydaPJh:i:n" arg; do
  n)
    OPT_DIR="true"
    ;;
+  k)


Could you also update usage() to describe what the -k flag is used for?

The current dev_setup change is actually a hack... (I'm surprised you didn't hate it lol, because below it's even trying to --break-system-packages)

I could not figure out how to use existing dockerfile templates for pepper service, and the current dockerfile for pepper service is quick and dirty.

I will figure out the right way to docker build and revert this part. But if you are ok with the hack, i guess i can keep using the hack until i figure out?

I think it's fine if this is just a quick hack

How is this image going to be built? If we want to use the GitHub workflows to automate this we probably want to do something similar to the other Dockerfiles for caching

alinush · 2024-02-26T20:28:31Z

oidb/pepper/common/src/jwt.rs

+    .map_err(|e| anyhow!("jwt decoding error: {}", e))
+}
+
+static DUMMY_DECODING_KEY: Lazy<DecodingKey> = Lazy::new(|| DecodingKey::from_secret(&[]));


What is this? Can you add a comment? Is this the JWK? If so, why are you calling it a decoding key?

they are required by the jsonwebtoken library to parse jwt without verifing sig (its default behavior is to parse and verify both in one call, but we don't want it).

It's JWK, but the library call it DecodingKey...

I found another library jwt with API that makes more sense, but yet to replace this.

I see. No worries, just move the statics inside the function if that's the only place where they are used?

Plus, in the end we will need to unify the JWT logic so that we can use the same calls in both the prover service, pepper service and the Rust authenticator.

oidb/pepper/common/src/jwt.rs

alinush · 2024-02-26T20:32:35Z

oidb/pepper/common/src/lib.rs

+}
+
+#[derive(Debug, Default, Deserialize, Serialize)]
+pub struct EncryptionPubKey {


Is this even used anywhere?

no, since we are not doing encryption at the moment.

Will it be used?

now i remember this can be gone, given EphemeralPublicKey in main.

sionescu · 2024-02-26T21:33:57Z

CODEOWNERS

@@ -98,6 +98,11 @@
 # Owners for the network and all its subdirectories.
 /network/ @gregnazario @joshlind @brianolson

+# Owners for the scripts
+/scripts/ @aptos-labs/devinfra


Let's make this @aptos-labs/prod-eng.

@aluon said to use devinfra. Should I change it to prod-eng or add prod-eng next to it?

@aluon do we need a different team here ?

Also @aluon, there seems to be an error regarding the devinfra team not existing.

Let's set this to aptos-labs/prod-eng for now. I was thinking we could create a devinfra team so we can add other folks but I'm not sure why it's erroring

@zjma this was not resolved. Can you change @aptos-labs/devinfra to @aptos-labs/prod-eng?

i didn't add this diff... why is it even in this PR?

anyway fixed

alinush · 2024-02-26T22:28:09Z

oidb/pepper/service/src/about.rs

+    git_commit: String,
+}
+
+pub static ABOUT_JSON: Lazy<String> = Lazy::new(|| {


Can you add comments? What is this?

this is for showing some metadata at /about.
e.g. the commit SHA1

alinush · 2024-02-26T22:29:08Z

oidb/pepper/service/src/lib.rs

+    let (pepper, vuf_proof) = vuf::scheme0::Scheme0::eval(&VUF_SCHEME0_SK, &input_bytes)?;
+    ensure!(vuf_proof.is_empty(), "internal proof error");
+    let pepper_hexlified = hex::encode(pepper);
+    Ok(pepper_hexlified)


Why are you not returning the proof here? @heliuchuan will want to verify the proof in the SDK.

because the proof is empty?

For BLS, sure, but when you change the scheme, will you put the proof inside pepper_hexlified?

in that case, pepper should be bcs::to_bytes(some_struct_that_include_output_and_proof)?

Sure, but the problem with this PR is that we do not have clean separation between structs and their serialization.

e.g., why is PepperResponse containing hex strings? It should have Vec<u8>'s and the serialization code (BCS, JSON) will decide the format.

oidb/pepper/service/src/vuf_keys.rs

alinush · 2024-02-26T23:22:33Z

oidb/pepper/service/src/vuf_keys.rs

+use once_cell::sync::Lazy;
+use ark_ff::PrimeField;
+
+pub struct VufScheme0Sk {


This is not even used...

alinush · 2024-02-27T00:22:15Z

oidb/pepper/common/src/lib.rs

+}
+
+/// The response to `PepperRequestV0`, which contains the calculated pepper (hexlified) or a processing error.
+#[derive(Debug, Deserialize, Serialize)]


Please add comments and explain these fields.

Not sure why there are two things here.

There is just one pepper.

oidb/pepper/service/src/lib.rs

oidb/pepper/service/Dockerfile

alinush · 2024-02-29T01:36:46Z

Cargo.toml

@@ -384,6 +385,8 @@ aptos-package-builder = { path = "aptos-move/package-builder" }
 aptos-peer-monitoring-service-client = { path = "peer-monitoring-service/client" }
 aptos-peer-monitoring-service-server = { path = "peer-monitoring-service/server" }
 aptos-peer-monitoring-service-types = { path = "peer-monitoring-service/types" }
+aptos-keyless-pepper-common = { path = "keyless/pepper/common" }


We will likely move most of this into crates/aptos-crypto and types over time.

alinush

Stamping assuming you've addressed the request/response formatting issues. Don't have time to look right now.

heliuchuan

stamp request response lgtm

v0 sennddddd

note eventually i think we should use axum over hyper

heliuchuan · 2024-02-29T21:39:38Z

keyless/pepper/service/src/main.rs

+            .header(ACCESS_CONTROL_ALLOW_ORIGIN, origin)
+            .header(ACCESS_CONTROL_ALLOW_CREDENTIALS, "true")
+            .header(ACCESS_CONTROL_ALLOW_METHODS, "GET, POST, OPTIONS")
+            .header(ACCESS_CONTROL_ALLOW_HEADERS, "Content-Type, Authorization")


let's set this to "*" for now

github-actions · 2024-03-01T10:11:28Z

✅ Forge suite `realistic_env_max_load` success on `477f395cdbd35d3a7e4228bfcdf480a4082c9fe6`

two traffics test: inner traffic : committed: 7979 txn/s, latency: 4918 ms, (p50: 4800 ms, p90: 5700 ms, p99: 10400 ms), latency samples: 3439100
two traffics test : committed: 100 txn/s, latency: 1880 ms, (p50: 1800 ms, p90: 2000 ms, p99: 6100 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.230, avg: 0.203", "QsPosToProposal: max: 0.320, avg: 0.268", "ConsensusProposalToOrdered: max: 0.455, avg: 0.422", "ConsensusOrderedToCommit: max: 0.296, avg: 0.272", "ConsensusProposalToCommit: max: 0.738, avg: 0.694"]
Max round gap was 1 [limit 4] at version 1652910. Max no progress secs was 4.41434 [limit 15] at version 1652910.
Test Ok

github-actions · 2024-03-01T10:11:53Z

✅ Forge suite `compat` success on `aptos-node-v1.9.5` ==> `477f395cdbd35d3a7e4228bfcdf480a4082c9fe6`

Compatibility test results for aptos-node-v1.9.5 ==> 477f395cdbd35d3a7e4228bfcdf480a4082c9fe6 (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 6988 txn/s, latency: 4726 ms, (p50: 4800 ms, p90: 7300 ms, p99: 7800 ms), latency samples: 244600
2. Upgrading first Validator to new version: 477f395cdbd35d3a7e4228bfcdf480a4082c9fe6
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 689 txn/s, latency: 35500 ms, (p50: 38700 ms, p90: 53700 ms, p99: 56400 ms), latency samples: 56500
3. Upgrading rest of first batch to new version: 477f395cdbd35d3a7e4228bfcdf480a4082c9fe6
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 229 txn/s, submitted: 543 txn/s, expired: 313 txn/s, latency: 38004 ms, (p50: 41800 ms, p90: 56900 ms, p99: 61400 ms), latency samples: 19315
4. upgrading second batch to new version: 477f395cdbd35d3a7e4228bfcdf480a4082c9fe6
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2330 txn/s, latency: 12640 ms, (p50: 12300 ms, p90: 17800 ms, p99: 18400 ms), latency samples: 111880
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> 477f395cdbd35d3a7e4228bfcdf480a4082c9fe6 passed
Test Ok

zjma marked this pull request as ready for review February 22, 2024 03:21

zjma requested review from alinush and heliuchuan February 22, 2024 03:21

zjma commented Feb 22, 2024

View reviewed changes

oidb/pepper/common/src/asymmetric_encryption/mod.rs Outdated Show resolved Hide resolved

zjma commented Feb 22, 2024

View reviewed changes

oidb/pepper/common/src/vuf/mod.rs Outdated Show resolved Hide resolved

gedigi reviewed Feb 23, 2024

View reviewed changes

oidb/pepper/service/src/jwk.rs Outdated Show resolved Hide resolved

gedigi reviewed Feb 23, 2024

View reviewed changes

oidb/pepper/service/src/lib.rs Outdated Show resolved Hide resolved

zjma commented Feb 23, 2024

View reviewed changes

oidb/pepper/common/src/nonce_derivation/mod.rs Outdated Show resolved Hide resolved

zjma commented Feb 26, 2024

View reviewed changes

oidb/pepper/common/src/lib.rs Outdated Show resolved Hide resolved

zjma commented Feb 26, 2024

View reviewed changes

oidb/pepper/service/src/lib.rs Outdated Show resolved Hide resolved

zjma commented Feb 26, 2024

View reviewed changes

oidb/pepper/common/src/vrf/mod.rs Outdated Show resolved Hide resolved