Skip to content

fix: reject empty HMAC keys in audit sign/verify#554

Merged
maziyarpanahi merged 2 commits into
maziyarpanahi:masterfrom
muhamedfazalps:fix/audit-empty-hmac-key
Jun 22, 2026
Merged

fix: reject empty HMAC keys in audit sign/verify#554
maziyarpanahi merged 2 commits into
maziyarpanahi:masterfrom
muhamedfazalps:fix/audit-empty-hmac-key

Conversation

@muhamedfazalps

Copy link
Copy Markdown
Contributor

Problem

AuditReport.sign/verify accepts empty strings and b'' as valid HMAC keys, producing worthless signatures with no warning. This is a silent security weakness for a signed-audit feature.

Fix

Added validation in _key_bytes to raise ValueError when the key is empty. This ensures a non-empty key is always required for signing and verification.

Fixes #529


If this helps, consider buying me a coffee! https://buymeacoffee.com/muhamedfazalps

muhamedfazalps and others added 2 commits June 21, 2026 16:29
_audit_report.sign/verify previously accepted empty strings and
b'' as valid HMAC keys, producing worthless signatures with no
warning. Added validation in _key_bytes to raise ValueError
when the key is empty, ensuring a non-empty key is always required.

Fixes maziyarpanahi#529
@maziyarpanahi maziyarpanahi added good first issue Good for newcomers help wanted Extra attention is needed improvement Hardening / refactor of existing code P2 Medium roadmap-v2 OpenMed V2 roadmap backlog labels Jun 22, 2026

@maziyarpanahi maziyarpanahi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you @muhamedfazalps. I reviewed this against #529 / OM-339 and added one maintainer follow-up commit: test: cover missing audit HMAC keys.

What I changed:

  • made None, empty string, and empty bytes all raise the same clear ValueError for audit signing and verification;
  • validated the verification key before comparing signatures so missing/empty verification keys are rejected distinctly from a wrong key;
  • documented the non-empty HMAC key requirement in AuditReport.sign() and AuditReport.verify();
  • added regression coverage for signing with missing/empty keys, verifying with missing/empty keys, and unsigned-report verification with a valid key.

Verification on the current PR checkout:

  • PYTHONPATH=/private/tmp/openmed-pr-554 /Users/maziyar/Developer/openmed/.venv/bin/python -m pytest tests/unit/core/test_audit_report.py -q -> 10 passed
  • /Users/maziyar/Developer/openmed/.venv/bin/ruff check openmed/core/audit.py tests/unit/core/test_audit_report.py -> passed
  • /Users/maziyar/Developer/openmed/.venv/bin/ruff format --check openmed/core/audit.py tests/unit/core/test_audit_report.py -> passed

I also copied the labels from #529 onto the PR. The branch is mergeable with no conflicts; GitHub has not attached hosted checks to the new head commit yet, so I verified the touched behavior locally.

@maziyarpanahi maziyarpanahi merged commit 79466fc into maziyarpanahi:master Jun 22, 2026
12 checks passed
@muhamedfazalps

Copy link
Copy Markdown
Contributor Author

Thanks for merging! 🎉

If this fix helped, consider supporting: https://buymeacoffee.com/muhamedfazalps

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

Labels

good first issue Good for newcomers help wanted Extra attention is needed improvement Hardening / refactor of existing code P2 Medium roadmap-v2 OpenMed V2 roadmap backlog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fail clearly when signing or verifying an audit report without an HMAC key

2 participants