Skip to content

fix(channel): normalize low-order peer key errors - #259

Open
dinakarjs wants to merge 1 commit into
agentrust-io:mainfrom
dinakarjs:fix/sealed-exchange-error-20261017
Open

dinakarjs wants to merge 1 commit into
agentrust-io:mainfrom
dinakarjs:fix/sealed-exchange-error-20261017

Conversation

@dinakarjs

Copy link
Copy Markdown
Contributor

What

Normalize X25519 exchange failures in SealedChannel.seal() to SealedChannelError, preserving the backend exception as the cause. The catch covers the exchange operation only. Add three low-order-key regression cases and a changelog entry.

Why

A 32-byte low-order public key passes key parsing but raises a raw backend ValueError during exchange. Callers handling the channel's documented error family cannot handle this consistently with other invalid keys or open failures.

Security impact

The low-order keys already fail closed before ciphertext is created. No encryption format, key acceptance, attestation binding or key-custody guarantee changes. This is error normalization, not a confidentiality bypass fix. The channel path requires two maintainer approvals under CONTRIBUTING.md.

Test plan

  • Before the fix: 3 new cases fail with raw ValueError; 7 existing channel tests pass.
  • After the fix: full unit suite, Python 3.12: 793 passed, 2 skipped, including all 10 channel tests and existing round-trip/wrong-key/tamper/AAD controls.
  • Ruff src/tests, mypy runtime/verify (47 files), Bandit and diff whitespace checks pass.
  • Tested local tree 6cee90c4918aa69768d87ad46992c8285c54afe8 matches the published tree. No hardware experiment performed.

DCO sign-off

  • I certify that I wrote or have the right to submit this contribution, and I agree to the Developer Certificate of Origin. Commit signed off.

Signed-off-by: Srinivasa Consultants <78673153+dinakarjs@users.noreply.github.com>
@dinakarjs
dinakarjs requested review from a team, carloshvp and zohebk8s as code owners October 16, 2026 20:14
@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Contributor Check: MEDIUM

Check Result
Profile MEDIUM
Credential LOW
Overall MEDIUM

Automated check by AgenTrust Contributor Check.

@github-actions github-actions Bot added the needs-review:MEDIUM Contributor check flagged MEDIUM risk label Oct 16, 2026
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@noah-ing noah-ing left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed exact head fee80aa against 305fedc. No actionable defect found. The catch covers only X25519 exchange ValueError, preserves its cause and detail, and normalizes already-rejected keys into the documented SealedChannelError.

Fresh Linux/Python 3.12 environment using the hash-pinned lock: the final full unit/conformance run passed 846 tests, with two real-hardware capture tests skipped. Independently authored checks passed 29 cases, including rejected encodings, exception boundaries, and deterministic base/head ciphertext compatibility. Ten normalization cases fail against the base with the expected raw ValueError.

Ruff, format, mypy, Bandit, dependency consistency and dependency audit pass. Source stayed unchanged; DCO trailer and clean merge simulation confirmed. The PR description accurately limits the security impact to error normalization. This is software/error-contract validation, not hardware-attestation evidence; the required two independent current-head maintainer approvals remain separate.

@rajnisht7 rajnisht7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the changes looks good, low order X25519 keys that parse but fail at exchange() now raise SealedChannelError with the backend ValueError preserved as cause, same as the existing invalid-key path. catch is limited to the exchange call, no ciphertext is produced, and the three regression cases cover it cleanly

@imran-siddique

Copy link
Copy Markdown
Member

Approved fee80aa: the three low-order cases fail with main's sealed.py and pass here, and the change only wraps the X25519 exchange. channel/ is a security path, so this needs a second approval. @carloshvp, could you review it by Monday 19 October?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:MEDIUM Contributor check flagged MEDIUM risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants