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

Threshold authority weights per operation. #1061

Closed
6 tasks
clockworkgr opened this issue Jun 15, 2018 · 9 comments
Closed
6 tasks

Threshold authority weights per operation. #1061

clockworkgr opened this issue Jun 15, 2018 · 9 comments
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2a Discussion Needed Prompt for team to discuss at next stand up. 3b Feature Classification indicating the addition of novel functionality to the design 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX) hardfork

Comments

@clockworkgr
Copy link
Member

clockworkgr commented Jun 15, 2018

User Story
As a user I want the ability to set authority threshold weight on the operation level rather than the account-level so that I can have finer-grained control of the account and improve security.

Example use cases

  1. As a gateway operator I have a service that automates the issuing of assets to users as deposits come in. This requires keeping the active authority keys on the machine running this service. I want to be able to set a low threshold weight to the issue_asset operation so a single key can be used, but have all other ops require multisig to limit the impact of a potentially compromised key.

  2. As a trader, I want to set up a trading bot on a server and similar to case 1 have placing and cancelling orders on a low threshold so that a single auth can be used but limit transfers and other ops to multisig.

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@sschiessl-bcp
Copy link

sschiessl-bcp commented Jun 15, 2018

I like this idea. Optional overriding the treshhold for hand-picked operations, while keeping the base logic.

I have no clue of the impact code-wise, but it will bring great security for automation tools.

@clockworkgr
Copy link
Member Author

clockworkgr commented Jun 15, 2018

Just realised that https://github.com/bitshares/bsips/blob/master/bsip-0029.md battles a similar security issue.

In BSIP29's case, we split the operation to 2 different ones and make one require owner auth. With the functionality above we would have multiple authorities and different weight thresholds for the 2 ops.

@pmconrad
Copy link
Contributor

Removed the "DEX" impact.

@sschiessl-bcp
Copy link

sschiessl-bcp commented Jun 18, 2018

Taking this more into a abstract level birthed the following idea:

We currently have two authorities (active, owner) and a memo key. We add a fourth:

Custom active
It contains a list of custom authorities that link an operation to an authority. One of those pairs contains:

  • operation id
  • allowed authority (meaning the full fledged authority scheme with threshold and weighted keys / accounts)

Custom active authorities can be added and deleted with active authority of the account.
If a custom active authority is set for an operation, this authority is treated as if it was the actual active authority of the account (important: NOT owner).

Impact
The impact on the backend should be quite limited I believe. Active approvals are collected in
transaction.cpp. In this method the Custom active authority could be queried to see if there is one for the requested operation, and if also present in the signature, be treated as active approval.

Use cases
Other than Alex's original one of course.

Account A adds Key Z as custom active authority for trade operation. The user can then login from anywhere with cloud login using the private key from key Z to unlock his account and only be granted trading possibilities.

Further securing the account against phishing
Arising issue here is now that an attacker could create a shitcoin and trade for that. To disallow that an additional market whitelisting should be added to the extensions of an account via account_update similar to whitelisted_by and whitelisting. This would be market_whitelisting, where asset pairs will entered. If whitelisting is present, only trading on those pairs is allowed.

I can think of many other use cases but I think the above is already strong enough. With such a setup and only logging in the trading authority, the phishing attempt that compromised OpenLedger would not have allowed to harm the account in the same extent (other then trading on already allowed pairs).

@ryanRfox ryanRfox added 1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2a Discussion Needed Prompt for team to discuss at next stand up. 3b Feature Classification indicating the addition of novel functionality to the design 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX) labels Jun 19, 2018
@sschiessl-bcp
Copy link

sschiessl-bcp commented Jul 13, 2018

Is there anything I can do or provide to help come to a conclusive decision if this makes it into a BSIP or gets support by the core team? If all I need to do is have patience then that's fine too

This also solves #1044 and #693

@clockworkgr
Copy link
Member Author

After careful consideration, I'm leaning slightly towards your abstraction.

My one would be easier to UI for and for people to understand but might lead to edge cases where 2 ops desired weights/authorities are 'incompatible"

Yours takes care of that by explicitly setting auths for each op if needed...Downside being it's harder for people to set up.

Or we could allow both. My scheme as described above with yours on top with higher priority to explicitly cover potential edge cases.

Anyone else wanna chip in? @oxarbitrage @abitmore @pmconrad @jmjatlanta ?

@sschiessl-bcp
Copy link

This also references #590

@clockworkgr
Copy link
Member Author

This is being turned into a BSIP. Further discussion here: bitshares/bsips#86

@pmconrad
Copy link
Contributor

pmconrad commented Nov 8, 2018

Discussion has moved to BSIP-40. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2a Discussion Needed Prompt for team to discuss at next stand up. 3b Feature Classification indicating the addition of novel functionality to the design 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX) hardfork
Projects
None yet
Development

No branches or pull requests

5 participants