From 6f3ea135539b33eb19bd42b53432f190969e741c Mon Sep 17 00:00:00 2001 From: Leopold Joy Date: Fri, 24 Jul 2026 21:28:59 +0100 Subject: [PATCH] fix: configure initial certificate roles Co-authored-by: OpenCode --- CHANGELOG.md | 4 +-- README.md | 9 +++--- docs/hinted-p384-nitro-attestation.md | 2 +- src/CertManager.sol | 12 ++++---- test/CertManager.t.sol | 35 +++++++++++++++--------- test/RootCertCheck.t.sol | 2 +- test/helpers/CertManagerDemo.sol | 4 ++- test/hinted/HintedNitroAttestation.t.sol | 8 +++--- 8 files changed, 45 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dc2d37..f2f6916 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,8 +50,8 @@ yet a general-availability release. ### Changed (breaking) - Verification is now hinted. Use `CertManager.verifyCACertWithHints` / `verifyClientCertWithHints` and `NitroValidator.validateAttestationWithHints`. -- Constructors now take an `IP384Verifier`: deploy `P384Verifier` → `CertManager(p384Verifier)` → - `NitroValidator(certManager, p384Verifier)`. +- Constructors now take an `IP384Verifier` and initial CertManager roles: deploy `P384Verifier` → + `CertManager(p384Verifier, initialOwner, initialRevoker)` → `NitroValidator(certManager, p384Verifier)`. - `validateAttestationWithHints` requires the certificate bundle to be verified/cached first; an uncached bundle reverts with `"inverse hint underflow"`. diff --git a/README.md b/README.md index 3605b7c..9e26d14 100644 --- a/README.md +++ b/README.md @@ -35,11 +35,12 @@ For the full design, security argument, and measured gas, see Deploy in this order (the verifier references are immutable): 1. `P384Verifier` -2. `CertManager(p384Verifier)` — pins the AWS Nitro root CA and sets the deployer as owner/revoker. +2. `CertManager(p384Verifier, initialOwner, initialRevoker)` — pins the AWS Nitro root CA and + configures the production owner and revoker atomically. 3. `NitroValidator(certManager, p384Verifier)` -After deployment, move ownership to the production admin and set the operational revoker with -`transferOwnership` / `setRevoker`. +Use hardened multisig or other production-controlled addresses for `initialOwner` and +`initialRevoker`; do not use a temporary deployer key for either role. ### Verification flow @@ -92,7 +93,7 @@ off-chain). Revoked certificates are rejected on both cold verification and cach independently of `notAfter`. Cached descendants are also rejected when their cached parent chain contains a revoked certificate. -- The deployer starts as both `owner` and `revoker`. +- `owner` and `revoker` are configured atomically by the constructor. - The owner can call `transferOwnership`, `setRevoker`, `unrevokeCert`, and revoke `ROOT_CA_CERT_HASH` as an emergency global halt (the root is identified by its pinned hash, since it is never parsed on-chain). diff --git a/docs/hinted-p384-nitro-attestation.md b/docs/hinted-p384-nitro-attestation.md index 08e7a04..fdbfeed 100644 --- a/docs/hinted-p384-nitro-attestation.md +++ b/docs/hinted-p384-nitro-attestation.md @@ -406,7 +406,7 @@ AWS's Nitro attestation documentation disables CRL checking in its sample valida flow. This implementation keeps CRL parsing off-chain and exposes an operational revocation hook on-chain: -- the `CertManager` deployer starts as both `owner` and `revoker`; +- the `CertManager` constructor configures the initial `owner` and `revoker` addresses atomically; - the owner can transfer ownership, rotate the revoker, undo accidental revocations with `unrevokeCert`, and revoke `ROOT_CA_CERT_HASH` as an emergency global halt; - the revoker can call `revokeCert` / `revokeCerts` for non-root AWS certificate diff --git a/src/CertManager.sol b/src/CertManager.sol index 29ecfd0..1de1f01 100644 --- a/src/CertManager.sol +++ b/src/CertManager.sol @@ -99,11 +99,13 @@ contract CertManager is ICertManager { if (msg.sender != revoker) revert NotRevoker(); } - constructor(IP384Verifier p384Verifier_) { + constructor(IP384Verifier p384Verifier_, address initialOwner, address initialRevoker) { require(address(p384Verifier_) != address(0), "missing P384 verifier"); + if (initialOwner == address(0)) revert InvalidOwner(); + if (initialRevoker == address(0)) revert InvalidRevoker(); p384Verifier = p384Verifier_; - owner = msg.sender; - revoker = msg.sender; + owner = initialOwner; + revoker = initialRevoker; _saveVerified( ROOT_CA_CERT_HASH, VerifiedCert({ @@ -114,8 +116,8 @@ contract CertManager is ICertManager { pubKey: ROOT_CA_CERT_PUB_KEY }) ); - emit OwnershipTransferred(address(0), msg.sender); - emit RevokerUpdated(address(0), msg.sender); + emit OwnershipTransferred(address(0), initialOwner); + emit RevokerUpdated(address(0), initialRevoker); } /// @notice DEPRECATED — always reverts. The fully on-chain (non-hinted) path is too expensive diff --git a/test/CertManager.t.sol b/test/CertManager.t.sol index 6db1293..51344d8 100644 --- a/test/CertManager.t.sol +++ b/test/CertManager.t.sol @@ -25,7 +25,7 @@ contract Asn1DecodeHarness { contract CertManagerHarness is CertManager { using Asn1Decode for bytes; - constructor() CertManager(new P384Verifier()) {} + constructor() CertManager(new P384Verifier(), msg.sender, msg.sender) {} function verifyBasicConstraints(bytes memory der, bool ca) external pure returns (int64) { return _verifyBasicConstraintsExtension(der, der.root(), ca); @@ -35,7 +35,7 @@ contract CertManagerHarness is CertManager { contract CertManagerPubKeyHarness is CertManager { using Asn1Decode for bytes; - constructor() CertManager(new P384Verifier()) {} + constructor() CertManager(new P384Verifier(), msg.sender, msg.sender) {} function parsePubKey(bytes memory subjectPublicKeyInfo) external pure returns (bytes memory) { return _parsePubKey(subjectPublicKeyInfo, subjectPublicKeyInfo.root()); @@ -45,7 +45,7 @@ contract CertManagerPubKeyHarness is CertManager { contract CertManagerExtensionsHarness is CertManager { using Asn1Decode for bytes; - constructor() CertManager(new P384Verifier()) {} + constructor() CertManager(new P384Verifier(), msg.sender, msg.sender) {} function verifyExtensions(bytes memory der, bool ca) external pure returns (int64) { return _verifyExtensions(der, der.root(), ca); @@ -86,6 +86,15 @@ contract CertManagerTest is Test { assertEq(int256(certManagerHarness.verifyBasicConstraints(hex"3000", false)), -1); } + function test_ConstructorSetsInitialRoles() public { + address initialOwner = address(0xA11CE); + address initialRevoker = address(0xB0B); + CertManager cm = new CertManager(new P384Verifier(), initialOwner, initialRevoker); + + assertEq(cm.owner(), initialOwner); + assertEq(cm.revoker(), initialRevoker); + } + function test_BasicConstraintsEmptySequenceRejectsCACert() public { vm.expectRevert(CertManager.InvalidBasicConstraints.selector); certManagerHarness.verifyBasicConstraints(hex"3000", true); @@ -158,7 +167,7 @@ contract CertManagerTest is Test { function test_ParseTbsRejectsTrailingSignedFields() public { vm.warp(1775145600); - CertManager cm = new CertManager(new P384Verifier()); + CertManager cm = new CertManager(new P384Verifier(), address(this), address(this)); bytes memory mutated = _appendTbsTrailingField(CB1); @@ -248,7 +257,7 @@ contract CertManagerTest is Test { // non-hinted verification path. function test_VerifyCACertWithHints_ShortS_Regression() public { vm.warp(1775145600); - CertManager cm = new CertManager(new P384Verifier()); + CertManager cm = new CertManager(new P384Verifier(), address(this), address(this)); P384HintCollector collector = new P384HintCollector(); // CB0 (AWS Nitro root) is pinned in the constructor. @@ -262,7 +271,7 @@ contract CertManagerTest is Test { function test_VerifyCACertWithHints_MalleableSignatureUsesSameTbsCacheKey() public { vm.warp(1775145600); - CertManager cm = new CertManager(new P384Verifier()); + CertManager cm = new CertManager(new P384Verifier(), address(this), address(this)); P384HintCollector collector = new P384HintCollector(); bytes32 rootHash = keccak256(CB0); @@ -285,7 +294,7 @@ contract CertManagerTest is Test { function test_VerifyCACertWithHints_RejectsMalleableRootAlias() public { vm.warp(1775145600); - CertManager cm = new CertManager(new P384Verifier()); + CertManager cm = new CertManager(new P384Verifier(), address(this), address(this)); P384HintCollector collector = new P384HintCollector(); bytes32 rootHash = keccak256(CB0); @@ -303,7 +312,7 @@ contract CertManagerTest is Test { function test_VerifyCACertWithHints_RejectsSignatureWrapperTagSubstitution() public { vm.warp(1775145600); - CertManager cm = new CertManager(new P384Verifier()); + CertManager cm = new CertManager(new P384Verifier(), address(this), address(this)); P384HintCollector collector = new P384HintCollector(); bytes32 rootHash = keccak256(CB0); @@ -320,7 +329,7 @@ contract CertManagerTest is Test { function test_VerifyCACertWithHints_RejectsTrailingSignatureFields() public { vm.warp(1775145600); - CertManager cm = new CertManager(new P384Verifier()); + CertManager cm = new CertManager(new P384Verifier(), address(this), address(this)); P384HintCollector collector = new P384HintCollector(); bytes32 rootHash = keccak256(CB0); @@ -335,7 +344,7 @@ contract CertManagerTest is Test { function test_VerifyCACertWithHints_RejectsOuterTagSubstitution() public { vm.warp(1775145600); - CertManager cm = new CertManager(new P384Verifier()); + CertManager cm = new CertManager(new P384Verifier(), address(this), address(this)); bytes32 rootHash = keccak256(CB0); bytes memory mutated = bytes.concat(CB1); @@ -347,7 +356,7 @@ contract CertManagerTest is Test { function test_VerifyCACertWithHints_RejectsTbsAlgorithmTagSubstitution() public { vm.warp(1775145600); - CertManager cm = new CertManager(new P384Verifier()); + CertManager cm = new CertManager(new P384Verifier(), address(this), address(this)); bytes32 rootHash = keccak256(CB0); bytes memory mutated = bytes.concat(CB1); @@ -629,7 +638,7 @@ contract CertManagerTest is Test { /// @dev Exposes the internal revocation-chain walk and lets tests seed the `verifiedParent` /// cache directly so the broken-chain (fail-closed) behaviour can be exercised in isolation. contract RevocationChainHarness is CertManager { - constructor() CertManager(new P384Verifier()) {} + constructor() CertManager(new P384Verifier(), msg.sender, msg.sender) {} function setParent(bytes32 child, bytes32 parent) external { verifiedParent[child] = parent; @@ -687,7 +696,7 @@ contract CertRevocationEventTest is Test { function setUp() public { // Deployer is both owner and revoker, so this contract can revoke/unrevoke directly. - cm = new CertManager(new P384Verifier()); + cm = new CertManager(new P384Verifier(), address(this), address(this)); } function test_RevokeCertEmitsSender() public { diff --git a/test/RootCertCheck.t.sol b/test/RootCertCheck.t.sol index 17a5e63..f2c3789 100644 --- a/test/RootCertCheck.t.sol +++ b/test/RootCertCheck.t.sol @@ -14,7 +14,7 @@ contract RootCertCheckTest is Test, CertManager { bytes public constant ROOT_CA_CERT = hex"3082021130820196a003020102021100f93175681b90afe11d46ccb4e4e7f856300a06082a8648ce3d0403033049310b3009060355040613025553310f300d060355040a0c06416d617a6f6e310c300a060355040b0c03415753311b301906035504030c126177732e6e6974726f2d656e636c61766573301e170d3139313032383133323830355a170d3439313032383134323830355a3049310b3009060355040613025553310f300d060355040a0c06416d617a6f6e310c300a060355040b0c03415753311b301906035504030c126177732e6e6974726f2d656e636c617665733076301006072a8648ce3d020106052b8104002203620004fc0254eba608c1f36870e29ada90be46383292736e894bfff672d989444b5051e534a4b1f6dbe3c0bc581a32b7b176070ede12d69a3fea211b66e752cf7dd1dd095f6f1370f4170843d9dc100121e4cf63012809664487c9796284304dc53ff4a3423040300f0603551d130101ff040530030101ff301d0603551d0e041604149025b50dd90547e796c396fa729dcf99a9df4b96300e0603551d0f0101ff040403020186300a06082a8648ce3d0403030369003066023100a37f2f91a1c9bd5ee7b8627c1698d255038e1f0343f95b63a9628c3d39809545a11ebcbf2e3b55d8aeee71b4c3d6adf3023100a2f39b1605b27028a5dd4ba069b5016e65b4fbde8fe0061d6a53197f9cdaf5d943bc61fc2beb03cb6fee8d2302f3dff6"; - constructor() CertManager(new P384Verifier()) {} + constructor() CertManager(new P384Verifier(), msg.sender, msg.sender) {} function setUp() public { vm.warp(1732580000); diff --git a/test/helpers/CertManagerDemo.sol b/test/helpers/CertManagerDemo.sol index 40ef0ca..b3b5d30 100644 --- a/test/helpers/CertManagerDemo.sol +++ b/test/helpers/CertManagerDemo.sol @@ -9,7 +9,9 @@ import {IP384Verifier} from "../../src/IP384Verifier.sol"; contract CertManagerDemo is CertManager { uint256 public immutable certificateExpiryGraceSeconds; - constructor(IP384Verifier p384Verifier_, uint256 certificateExpiryGraceSeconds_) CertManager(p384Verifier_) { + constructor(IP384Verifier p384Verifier_, uint256 certificateExpiryGraceSeconds_) + CertManager(p384Verifier_, msg.sender, msg.sender) + { certificateExpiryGraceSeconds = certificateExpiryGraceSeconds_; } diff --git a/test/hinted/HintedNitroAttestation.t.sol b/test/hinted/HintedNitroAttestation.t.sol index cb6f8e2..d70c659 100644 --- a/test/hinted/HintedNitroAttestation.t.sol +++ b/test/hinted/HintedNitroAttestation.t.sol @@ -120,7 +120,7 @@ contract HintedNitroAttestationTest is Test { function setUp() public { vm.warp(1767472867); // 2026-01-03T20:41:07Z, matching the attestation timestamp. p384Verifier = new P384Verifier(); - certManager = new CertManager(p384Verifier); + certManager = new CertManager(p384Verifier, address(this), address(this)); validator = new NitroValidator(certManager, p384Verifier); parser = new NitroValidatorParseHarness(certManager, p384Verifier); hintCollector = new P384HintCollector(); @@ -708,7 +708,7 @@ contract HintedNitroAttestationTest is Test { function test_HintedValidationRequiresWarmCache() public { bytes memory attestation = _repairMissingPublicKeyBytes(_decodeBase64(_realAttestationB64())); (bytes memory attestationTbs, bytes memory signature) = validator.decodeAttestationTbs(attestation); - CertManager freshCertManager = new CertManager(p384Verifier); + CertManager freshCertManager = new CertManager(p384Verifier, address(this), address(this)); NitroValidator freshValidator = new NitroValidator(freshCertManager, p384Verifier); vm.expectRevert("inverse hint underflow"); @@ -780,13 +780,13 @@ contract HintedNitroAttestationTest is Test { uint64 notAfter = certManager.loadVerified(certHash).notAfter; // Exactly at notAfter: cold verification on a fresh manager succeeds (cert still valid). - CertManager atBoundary = new CertManager(p384Verifier); + CertManager atBoundary = new CertManager(p384Verifier, address(this), address(this)); vm.warp(notAfter); atBoundary.verifyCACertWithHints(caCert, parentHash, hints); assertGt(atBoundary.loadVerified(certHash).pubKey.length, 0, "cert valid at notAfter"); // One second later: cold verification on a fresh manager is rejected as expired. - CertManager pastBoundary = new CertManager(p384Verifier); + CertManager pastBoundary = new CertManager(p384Verifier, address(this), address(this)); vm.warp(uint256(notAfter) + 1); vm.expectRevert("certificate not valid anymore"); pastBoundary.verifyCACertWithHints(caCert, parentHash, hints);