Skip to content
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.

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

extension: expose generation of management contract uuid to allow external signing of approval request #1613

Conversation

rodion-lim-partior
Copy link
Contributor

@rodion-lim-partior rodion-lim-partior commented Feb 22, 2023

This PR introduces the following changes to the extension API:

  1. Expose the generation of management contract uuid as a json-rpc call

This allows the subsequent transaction for approval of contract state extension to be submitted as a rawPrivateTransaction and signed externally to the doVote function of the management contract.

@CLAassistant
Copy link

CLAassistant commented Feb 22, 2023

CLA assistant check
All committers have signed the CLA.

@antonydenyer
Copy link
Contributor

Thanks for the PR. I'm a little reticent to merge this, you can call the contract directly using standard tooling. I think there are some endpoints that have been added that are in the same situation, but given they are already in the product we're not going to be removing them.

Could you provide further details as to why this is needed and something that can not be done without this change?

@rodion-lim-partior
Copy link
Contributor Author

rodion-lim-partior commented Mar 10, 2023

Hi @antonydenyer , one thing I noticed was having to get the uuid to be an encoded hash of the management contract stored within the "standard" database of Tessera before contract state extension can be picked up for the state dump upon approval. Correct me if I'm wrong, but getting the uuid stored requires hitting the Q2T send endpoint. Without exposing this in the RPC calls, our external clients need to hit the Q2T endpoint.

I agree with you that contract state extension approval with an external signer can be done without this change, but I'm not too sure if having an external client call the Q2T endpoint is a good design choice, and whether there are any other ways around this. Thanks.

Copy link
Contributor

@antonydenyer antonydenyer left a comment

Choose a reason for hiding this comment

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

Fair enough - it's one of things that you could do without changes to quorum. But perhaps you're right, the correct place to do it is here.

Can you add an acceptance test in https://github.com/ConsenSys/quorum-acceptance-tests? Then we can get this merged.

@rodion-lim-partior
Copy link
Contributor Author

Got it, thanks for the clarification. I'll add the acceptance test in and update again.

@Krish1979
Copy link
Collaborator

Krish1979 commented Mar 18, 2023

contract extension was always built like as an admin only feature i.e., to work with a node managed account and that the sender and receiver nodes are managing the extension and not by some externally owned account operating on the receiver node. The PR relaxes the rule a bit and there are certain processes for which an account needs to be maintained on the node. Could you please add more details as why can't to manage an account on the node.

@Krish1979 Krish1979 closed this Mar 18, 2023
@Krish1979 Krish1979 reopened this Mar 18, 2023
@rodion-lim-partior
Copy link
Contributor Author

Hi @Krish1979, reason for raising this PR is to enable parties to use the contract state extension approval feature without storing any local account within the node (using pre-signed raw txn instead), which could increase the attack surface if the RPC endpoint gets compromised.

@Krish1979
Copy link
Collaborator

Krish1979 commented Mar 20, 2023

as long as you have local account's private key stored in a vault (integrated) and using Quorum or Clef to sign the account, you are not elevating the risk in anyway.

@krishnan-narayanan-partior

Hi @Krish1979, use of geth for account signing is ruled out as this will be deprecated in future releases. Clef only supports hashicorp vault (through plugin) and there is no support for HSMs based signing. This PR will allow wider options for account signing during the contract extension approval process.

@Krish1979
Copy link
Collaborator

There is no plan to deprecate account signing in Quorum. We may still retain that, also you could use https://github.com/ConsenSys/quorum-key-manager for wider option.

@krishnan-narayanan-partior

@Krish1979 Thanks for sharing the link, the quorum key manager mainly supports KV store and doesn't support HSMs which are FIPS-140 Level 2-3 standard. Can the account signing be relaxed instead of mandating use of some plugins like clef and QKM with limited capabilities?

You mentioned the below, can you please let us know which processes requires an account to be maintained on the node?

The PR relaxes the rule a bit and there are certain processes for which an account needs to be maintained on the node

@rodion-lim-partior
Copy link
Contributor Author

rodion-lim-partior commented Mar 24, 2023

@antonydenyer, I raised a PR to add an acceptance test for the new feature introduced in the quorum-acceptance-tests repository. Thanks.

@antonydenyer
Copy link
Contributor

I think the main outstanding question is how much risk does exposing the approval uuid add?

@rodion-lim-partior
Copy link
Contributor Author

Hi @antonydenyer, even without exposing the approval uuid, recipients are able to approve with an external signer (using a non admin private/public key pair) if they call the q2t endpoint directly, before hitting the management contract directly. In terms of outstanding risk, it would be pretty similar for both cases. Also, this reduces the need for participants to maintain unlocked accounts within the rpc node just for the purposes of approving contract state extensions.

@Krish1979
Copy link
Collaborator

@rodion-lim-partior - The contract extension method was always designed to be node level operation ("using node managed account") and not account level operation. The contract extension initiation still has to be triggered by a node managed account. Could you please confirm if you intend to allow initiating contract extension from external owned accounts as well ?

@rodion-lim-partior
Copy link
Contributor Author

@Krish1979, there's no intention to allow initiating contract state extension from externally owned accounts, only the approval of contract state extension. Initiation of contract state extension still requires node managed accounts even after this PR.

@antonydenyer antonydenyer enabled auto-merge (squash) March 29, 2023 11:42
@antonydenyer antonydenyer merged commit 3594225 into Consensys:master Mar 29, 2023
14 of 15 checks passed
@rodion-lim-partior rodion-lim-partior deleted the feature/support_externally_signed_contract_extension_approval branch August 14, 2023 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants