Skip to content

feat: emit msg.sender in cert revocation events (I-02) #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leanthebean wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: emit msg.sender in cert revocation events (I-02) #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions src/CertManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,16 @@ contract CertManager is ICertManager {
using LibAsn1Ptr for Asn1Ptr;
using LibBytes for bytes;

error NotOwner();
error NotRevoker();
error IncompleteCertChain();
error DeprecatedEntrypoint();
error InvalidOwner();
error InvalidRevoker();

event CertVerified(bytes32 indexed certHash);
event CertRevoked(bytes32 indexed certHash);
event CertUnrevoked(bytes32 indexed certHash);
event CertRevoked(bytes32 indexed certHash, address indexed account);
event CertUnrevoked(bytes32 indexed certHash, address indexed account);
event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
event RevokerUpdated(address indexed previousRevoker, address indexed newRevoker);

Expand Down Expand Up @@ -78,11 +85,11 @@ contract CertManager is ICertManager {
}

function _onlyOwner() internal view {
require(msg.sender == owner, "not owner");
if (msg.sender != owner) revert NotOwner();
}

function _onlyRevoker() internal view {
require(msg.sender == revoker, "not revoker");
if (msg.sender != revoker) revert NotRevoker();
}

constructor(IP384Verifier p384Verifier_) {
Expand All @@ -107,12 +114,12 @@ contract CertManager is ICertManager {
/// @notice DEPRECATED — always reverts. The fully on-chain (non-hinted) path is too expensive
/// post-Fusaka and has been removed. Use {verifyCACertWithHints}.
function verifyCACert(bytes memory, bytes32) external pure returns (bytes32) {
revert("use hinted cert verification");
revert DeprecatedEntrypoint();
}

/// @notice DEPRECATED — always reverts. Use {verifyClientCertWithHints}.
function verifyClientCert(bytes memory, bytes32) external pure returns (VerifiedCert memory) {
revert("use hinted cert verification");
revert DeprecatedEntrypoint();
}

/// @notice Verify a CA certificate against its (already-cached) parent and cache the result.
Expand Down Expand Up @@ -168,13 +175,13 @@ contract CertManager is ICertManager {
}

function transferOwnership(address newOwner) external onlyOwner {
require(newOwner != address(0), "invalid owner");
if (newOwner == address(0)) revert InvalidOwner();
emit OwnershipTransferred(owner, newOwner);
owner = newOwner;
}

function setRevoker(address newRevoker) external onlyOwner {
require(newRevoker != address(0), "invalid revoker");
if (newRevoker == address(0)) revert InvalidRevoker();
emit RevokerUpdated(revoker, newRevoker);
revoker = newRevoker;
}
Expand All @@ -195,12 +202,12 @@ contract CertManager is ICertManager {

function unrevokeCert(bytes32 certId) external onlyOwner {
revoked[certId] = false;
emit CertUnrevoked(certId);
emit CertUnrevoked(certId, msg.sender);
}

function _revokeCert(bytes32 certId) internal {
revoked[certId] = true;
emit CertRevoked(certId);
emit CertRevoked(certId, msg.sender);
}

function _requireCanRevoke(bytes32 certId) internal view {
Expand Down Expand Up @@ -235,7 +242,7 @@ contract CertManager is ICertManager {
// Fail closed: a chain that terminates at bytes32(0) without reaching the pinned root is
// broken and must not be treated as a verified, non-revoked chain. Reverting here instead
// of returning silently means revocation safety never depends on upstream guards.
revert("incomplete cert chain");
revert IncompleteCertChain();
}

function _verifyCert(
Expand Down
4 changes: 2 additions & 2 deletions src/ICertManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ interface ICertManager {

// --- DEPRECATED: these always revert; use the *WithHints variants above. ---

/// @dev DEPRECATED — always reverts ("use hinted cert verification").
/// @dev DEPRECATED — always reverts.
function verifyCACert(bytes memory cert, bytes32 parentCertHash) external returns (bytes32);

/// @dev DEPRECATED — always reverts ("use hinted cert verification").
/// @dev DEPRECATED — always reverts.
function verifyClientCert(bytes memory cert, bytes32 parentCertHash) external returns (VerifiedCert memory);
}
53 changes: 51 additions & 2 deletions test/CertManager.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,12 +386,61 @@ contract RequireCachedChainNotRevokedTest is Test {
// verifiedParent[PARENT] is unset (bytes32(0)), so the chain is broken: it can never
// reach ROOT_CA_CERT_HASH. The fixed function must fail closed instead of returning.
cm.setParent(CHILD, PARENT);
vm.expectRevert("incomplete cert chain");
vm.expectRevert(CertManager.IncompleteCertChain.selector);
cm.requireCachedChainNotRevoked(CHILD);
}

function test_RevertsOnZeroCertHash() public {
vm.expectRevert("incomplete cert chain");
vm.expectRevert(CertManager.IncompleteCertChain.selector);
cm.requireCachedChainNotRevoked(bytes32(0));
}
}

/// @dev Regression coverage for BLOCKSEC-5249 finding I-02: `CertRevoked` / `CertUnrevoked` now
/// include the acting `msg.sender` as an indexed topic so revocation activity is monitorable.
contract CertRevocationEventTest is Test {
CertManager internal cm;

event CertRevoked(bytes32 indexed certHash, address indexed account);
event CertUnrevoked(bytes32 indexed certHash, address indexed account);

bytes32 internal constant CERT_ID = bytes32(uint256(0xabc));

function setUp() public {
// Deployer is both owner and revoker, so this contract can revoke/unrevoke directly.
cm = new CertManager(new P384Verifier());
}

function test_RevokeCertEmitsSender() public {
vm.expectEmit(true, true, false, true, address(cm));
emit CertRevoked(CERT_ID, address(this));
cm.revokeCert(CERT_ID);
}

function test_RevokeCertsEmitsSender() public {
bytes32[] memory ids = new bytes32[](1);
ids[0] = CERT_ID;
vm.expectEmit(true, true, false, true, address(cm));
emit CertRevoked(CERT_ID, address(this));
cm.revokeCerts(ids);
}

function test_UnrevokeCertEmitsSender() public {
cm.revokeCert(CERT_ID);
vm.expectEmit(true, true, false, true, address(cm));
emit CertUnrevoked(CERT_ID, address(this));
cm.unrevokeCert(CERT_ID);
}

/// @dev The recorded account is the actual caller, not the contract: a delegated revoker
/// address shows up in the event topic.
function test_RevokeCertRecordsActualCaller() public {
address delegatedRevoker = address(0xBEEF);
cm.setRevoker(delegatedRevoker);

vm.expectEmit(true, true, false, true, address(cm));
emit CertRevoked(CERT_ID, delegatedRevoker);
vm.prank(delegatedRevoker);
cm.revokeCert(CERT_ID);
}
}
22 changes: 11 additions & 11 deletions test/hinted/HintedNitroAttestation.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,43 +390,43 @@ contract HintedNitroAttestationTest is Test {

address newRevoker = address(0xBEEF);
vm.prank(address(0xCAFE));
vm.expectRevert("not owner");
vm.expectRevert(CertManager.NotOwner.selector);
certManager.setRevoker(newRevoker);

vm.expectRevert("invalid revoker");
vm.expectRevert(CertManager.InvalidRevoker.selector);
certManager.setRevoker(address(0));

certManager.setRevoker(newRevoker);
assertEq(certManager.revoker(), newRevoker);

bytes32 otherCertHash = keccak256("other cert");
vm.prank(address(0xCAFE));
vm.expectRevert("not revoker");
vm.expectRevert(CertManager.NotRevoker.selector);
certManager.revokeCert(otherCertHash);

vm.prank(newRevoker);
certManager.revokeCert(otherCertHash);
assertTrue(certManager.revoked(otherCertHash));

vm.prank(newRevoker);
vm.expectRevert("not owner");
vm.expectRevert(CertManager.NotOwner.selector);
certManager.unrevokeCert(otherCertHash);

certManager.unrevokeCert(otherCertHash);
assertFalse(certManager.revoked(otherCertHash));

vm.expectRevert("invalid owner");
vm.expectRevert(CertManager.InvalidOwner.selector);
certManager.transferOwnership(address(0));

address newOwner = address(0xA11CE);
vm.prank(address(0xCAFE));
vm.expectRevert("not owner");
vm.expectRevert(CertManager.NotOwner.selector);
certManager.transferOwnership(newOwner);

certManager.transferOwnership(newOwner);
assertEq(certManager.owner(), newOwner);

vm.expectRevert("not owner");
vm.expectRevert(CertManager.NotOwner.selector);
certManager.setRevoker(address(0x1234));

vm.prank(newOwner);
Expand Down Expand Up @@ -455,7 +455,7 @@ contract HintedNitroAttestationTest is Test {
bytes32 rootHash = certManager.ROOT_CA_CERT_HASH();

vm.prank(newRevoker);
vm.expectRevert("not owner");
vm.expectRevert(CertManager.NotOwner.selector);
certManager.revokeCert(rootHash);
assertFalse(certManager.revoked(rootHash));

Expand All @@ -464,7 +464,7 @@ contract HintedNitroAttestationTest is Test {
certHashes[1] = rootHash;

vm.prank(newRevoker);
vm.expectRevert("not owner");
vm.expectRevert(CertManager.NotOwner.selector);
certManager.revokeCerts(certHashes);
assertFalse(certManager.revoked(certHashes[0]));
assertFalse(certManager.revoked(rootHash));
Expand Down Expand Up @@ -725,10 +725,10 @@ contract HintedNitroAttestationTest is Test {
}

function test_DeployableCertManagerDisablesUnhintedEntrypoints() public {
vm.expectRevert("use hinted cert verification");
vm.expectRevert(CertManager.DeprecatedEntrypoint.selector);
certManager.verifyCACert("", bytes32(0));

vm.expectRevert("use hinted cert verification");
vm.expectRevert(CertManager.DeprecatedEntrypoint.selector);
certManager.verifyClientCert("", bytes32(0));

vm.expectRevert("use hinted attestation verification");
Expand Down
Loading