Use whole unsigned VC as VC proof signature payload #2404
Conversation
kziemianek
added
the
C0-breaking
There was a problem hiding this comment.
Nice and clean, not much to complain, thanks!
About the registry - this PR removes it completely. I'm thinking in the future, we might want to add "publish vc" feature where the user can link the selected VCs to their DID to make them public, so that anyone can see them by tracking down the DID. For that we'll need (again) a registry 🤔
| const lastRegisteredEnclave = (await apiAtVcIssuedBlock.query.teerex.enclaveRegistry(enclaveCount)) | ||
| .value as TeerexPrimitivesEnclave; |
There was a problem hiding this comment.
I think checking scheduledEnclave should be more accurate - enclave registration needs to comply with it too. Right now I can't think of any case where scheduledEnclave and enclaveRegistry don't match though
Btw even if we want to check enclaveRegistry we should enclaveCount from the parachainBlockHash height
There was a problem hiding this comment.
But Teerex's ScheduledEnclave is empty so there is no value to compare against :(
There was a problem hiding this comment.
Ah in dev it's not populated, but in production it shouldn't be empty
There was a problem hiding this comment.
Maybe we should add the scheduled enclave even in dev - like here: https://github.com/litentry/litentry-parachain/pull/2382/files#diff-f941434353cbdda40f528800d59a89b42e5e51396b03c092bf41ef8e294d8fd5
There was a problem hiding this comment.
Maybe we can always populate it with worker's mrenclave for first validateer in dev see: #2382 (comment)
There was a problem hiding this comment.
@Kailai-Wang, @felixfaisal what do you think?
There was a problem hiding this comment.
fyi: the vc-sdk uses scheduledEnclave's value
There was a problem hiding this comment.
@Kailai-Wang @kziemianek I think we can always populate it for first validateer.
|
| const enclaveCount = await context.api.query.teerex.enclaveCount(); | ||
| // prepare teerex enclave registry data for further checks | ||
| const parachainBlockHash = await context.api.query.system.blockHash(vcPayloadJson.parachainBlockNumber); | ||
| const apiAtVcIssuedBlock = await context.api.at(parachainBlockHash); |
There was a problem hiding this comment.
parachainBlockNumber is the block used to calculate the VC, and it won't match the VC's Issuance block. Is it likely that the registered enclave changes between those two instances? better said, should we account for such a case?
There was a problem hiding this comment.
I think the time of VC's issuance is not important here - it doesn't matter what happens next after VC was generated as long as the VC was generated by legit enclave, but I would love to hear other opinions too.
Yea I'm not talking about old VCs, but rather in the future - but OK we can always add it back whenever needed. |
|
I will delete |
|
Requested review again as there are new changes. |
|
I'll update weights and merge it. |
Co-authored-by: kziemianek <kziemianek@users.noreply.github.com>
Co-authored-by: kziemianek <kziemianek@users.noreply.github.com>
VCManagementrelated toVCRegistry.After this change newly issued VC content can be verified just by verifying proof's signature, there would be no need to reach out to onchain's
VCRegistry.