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

BSIP 40: Custom active authorities #1285

Closed
5 of 15 tasks
sschiessl-bcp opened this issue Aug 24, 2018 · 4 comments
Closed
5 of 15 tasks

BSIP 40: Custom active authorities #1285

sschiessl-bcp opened this issue Aug 24, 2018 · 4 comments
Assignees
Labels
1a Epic High level concept to be addressed. Description should contain a list referencing child User Stories 2a Discussion Needed Prompt for team to discuss at next stand up. 3b Feature Classification indicating the addition of novel functionality to the design 4b Normal Priority Priority indicating the moderate impact to system/user -OR- existing workaround is costly to perform 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 DEX Impact flag identifying the Decentralized EXchange, market engine, etc. 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) 9d Gigantic Effort estimation indicating TBD feature hardfork

Comments

@sschiessl-bcp
Copy link

sschiessl-bcp commented Aug 24, 2018

Strengthening user security is one of the main factors to elevate BitShares. In light of recent hacking and phishing attempts this becomes even more important. The need for a more sophisticated account security preceeded the idea for a finer-grained control of account permissions. We propose to add an additional authority to the account, called Custom Active (Permission). The permission contains a list of operationid-to-authority mappings that each grant access to the respective operation as if it were the active permission of the account. Additionally, the arguments of said operation can be restricted.

User Story
As a user I want a more sophisticated control and management capabilities for my permissions so that the impact of a compromised key is reduced and on-chain tasks can be delegated.

This covers issues reported here as well

The BSIP

The BSIP drafting is completed and it is available here
https://github.com/bitshares/bsips/blob/master/bsip-0040.md

CORE TEAM TASK LIST

  • BSIP
    • Draft: @sschiessl-bcp @xeroc @clockworkgr
      • Estimate: 40 hours
    • Review, refine, publish: @sschiessl-bcp @bitshares/core-dev
    • Submit to BSIP repository
    • Submit Milestone 1 for Community approval
    • Submit Milestone 2 for Community approval
    • Submit Milestone 3 for Community approval
    • Submit Milestone 4 for Community approval
  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@sschiessl-bcp sschiessl-bcp changed the title BSIP: Custom active authorities BSIP 40: Custom active authorities Aug 24, 2018
@abitmore abitmore added hardfork feature 3b Feature Classification indicating the addition of novel functionality to the design 4b Normal Priority Priority indicating the moderate impact to system/user -OR- existing workaround is costly to perform 9d Gigantic Effort estimation indicating TBD labels Aug 27, 2018
@abitmore abitmore self-assigned this Aug 27, 2018
@ryanRfox ryanRfox added 1a Epic High level concept to be addressed. Description should contain a list referencing child User Stories 2a Discussion Needed Prompt for team to discuss at next stand up. 6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. 6 UX Impact flag identifying the User Interface (UX) 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 Security Impact flag identifying system/user security 6 DEX Impact flag identifying the Decentralized EXchange, market engine, etc. labels Aug 28, 2018
@ryanRfox
Copy link
Contributor

This Issue will serve as the Task List to track BSIP40 future development tasks (Issue). Discussion will take place within relevant tasks.

Assigned @sschiessl-bcp as the BSIP author to track the hours (40) for the draft.

@nathanielhourt
Copy link
Contributor

During the core team conference call yesterday, I was asked to evaluate the BSIP-40 implementation and determine its completeness and viability for adding to the 201904 release. I promised to look into it and return a response within a week. In the spirit of short-circuit evaluation, I am returning my response now.

BSIP-40 is cool, without a doubt, and it would be a nice feature for server admins and power users; however, the motivation for BSIP-40 is opened as due to recent hacking and phishing attempts... and need for better user education on account security. These motivations are important, and solutions to these issues should be implemented.

It seems to me, however, that BSIP-40 is a non-sequitur to these motivations. BSIP-40 offers more fine-grained security options to sophisticated users who already possess a firm understanding of BitShares, including topics like key-based authentication and operations. It offers more security options, but in return it makes the topic of BitShares account security more complex, not less. This is contrary to the stated goal of better user education.

It may be argued that this is merely a UI problem -- that the UI can wrap the complexities of keys and operation types, to offer powerful security packages in a format palatable to end-users. In my opinion, however, this serves only to distance the user even further from the complexities, encouraging the user to understand less about their account's security and rely more on ever-more-sophisticated tools the experts made. This is, again, inimical to the stated goals.

Now, I do acknowledge that I was asked to evaluate the implementation of BSIP-40, not whether its motivation is well-stated. Upon an initial review of the implementation, however, I have determined that the coded implementation has substantial room for improvement, but due to its size, each of these improvements will involve massive amounts of rework throughout the implementation.

Furthermore, due to the security-critical nature of this feature, it is my opinion that it would be unwise to accept such a large patch from a single source, no matter how well-reviewed. The implementation of such a feature should be created by multiple developers, who are well-familiarized with BitShares core code, engaged throughout the process, to foster dynamic conversation on the best implementation strategies for maintainability, concision, developer understanding, and speed of the code. The code written for BSIP-40 is full of highly complex, boilerplate-heavy template metaprogramming code, and a single typo or copy-paste oversight could easily translate to compromised accounts and tokens, while such mistakes can be easily missed by reviewers.

For the above reasons, I have concluded that the BSIP-40 implementation is not ready for fast-track inclusion into the current 201904 release. It may be possible to prepare it for the 201910 release, but I would be most comfortable with an implementation which is the result of collaboration between core developers, rather than outside contractors.

@xeroc
Copy link
Member

xeroc commented Feb 21, 2019

Historically, the idea of having some external (non core devs) resource work on BSIP40 implementation was simply due to lack of development resources and some desperate need for the functionality. Hence, an agreement was made back in the days to continue with abit's initial stubs and implement all the rest through Blockchain Projects B.V. with guidance from the core team.

Surely, we all agree that the final say comes from the core development team who has ultimate veto power over what code fits their QA needs and what needs to be improved.

With that said, I do understand and agree with not considering the code ready for fast-track inclusion into the upcoming hard-fork - I was never convinced we could make it - and that more review work is required due to the substantial security implications it has.

I would propose the following:

  1. @nathanhourt does an initial review and provides feedback about where improvements need to take place
  2. Core team and Blockchain Projects agree on the "who works on what".
  3. The entire core team does a detailed review on the code base (at least 3 people).

As you can see, I do want this feature to be included eventually as it significantly reduces implications of hacks, in particular in light of (BBF's) escrow systems that can greatly benefit from BSIP40.

@pmconrad
Copy link
Contributor

Done with #1860

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1a Epic High level concept to be addressed. Description should contain a list referencing child User Stories 2a Discussion Needed Prompt for team to discuss at next stand up. 3b Feature Classification indicating the addition of novel functionality to the design 4b Normal Priority Priority indicating the moderate impact to system/user -OR- existing workaround is costly to perform 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 DEX Impact flag identifying the Decentralized EXchange, market engine, etc. 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) 9d Gigantic Effort estimation indicating TBD feature hardfork
Projects
Development

No branches or pull requests

7 participants