Fix Noise X25519 key usage and tests

[meatsquirk]

Summary

• Use X25519 public keys for Noise static key handling and validate key type early
• Simplify handshake payload creation and reduce duplication
• Update noise test fixtures/expectations to use X25519 noise keys

Testing

• /opt/homebrew/bin/python3.11 -m pytest tests/core/security/noise
• /opt/homebrew/bin/python3.11 -m pytest tests/core/security

Issue

• Fixes Noise handshake uses Ed25519 identity key as static DH key instead of X25519 #1182 (Noise handshake uses Ed25519 identity key as static DH key instead of X25519 #1182)

[acul71]

Hello @meatsquirk thanks, for this PR.
You are on the right path, but
Some lint typecheck and doc (newsfragment) issues have to be resolved.
Would you consider also to fix tests/utils/factories.py — noise_conn_factory ?
The examples can be fixed in a following issue/PR: Would you like to work also on that?

Full review here:

AI PR Review: #1195 — Fix Noise X25519 key usage and tests

---

1. Summary of Changes

What the PR does: This PR enforces X25519 for the Noise static key in the libp2p Noise handshake, aligns types with the libp2p Noise spec, and updates tests to use and assert X25519 keys.

Issue addressed: Fixes #1182 (Noise handshake uses Ed25519 identity key as static DH key instead of X25519). The issue describes spec non-compliance when the Noise static key is not X25519; this PR makes the library enforce and type the static key as X25519 and fail fast when a non-X25519 key is passed.

Modules/files affected:

• libp2p/security/noise/patterns.py — Core Noise handshake: adds _validate_noise_static_key() (runtime X25519 check), uses X25519PublicKey instead of Ed25519PublicKey for static key handling, simplifies make_handshake_payload (single return path, clearer extensions logic), and updates _get_pubkey_from_noise_keypair to return X25519PublicKey.
• tests/core/security/noise/test_early_data_extensions.py — One fixture now uses noise_static_key_factory() and passes the private key object directly.
• tests/core/security/noise/test_noise_interop.py — New file: tests for X25519 key type, signed data format, rejection of non-X25519 keys, full handshake with X25519, and key derivation.
• tests/core/security/noise/test_performance.py — Uses noise_static_key_factory() and noise_privkey=noise_keypair (private key object) instead of Ed25519 keypair.

Breaking changes / deprecations: None. Callers that already pass X25519 (default host, interop) are unchanged. Callers that passed Ed25519 or other key types will now get a clear NoiseStateError at handshake payload creation instead of later DH/signature failures.

---

2. Branch Sync Status and Merge Conflicts

Branch Sync Status

• Status: ℹ️ Ahead of origin/main
• Details: Branch is 0 commits behind, 4 commits ahead of origin/main (branch_sync_status.txt: 0 4). Ready to merge from a sync perspective.

Merge Conflict Analysis

• Conflicts detected: ✅ No conflicts — The PR branch merges cleanly into origin/main (git merge --no-commit --no-ff origin/main completed with exit code 0; log shows "Already up to date" and "=== NO MERGE CONFLICTS DETECTED ===").

---

3. Strengths

• Spec alignment: Correctly enforces the libp2p Noise spec (X25519 for static key and signed data) and fails fast with a clear error when the wrong key type is passed.
• Type correctness: Replacing Ed25519PublicKey with X25519PublicKey for the Noise static key reflects the actual key type and improves type safety.
• Simpler logic: make_handshake_payload is easier to follow: one validation call, one payload construction path with a small final_extensions helper instead of nested if/else.
• Test coverage: New test_noise_interop.py covers key type checks, rejection of non-X25519 keys, signature format, full handshake, and data exchange, which will prevent regressions.
• Test fixes: Performance and early-data tests no longer rely on the wrong key type (Ed25519); they use the shared X25519 factory.

---

4. Issues Found

Critical

• File: newsfragments/ (missing)
• Line(s): N/A
• Issue: Missing newsfragment for issue Noise handshake uses Ed25519 identity key as static DH key instead of X25519 #1182. The PR body states "Fixes Noise handshake uses Ed25519 identity key as static DH key instead of X25519 #1182" but there is no file 1182.bugfix.rst (or equivalent) under newsfragments/. Project policy requires a newsfragment per fixed issue.
• Suggestion: Add newsfragments/1182.bugfix.rst with a short user-facing description (e.g. "Noise handshake now requires and validates X25519 static keys per libp2p Noise spec; invalid key types raise a clear error.") and ensure the file ends with a newline.

Major

• File: libp2p/security/noise/patterns.py

• Line(s): 164, 168 (approximate in PR diff)

• Issue: E501 — Line too long (> 88 characters). Lint reports:

  • Line with f"make_handshake_payload: X25519 pubkey: {noise_static_pubkey.to_bytes().hex()}" is 92 characters.
  • Comment line "Determine final extensions: prioritize extensions.early_data, fallback to self.early_data" is 99 characters.

• Suggestion: Break the debug string across lines (e.g. assign noise_static_pubkey.to_bytes().hex() to a variable and use it in the f-string on the next line). Shorten or wrap the comment to stay within 88 characters.

• File: tests/core/security/noise/test_noise_interop.py

• Line(s): 104 (docstring), 174–175 (attribute access)

• Issue 1: E501 — Docstring "Verify signed data format: 'noise-libp2p-static-key:' + X25519 Montgomery bytes." is 94 characters.

• Issue 2: Pyrefly type error — Object of class ISecureConn has no attribute remote_permanent_pubkey. The factory returns tuple[ISecureConn, ISecureConn]; ISecureConn (and AbstractSecureConn) do not declare a remote_permanent_pubkey attribute. The concrete SecureSession/BaseSession implements get_remote_public_key() and has the attribute, but the interface does not expose the attribute name.

• Suggestion: Shorten the docstring or wrap it. For the type checker, use the interface method: pubkey = conn.get_remote_public_key() and then assert isinstance(pubkey, X25519PublicKey) and assert pubkey.get_type() == KeyType.X25519. This satisfies the type checker and still asserts the key type.

Minor

• File: tests/core/security/noise/test_noise_interop.py
• Line(s): ~82–84 (test_make_handshake_payload_uses_x25519_pubkey)
• Issue: expected_signed_data and actual_signed_data are both computed as make_data_to_be_signed(noise_pubkey), so the assertion actual_signed_data == expected_signed_data is trivially true and does not verify that the payload was actually signed with that data.
• Suggestion: Either assert that verify_handshake_payload_sig(payload, noise_pubkey) is True (which the test already does) and remove the redundant equality, or derive expected_signed_data from the payload (e.g. from what was signed) and compare to make_data_to_be_signed(noise_pubkey) so the test validates the signed content.

---

5. Security Review

• Key type enforcement: The PR improves security by ensuring only X25519 is used for Noise DH and signed data, matching the spec and avoiding cross-curve misuse (e.g. Ed25519 bytes interpreted as X25519).
• Fail-fast: Rejecting non-X25519 keys at payload creation reduces the risk of subtle interop or cryptographic issues from wrong key types.
• No new risks identified. No change to key storage, logging of key material, or handling of external input beyond the existing handshake path.

Verdict: Positive security impact; no additional mitigations required for this change set.

---

6. Documentation and Examples

• Code-level docs: _validate_noise_static_key has a clear docstring (purpose and raised exception). Existing comments in make_handshake_payload and _get_pubkey_from_noise_keypair are updated to mention X25519.
• User-facing docs / examples: No changes to README, tutorials, or example code in this PR. The behavioral change (stricter key type) is appropriate to document in the newsfragment (see §4 Critical). No additional doc or example updates are strictly required for this PR.

---

7. Newsfragment Requirement

Status: ❌ BLOCKER — Missing newsfragment

• Severity: CRITICAL / BLOCKER
• Issue: PR fixes Noise handshake uses Ed25519 identity key as static DH key instead of X25519 #1182 but there is no newsfragment file for issue 1182 under newsfragments/. Grep for 1182 and 1195 in newsfragments/ shows no matching file.
• Impact: PR cannot be approved until a valid newsfragment is added (project requirement).
• Suggestion:
  1. Add newsfragments/1182.bugfix.rst.
  2. Content (ReST, user-facing): e.g. "Noise handshake now requires X25519 static keys per libp2p Noise spec. Passing a non-X25519 key raises a clear error."
  3. Ensure the file ends with a newline.
• Action required: Add 1182.bugfix.rst (or the correct type for this change) before merge.

---

8. Tests and Validation

New tests: test_noise_interop.py adds focused tests for:

• X25519 key type from factory and in handshake payload
• Rejection of non-X25519 noise_static_key via NoiseStateError
• Signed data format (prefix + X25519 bytes)
• Signature verification with X25519 public key
• Full handshake and data exchange with X25519 keys
• Key derivation and round-trip of X25519 keys

Updated tests: test_performance.py and test_early_data_extensions.py now use noise_static_key_factory() and the correct private-key argument, so they no longer depend on the wrong key type.

Test run: Full suite (make test) — 2137 passed, 16 skipped, 25 warnings. Exit code 0. No failures in Noise or security tests.

Linting (make lint)

• Result: Failed (exit code 1).
• Ruff (E501): 3 line-length violations:
  • libp2p/security/noise/patterns.py:164 — 92 > 88 chars (debug f-string).
  • libp2p/security/noise/patterns.py:168 — 99 > 88 chars (comment).
  • tests/core/security/noise/test_noise_interop.py:104 — 94 > 88 chars (docstring).
• Pyrefly: 1 error — tests/core/security/noise/test_noise_interop.py:174:35-63: Object of class ISecureConn has no attribute remote_permanent_pubkey.
• Other: ruff format made some changes (import order, spacing); those are fine. mypy passed.

Type checking (make typecheck)

• Result: Failed (exit code 1).
• Pyrefly: Same error as above — ISecureConn has no attribute remote_permanent_pubkey. Use get_remote_public_key() and then assert on the returned PublicKey type.

Documentation build (make linux-docs)

• Result: Succeeded (exit code 0). No errors or relevant warnings. Doctests passed.

---

9. Recommendations for Improvement

1. Add newsfragment 1182.bugfix.rst (or appropriate type) and satisfy the project’s release-notes requirement.
2. Fix lint (E501): Shorten or split the long lines in patterns.py and the docstring in test_noise_interop.py.
3. Fix typecheck: In test_full_handshake_with_x25519, use conn.get_remote_public_key() instead of conn.remote_permanent_pubkey, then assert isinstance(pubkey, X25519PublicKey) and pubkey.get_type() == KeyType.X25519.
4. Tighten test: In test_make_handshake_payload_uses_x25519_pubkey, either remove the redundant expected_signed_data == actual_signed_data assertion or base the expected value on the payload so the test actually validates signed content.

---

10. Questions for the Author

1. Should AbstractSecureConn / ISecureConn be extended to declare remote_permanent_pubkey (or a read-only property) for type checkers and documentation, or is using get_remote_public_key() in tests the preferred approach?
2. Do you want to add a brief note in the Noise module docstring or in the transport docstring that the Noise static key must be X25519 (with a link to the spec)?

---

11. Overall Assessment

• Quality rating: Good — Correct behavior, clearer types, and better tests; blocked by missing newsfragment and fixable lint/type issues.
• Security impact: Low (positive) — Enforces spec and fails fast on wrong key type.
• Merge readiness: Needs fixes — Add newsfragment; fix E501 and pyrefly (use get_remote_public_key() in test).
• Confidence: High — Review is based on full diff, issue Noise handshake uses Ed25519 identity key as static DH key instead of X25519 #1182, branch/merge checks, and captured lint/typecheck/test/docs output.

---

12. Scope and follow-up (issue #1182 comment)

The maintainer comment on issue #1182 notes that the main and interop paths already use X25519, and that the wrong pattern (identity key or Ed25519 as Noise static key) still appears in examples and some test helpers. It concludes that those should be fixed so documentation and tests match the spec. The following scope split is recommended:

In this PR (#1195):

• Library enforcement + Noise test fixes (current scope) is appropriate. This PR already fixes test_performance.py and test_early_data_extensions.py. No need to expand scope beyond that for the core fix.
• Optional but useful: Fix tests/utils/factories.py — noise_conn_factory, which currently uses noise_transport_factory(create_ed25519_key_pair()). Once the library rejects non-X25519 keys, any test using this factory will fail. Fixing it in this PR (e.g. use noise_static_key_factory() or equivalent) keeps the suite green and is a small, consistent change. If not done here, it should be done in an immediate follow-up.

Follow-up issue/PR:

• Examples should be fixed in a separate issue/PR:
  • examples/websocket/websocket_demo.py (Ed25519 for Noise key)
  • examples/doc-examples/example_encryption_noise.py, example_running.py, example_multiplexer.py, example_peer_discovery.py (secp256k1 identity as noise_privkey)
• Rationale: Different concern (docs/onboarding vs security fix), more files, and issue Noise handshake uses Ed25519 identity key as static DH key instead of X25519 #1182 is about the Noise handshake/library behaviour; the core fix is in this PR. A dedicated “Fix Noise examples to use X25519” issue/PR keeps the security PR focused and makes the docs change easy to find and review.

[acul71]

Hello @meatsquirk , are you working on this PR, do you need help ?