An easy-to-host PDS on the ATProtocol, MacOS. Grandma-approved.
fix(relay): address remaining PR review issues for MM-92
- Fix #5: Config.signing_key_master_key leaks via Debug and clone
- Wrap signing_key_master_key in Sensitive<Zeroizing<[u8; 32]>>
- Adds Sensitive newtype that redacts Debug output to "***"
- Zeroizing ensures key bytes are securely zeroized on drop
- Never copies key into non-zeroizing allocation
- Fix #6: CreateSigningKeyRequest.algorithm should be one-variant enum
- Replace Option<String> with Algorithm enum (single P256 variant)
- Serde validates at deserialization time, not runtime
- Remove dead runtime algorithm matching code
- Updated tests to expect 422 (Unprocessable Entity) for invalid enum
- Fix #7: Remove dead CryptoError::InvalidKeyId variant
- Variant was never constructed in this PR
- Fix #8: Wrap raw_bytes in Zeroizing in keys.rs
- Ensures intermediate GenericArray from secret_key.to_bytes() is zeroized
- Guards against future changes to p256 library behavior
- Fix #9: Add PRAGMA table_info test for V003 relay_signing_keys columns
- Validates exact column order and names: id, algorithm, public_key,
private_key_encrypted, created_at
- Fix #10: Add V003 PRIMARY KEY uniqueness constraint test
- Verifies duplicate id inserts fail with constraint violation
- Fix #11: Introduce DidKeyUri newtype for P256Keypair.key_id
- Prevents silent positional swap bugs in SQL binds and API responses
- Type-safe distinction between key_id (did:key:z...) and public_key (z...)
- Converts to string for DB inserts and JSON responses
Changes:
- crates/common: Add zeroize dependency, Sensitive<T> wrapper, export it
- crates/crypto: Add DidKeyUri newtype, remove InvalidKeyId CryptoError variant
- crates/relay: Add zeroize dependency, update handler to use new types, add V003 tests
authored by
Tangled
3e96165c
6ab3ce66
+129
-37
+3
Cargo.lock
+3
Cargo.lock
···
377
377
"tokio",
378
378
"toml",
379
379
"tracing",
380
+
"zeroize",
380
381
]
381
382
382
383
[[package]]
···
1810
1811
"serde",
1811
1812
"serde_json",
1812
1813
"sqlx",
1814
+
"subtle",
1813
1815
"tempfile",
1814
1816
"thiserror",
1815
1817
"tokio",
···
1818
1820
"tracing",
1819
1821
"tracing-opentelemetry",
1820
1822
"tracing-subscriber",
1823
+
"zeroize",
1821
1824
]
1822
1825
1823
1826
[[package]]
+1
crates/common/Cargo.toml
+1
crates/common/Cargo.toml
+19
-3
crates/common/src/config.rs
+19
-3
crates/common/src/config.rs
···
1
1
use serde::Deserialize;
2
2
use std::collections::HashMap;
3
3
use std::path::PathBuf;
4
+
use zeroize::Zeroizing;
5
+
6
+
/// A wrapper that suppresses Debug output for sensitive values.
7
+
#[derive(Clone)]
8
+
pub struct Sensitive<T>(pub T);
9
+
10
+
impl<T> std::fmt::Debug for Sensitive<T> {
11
+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
12
+
f.write_str("***")
13
+
}
14
+
}
4
15
5
16
/// Validated, fully-resolved relay configuration.
6
17
#[derive(Debug, Clone)]
···
22
33
// Operator authentication for management endpoints (e.g., POST /v1/relay/keys).
23
34
pub admin_token: Option<String>,
24
35
// AES-256-GCM master key for encrypting signing key private keys at rest.
25
-
pub signing_key_master_key: Option<[u8; 32]>,
36
+
pub signing_key_master_key: Option<Sensitive<Zeroizing<[u8; 32]>>>,
26
37
}
27
38
28
39
/// Optional privacy/ToS links surfaced by `com.atproto.server.describeServer`.
···
317
328
iroh: raw.iroh,
318
329
telemetry,
319
330
admin_token: raw.admin_token,
320
-
signing_key_master_key: raw.signing_key_master_key,
331
+
signing_key_master_key: raw
332
+
.signing_key_master_key
333
+
.map(|k| Sensitive(Zeroizing::new(k))),
321
334
})
322
335
}
323
336
···
759
772
0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c,
760
773
0x1d, 0x1e, 0x1f, 0x20,
761
774
];
762
-
assert_eq!(config.signing_key_master_key, Some(expected));
775
+
assert_eq!(
776
+
config.signing_key_master_key.as_ref().map(|s| &*s.0),
777
+
Some(&expected)
778
+
);
763
779
}
764
780
765
781
#[test]
+2
-2
crates/common/src/lib.rs
+2
-2
crates/common/src/lib.rs
···
3
3
mod error;
4
4
5
5
pub use config::{
6
-
BlobsConfig, Config, ConfigError, ContactConfig, IrohConfig, OAuthConfig, ServerLinksConfig,
7
-
TelemetryConfig,
6
+
BlobsConfig, Config, ConfigError, ContactConfig, IrohConfig, OAuthConfig, Sensitive,
7
+
ServerLinksConfig, TelemetryConfig,
8
8
};
9
9
pub use config_loader::load_config;
10
10
pub use error::{ApiError, ErrorCode};
-2
crates/crypto/src/error.rs
-2
crates/crypto/src/error.rs
+18
-5
crates/crypto/src/keys.rs
+18
-5
crates/crypto/src/keys.rs
···
14
14
/// 0x1200 encoded as LEB128 varint = [0x80, 0x24].
15
15
const P256_MULTICODEC_PREFIX: &[u8] = &[0x80, 0x24];
16
16
17
+
/// A `did:key:z...` URI — the canonical identifier for a P-256 keypair.
18
+
///
19
+
/// Distinct from `public_key` (a bare multibase string) at the type level to prevent
20
+
/// positional swap bugs in SQL binds and API responses.
21
+
#[derive(Debug, Clone, PartialEq, Eq)]
22
+
pub struct DidKeyUri(pub String);
23
+
24
+
impl std::fmt::Display for DidKeyUri {
25
+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
26
+
f.write_str(&self.0)
27
+
}
28
+
}
29
+
17
30
/// A generated P-256 keypair.
18
31
///
19
32
/// `private_key_bytes` is zeroized on drop. Callers must encrypt it with
20
33
/// [`encrypt_private_key`] before storing and drop this struct promptly.
21
34
pub struct P256Keypair {
22
35
/// Full `did:key:z...` URI — use as the database primary key.
23
-
pub key_id: String,
36
+
pub key_id: DidKeyUri,
24
37
/// Multibase base58btc-encoded compressed public key point (no `did:key:` prefix).
25
38
pub public_key: String,
26
39
/// Raw 32-byte P-256 private key scalar. Zeroized on drop.
···
43
56
44
57
// multibase::encode with Base58Btc prepends the 'z' prefix automatically.
45
58
let multibase_encoded = multibase::encode(Base::Base58Btc, &multikey);
46
-
let key_id = format!("did:key:{multibase_encoded}");
59
+
let key_id = DidKeyUri(format!("did:key:{multibase_encoded}"));
47
60
let public_key_str = multibase::encode(Base::Base58Btc, compressed_bytes);
48
61
49
62
// Copy private key bytes into a Zeroizing wrapper.
50
-
let raw_bytes = secret_key.to_bytes();
63
+
let raw_bytes = Zeroizing::new(secret_key.to_bytes());
51
64
let mut private_key_bytes = Zeroizing::new([0u8; 32]);
52
65
private_key_bytes.copy_from_slice(raw_bytes.as_slice());
53
66
···
132
145
133
146
// key_id must be a valid did:key URI with P-256 multicodec prefix.
134
147
assert!(
135
-
keypair.key_id.starts_with("did:key:z"),
148
+
keypair.key_id.0.starts_with("did:key:z"),
136
149
"key_id must start with did:key:z"
137
150
);
138
151
139
152
// Decode the multibase portion and verify the multicodec prefix.
140
-
let multibase_part = keypair.key_id.strip_prefix("did:key:").unwrap();
153
+
let multibase_part = keypair.key_id.0.strip_prefix("did:key:").unwrap();
141
154
let (_, multikey_bytes) = multibase::decode(multibase_part).unwrap();
142
155
assert_eq!(
143
156
&multikey_bytes[..2],
+3
-1
crates/crypto/src/lib.rs
+3
-1
crates/crypto/src/lib.rs
+1
crates/relay/Cargo.toml
+1
crates/relay/Cargo.toml
+60
crates/relay/src/db/mod.rs
+60
crates/relay/src/db/mod.rs
···
766
766
767
767
assert_eq!(mode, "wal", "pool must use WAL journal mode");
768
768
}
769
+
770
+
#[tokio::test]
771
+
async fn v003_relay_signing_keys_columns_are_correct() {
772
+
let pool = in_memory_pool().await;
773
+
run_migrations(&pool).await.unwrap();
774
+
775
+
// PRAGMA table_info returns: (cid, name, type, notnull, dflt_value, pk)
776
+
let columns: Vec<(i32, String, String, i32, Option<String>, i32)> =
777
+
sqlx::query_as("PRAGMA table_info(relay_signing_keys)")
778
+
.fetch_all(&pool)
779
+
.await
780
+
.expect("PRAGMA table_info must succeed");
781
+
782
+
let names: Vec<&str> = columns.iter().map(|r| r.1.as_str()).collect();
783
+
assert_eq!(
784
+
names,
785
+
vec![
786
+
"id",
787
+
"algorithm",
788
+
"public_key",
789
+
"private_key_encrypted",
790
+
"created_at"
791
+
],
792
+
"relay_signing_keys must have exactly these columns in order"
793
+
);
794
+
}
795
+
796
+
#[tokio::test]
797
+
async fn v003_relay_signing_keys_id_is_unique() {
798
+
let pool = in_memory_pool().await;
799
+
run_migrations(&pool).await.unwrap();
800
+
801
+
let insert = "INSERT INTO relay_signing_keys \
802
+
(id, algorithm, public_key, private_key_encrypted, created_at) \
803
+
VALUES (?, ?, ?, ?, datetime('now'))";
804
+
805
+
// First insert: must succeed.
806
+
sqlx::query(insert)
807
+
.bind("did:key:ztest123")
808
+
.bind("p256")
809
+
.bind("zpubkey1")
810
+
.bind("base64encodedvalue1")
811
+
.execute(&pool)
812
+
.await
813
+
.expect("first insert must succeed");
814
+
815
+
// Second insert with same id: must fail.
816
+
let result = sqlx::query(insert)
817
+
.bind("did:key:ztest123")
818
+
.bind("p256")
819
+
.bind("zpubkey2")
820
+
.bind("base64encodedvalue2")
821
+
.execute(&pool)
822
+
.await;
823
+
824
+
assert!(
825
+
result.is_err(),
826
+
"duplicate id must be rejected by PRIMARY KEY constraint"
827
+
);
828
+
}
769
829
}
+22
-24
crates/relay/src/routes/create_signing_key.rs
+22
-24
crates/relay/src/routes/create_signing_key.rs
···
13
13
use crate::app::AppState;
14
14
15
15
#[derive(Deserialize)]
16
+
#[serde(rename_all = "lowercase")]
17
+
enum Algorithm {
18
+
P256,
19
+
}
20
+
21
+
#[derive(Deserialize)]
16
22
#[serde(rename_all = "camelCase")]
17
23
pub struct CreateSigningKeyRequest {
18
-
#[serde(default)]
19
-
algorithm: Option<String>,
24
+
#[allow(dead_code)]
25
+
algorithm: Algorithm,
20
26
}
21
27
22
28
// Response uses camelCase per JSON API convention (keyId, publicKey).
···
33
39
pub async fn create_signing_key(
34
40
State(state): State<AppState>,
35
41
headers: HeaderMap,
36
-
Json(payload): Json<CreateSigningKeyRequest>,
42
+
Json(_payload): Json<CreateSigningKeyRequest>,
37
43
) -> Result<Json<CreateSigningKeyResponse>, ApiError> {
38
44
// --- Auth: require matching Bearer token ---
39
45
// Check this first so unauthenticated callers cannot probe server configuration.
···
67
73
));
68
74
}
69
75
70
-
// --- Algorithm: only p256 is supported ---
71
-
match payload.algorithm.as_deref() {
72
-
Some("p256") => {}
73
-
_ => {
74
-
return Err(ApiError::new(
75
-
ErrorCode::InvalidClaim,
76
-
"unsupported algorithm; expected \"p256\"",
77
-
))
78
-
}
79
-
}
80
-
81
76
// --- Master key: return 503 if not configured ---
82
77
let master_key: &[u8; 32] = state
83
78
.config
84
79
.signing_key_master_key
85
80
.as_ref()
81
+
.map(|s| &*s.0)
86
82
.ok_or_else(|| {
87
83
ApiError::new(
88
84
ErrorCode::ServiceUnavailable,
···
111
107
(id, algorithm, public_key, private_key_encrypted, created_at) \
112
108
VALUES (?, ?, ?, ?, datetime('now'))",
113
109
)
114
-
.bind(&keypair.key_id)
110
+
.bind(keypair.key_id.to_string())
115
111
.bind("p256")
116
112
.bind(&keypair.public_key)
117
113
.bind(&private_key_encrypted)
···
123
119
})?;
124
120
125
121
Ok(Json(CreateSigningKeyResponse {
126
-
key_id: keypair.key_id,
122
+
key_id: keypair.key_id.to_string(),
127
123
public_key: keypair.public_key,
128
124
algorithm: "p256".to_string(),
129
125
}))
···
138
134
http::{Request, StatusCode},
139
135
};
140
136
use tower::ServiceExt;
137
+
use zeroize::Zeroizing;
141
138
142
139
use crate::app::{app, test_state, AppState};
140
+
use common::Sensitive;
143
141
144
142
/// Build an AppState with both admin_token and signing_key_master_key configured.
145
143
async fn test_state_with_keys() -> AppState {
146
144
let base = test_state().await;
147
145
let mut config = (*base.config).clone();
148
146
config.admin_token = Some("test-admin-token".to_string());
149
-
config.signing_key_master_key = Some([
147
+
config.signing_key_master_key = Some(Sensitive(Zeroizing::new([
150
148
0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e,
151
149
0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c,
152
150
0x1d, 0x1e, 0x1f, 0x20,
153
-
]);
151
+
])));
154
152
AppState {
155
153
config: Arc::new(config),
156
154
db: base.db,
···
355
353
356
354
#[tokio::test]
357
355
async fn unsupported_algorithm_returns_400() {
358
-
// MM-92.AC5.1
356
+
// MM-92.AC5.1: serde rejects unknown enum variant with 422 Unprocessable Entity
359
357
let response = app(test_state_with_keys().await)
360
358
.oneshot(post_keys(
361
359
r#"{"algorithm": "k256"}"#,
···
363
361
))
364
362
.await
365
363
.unwrap();
366
-
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
364
+
assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY);
367
365
}
368
366
369
367
#[tokio::test]
370
368
async fn empty_algorithm_returns_400() {
371
-
// MM-92.AC5.2
369
+
// MM-92.AC5.2: serde rejects empty string for enum variant with 422 Unprocessable Entity
372
370
let response = app(test_state_with_keys().await)
373
371
.oneshot(post_keys(r#"{"algorithm": ""}"#, Some("test-admin-token")))
374
372
.await
375
373
.unwrap();
376
-
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
374
+
assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY);
377
375
}
378
376
379
377
#[tokio::test]
380
378
async fn missing_algorithm_field_returns_400() {
381
-
// MM-92.AC5.3: empty JSON body — algorithm field absent, defaults to None → 400
379
+
// MM-92.AC5.3: missing required field returns 422 Unprocessable Entity
382
380
let response = app(test_state_with_keys().await)
383
381
.oneshot(post_keys(r#"{}"#, Some("test-admin-token")))
384
382
.await
385
383
.unwrap();
386
-
assert_eq!(response.status(), StatusCode::BAD_REQUEST);
384
+
assert_eq!(response.status(), StatusCode::UNPROCESSABLE_ENTITY);
387
385
}
388
386
389
387
// --- Master key test ---