expose peer certificate fingerprint on AuthenticatedConnection#18
expose peer certificate fingerprint on AuthenticatedConnection#18rektide wants to merge 2 commits into
Conversation
Upstream mdns-sd v0.19 includes ServiceDaemon::new_with_port() natively, making the kaidokert/custom_port fork unnecessary. Remove the [patch] override and bump the dependency.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request exposes the peer's SPKI SHA-256 fingerprint on the server-side AuthenticatedConnection via a new peer_fingerprint() accessor, and adds an integration test to verify this behavior. The review feedback suggests removing an unnecessary tokio::time::sleep in the test to prevent potential flakiness, and reusing the existing fingerprint computation logic from the server implementation instead of duplicating it in the test file.
| // Give the server a moment to start listening. | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | ||
|
|
There was a problem hiding this comment.
Using tokio::time::sleep to wait for the server to start listening is a potential source of test flakiness and is unnecessary here. QuinnServer::bind binds the UDP socket synchronously, so the server is ready to receive connections immediately after bind returns. Any incoming connection packets sent by the client will be buffered by the OS and Quinn's background driver even if the server task hasn't polled accept() yet.
| fn compute_spki_fingerprint(cert_der: &[u8]) -> Result<[u8; 32], String> { | ||
| let (_, cert) = X509Certificate::from_der(cert_der) | ||
| .map_err(|e| format!("Failed to parse certificate: {e}"))?; | ||
| let spki_bytes = cert.public_key().raw; | ||
|
|
||
| let mut hasher = Sha256::new(); | ||
| hasher.update(spki_bytes); | ||
| let hash = hasher.finalize(); | ||
|
|
||
| let mut fingerprint = [0u8; 32]; | ||
| fingerprint.copy_from_slice(&hash); | ||
| Ok(fingerprint) | ||
| } |
There was a problem hiding this comment.
This helper function is a duplicate of QuinnServer::compute_spki_fingerprint in server.rs. To improve maintainability and avoid code duplication, consider making QuinnServer::compute_spki_fingerprint public in server.rs and reusing it here. This would also allow removing the x509-parser and sha2 dependencies from this test file.
`QuinnServer::authenticate_connection` extracts the peer certificate SPKI SHA-256 fingerprint and binds it into SPAKE2 via `CryptoData::set_peer_fingerprint` to satisfy RFC 9382. After SPAKE2 completes, the fingerprint was discarded: `AuthenticatedConnection` stored only the `quinn::Connection`, so callers of `accept()` could not observe which peer had just authenticated. The fingerprint is the only stable cryptographic identifier for an OSP agent. IP, port, and mDNS instance name can all churn while the fingerprint stays constant. W3C OpenScreen and openscreen-rs discovery (`service_info.rs` defines `PartialEq` on fingerprint alone) both treat it as peer identity. Receivers need it on the accept side to: - key per-peer state (rate-limit counters, presentation sessions) - cross-reference an authenticated link against mDNS discovery records - distinguish multiple simultaneous controllers This carries the verified fingerprint out of `authenticate_connection` on the `AuthenticatedConnection` struct, alongside a `peer_fingerprint()` accessor mirroring the existing `remote_address()` / `is_closed()` style. The dial side already had this: `QuinnClient::new` takes the expected fingerprint as a parameter, so callers retain it directly. This makes the two halves symmetric. The field is populated from the local that was already computed at the top of `authenticate_connection`, so there is no new cryptographic work and no behavior change beyond the exposed value. Adds an integration test (`peer_fingerprint_exposure.rs`) that drives both client and server to a fully authenticated connection and asserts `peer_fingerprint()` on the server side equals the SPKI SHA-256 of the client certificate, computed independently via x509-parser + sha2.
1 participant
QuinnServer::authenticate_connection extracts the peer certificate SPKI SHA-256 fingerprint and binds it into SPAKE2 via CryptoData::set_peer_fingerprint to satisfy RFC 9382. After SPAKE2 completes, the fingerprint was discarded: AuthenticatedConnection stored only the quinn::Connection, so callers of accept() could not observe which peer had just authenticated.
Why
The fingerprint is the only stable cryptographic identifier for an OSP agent. IP, port, and mDNS instance name can all churn while the fingerprint stays constant. W3C OpenScreen and openscreen-rs discovery (
service_info.rsdefinesPartialEqon fingerprint alone) both treat it as peer identity. Receivers need it on the accept side to:Change
Carries the verified fingerprint out of
authenticate_connectionon theAuthenticatedConnectionstruct, alongside apeer_fingerprint()accessor mirroring the existingremote_address()/is_closed()style. The dial side already had this:QuinnClient::newtakes the expected fingerprint as a parameter, so callers retain it directly. This makes the two halves symmetric.The field is populated from the local that was already computed at the top of
authenticate_connection, so there is no new cryptographic work and no behavior change beyond the exposed value.Test
Adds an integration test (
peer_fingerprint_exposure.rs) that drives both client and server to a fully authenticated connection and assertspeer_fingerprint()on the server side equals the SPKI SHA-256 of the client certificate, computed independently viax509-parser+sha2.