Custom active permission #86

sschiessl-bcp · 2018-07-25T12:34:22Z

No description provided.

abitmore · 2018-07-25T14:38:45Z

Things to think about:

verify_authority function / API, verify_account_authority API
get_potential_signatures / get_required_signatures API
how to evaluate proposals
complexity

sschiessl-bcp · 2018-07-25T14:50:29Z

Complexity I can't argue. For the rest I was hoping it can be implemented on a deep level where the active authority is granted if a custom active is present, without changing any other existing logic.

Idea, for example, might be better ways: when you are iterating the given signatures you are checking the required accounts if they have any custom active authority that matches the given signature and the operation and does pass the asserts. If so, active authority of the required account is granted.

I am not familiar with the backend too much, excuse if that was rubbish

abitmore · 2018-07-25T16:31:58Z

Ideally a custom permission item can be defined as

struct custom_permission_object
{
   account_id;
   custom_authority;
   operation_id;
   operation_permission_details;
};

Among it, custom_authority and operation_id are m-n mapped.

The operation_permission_details field is operation-specific and can be very complex, for example, for a transfer, people may want to set limitations on:

what asset can be used as fee
how much the fee can be
who to receive
what asset can be transferred
how much can be transferred
- one-time limit
- daily limit or similar
frequency of being executed
memo size

or some combinations of those limitations, E.G. can transfer at most 100 asset X to account A 3 times a day, at most 1000 asset Y to account B 5 times a day, and etc.

sschiessl-bcp · 2018-07-25T20:39:37Z

struct custom_permission_object
{
   account_id;
   custom_authority;
   operation_id;
   operation_permission_details;
};

a) This object includes the account_id, so you envision this not to be part of the account_object, correct?
Just to say it: This is exactly the initial idea just forumlated more towards an actual implementation, correct?

b) You are even upgrading the possible asserts, or limitations as you called it. Flexibility would be great, e.g. to define allowed ranges for integer arguments. My only wish would be that the logic is so generic that no custom logic has to be done for a specific operation, do you think thats possible? I would rather restrict the flexibility than to do operation specific custom code.

c) How would conditions that requre historic information (like a daily transfer limit) be implemented? Would you store historic information wihtin the custom_permission_object object as well?

abitmore · 2018-07-25T21:09:18Z

I was just writing what I've thought of. The potential use cases or directions to implement. Some of them might be too hard to implement or have some limitations so not worth the efforts. We need more discussion / brainstorming.

sschiessl-bcp · 2018-07-26T07:02:01Z

I was just writing what I've thought of. The potential use cases or directions to implement. Some of them might be too hard to implement or have some limitations so not worth the efforts. We need more discussion / brainstorming.

I see. My point of the BSIP was to introduce a security layer, but I guess it could also be extended to allow even more sophisticated use cases. Let's brainstorm and go through your points

what asset can be used as fee

who to receive

what asset can be transferred

This is a simple restriction of argument value

   assert: {
        arguments: {
            to: {
               value: [ 'list', 'of', 'allowed', 'values']
           }
       }
    }

frequency of being executed

one-time limit

This requires history as one needs to know: Last time of execution. One time limit would then be
having no or infinite interval_in_sec

   assert: {
        frequency: {
           last: timestamp of last execution,
           interval_in_sec: integer, if (last == null || now >= last + interval_in_sec) execution is allowed, and set last
        }
    }

how much the fee can be

how much can be transferred

memo size

This is a range limitation. For integers it's the value, for string it's the length.

   assert: {
        arguments: {
            to: {
               min: integer, minimum value, inclusive,
               max: integer, maximum value, inclusive
           }
        }
    }

daily limit or similar

This is a range limitation. For integers it's the value, for string it's the length. How does this overlap with withdrawal_permission?

   assert: {
        arguments: {
            to: {
               limit: {
                  last: timestamp when counter was reset,
                  cumsum: integer, sum of all amounts that happened since last timestamp,
                  max: maximum allowed,
                  interval_in_sec: timeinterval, if (last == null || now >= last + interval_in_sec) set cumsum=0 and set last
           }
        }
    }

So in total this would boil down to the following generic scheme of asserts:

   assert: {
        arguments: {
            argument_name: {
               value: [ 'list', 'of', 'allowed', 'values'],
               min: integer, minimum value, inclusive,
               max: integer, maximum value, inclusive,
               limit: {
                  last: timestamp when counter was reset,
                  cumsum: integer, sum of all amounts that happened since last timestamp,
                  cumsum_max: maximum allowed,
                  interval_in_sec: timeinterval, if (last == null || now >= last + interval_in_sec) set cumsum=0 and set last
           }
        },
        frequency: {
           last: timestamp of last execution,
           interval_in_sec: integer, if (last == null || now >= last + interval_in_sec) execution is allowed, and set last
        }
    }

This would allow the most fine-grained control possible, but I am unsure as if can be implemented like this in an efficient manner. It would have my vote as I am in big favor of abstract concepts.

xeroc · 2018-07-26T07:19:04Z

The operation_permission_details field is operation-specific and can be very complex, for example, for a transfer, people may want to set limitations on:

Our idea was to actual keep it rather simple (if possible). I was thinking along the lines of

having an regular operation structure as part of the custom operation structure
each attribute of the operation can have a value or NULL
if NULL, the attribute in an operation to be evaluated can have any value
if not NULL, the value of the attribute has to be exactly the same.

This way we can grant access to specific operations to specific accounts and limit the attributes of an operation.

That was the original idea that should (imho) be rather easy to implement and add quite some power to the permissions system. Not sure a) it make sense to make it more complicated (not saying we shouldn't) and b) there is no way to get both implemented in two steps.

I like flexibility, but not at the cost of having the feature postponed for long.

Limitations

For operations that have complex attributes (like new_options for account_update_operation), we cannot limit values with in those complex attributes (think votes within new_options)

Thus, in order to grant access to a key to only voting, we would need to create a new operation that takes the votes directly instead of a nested options attribute.

Also, of course, we couldn't limit amounts to anything other than an exact match.

pmconrad · 2018-07-26T15:24:25Z

bsip-0040.md

+ - Proposal Update Key: Approve proposals (2FA comes to mind)
+ - Faucet Key: Allow only to create accounts
+The above list of named keys is nothing that is known to the backend as the backend should have an abstract implementation.
+ The UI should provide a button "Create Trading Key" that properly configures the respective custom active permission entry.


UI changes are out of scope for a BSIP. Remove this line, or replace "should" with "could" to make it clear that this is not part of the specification.

pmconrad · 2018-07-26T15:25:03Z

bsip-0040.md

+
+# Abstract
+
+Strengthening user security is one of the main factors to elevate BitShares. Inlight of recent 


"In light" - two words

pmconrad · 2018-07-26T15:27:36Z

bsip-0040.md

+custom_active_permission = list of custom_active_authority items
+custom_active_authority = {
+    operationid,
+    auhtority,


pmconrad · 2018-07-26T15:28:27Z

bsip-0040.md

+# Rational
+
+The description here is more on a superficial level and no recommendation how it can best be implemented.
+Custom active permission is a list of custom active authorities. A `custom active authority` contains an `operation_id`, an `authority` (just like with active permission) and `assert`s than can be used to restricted arguments. When a transaction is signed with such an authority the backend checks if the contained operation has a corresponding custom active authority entry and if so acts as if the active authority of the corresponding account is given. It also checks if the arguments are in the allowed range.


pmconrad · 2018-07-26T15:35:25Z

bsip-0040.md

+* Extend `account_update` or create a new operation to allow changing the custom active permission
+* Operation-specific authorities (if present) must be evaluated in incoming transactions
+* Additional committee parameters may be needed to limit the extend of usage of this feature
+


Please elaborate:

can an account specify different authorities for the same operation but different asserts (i. e. require key 1 for transfer to Alice and key 2 for transfer to Bob)?

list possible assertions for each operation

specify assert logic, e. g. multiple fields/values, boolean operations, supported operators...

I have included a more detailed description, please see here

https://github.com/sschiessl-bcp/bsips/blob/patch-1/bsip-0040.md

sschiessl-bcp · 2018-07-27T12:44:10Z

I have updated the specifications of the asserts, please have a look
https://github.com/sschiessl-bcp/bsips/blob/patch-1/bsip-0040.md (file in this pull request)

@abitmore @pmconrad

clockworkgr · 2018-07-27T13:35:18Z

Are asserts based on historical data possible seeing as witness nodes don't really carry history?

sschiessl-bcp · 2018-08-09T09:22:43Z

All comments and reviews included up to now.

abitmore · 2018-08-09T19:48:23Z

bsip-0040.md

@@ -102,7 +102,8 @@ List of possible restrictions are:
 | ------------- |:-------------:| -----:|
 | `any`      | [`list`, `of`, `allowed`, `values`] | stateless |
 | `none`     | [`none`, `of`, `these`, `values`] | stateless |
-| `lt, le, gt, ge`      | `comparative` | stateless |
+| `lt, le, gt, ge, eq`      | `comparative` | stateless |


We already added eq, why not add ne aka "not equal"?

abitmore · 2018-08-09T19:55:18Z

bsip-0040.md

 - `delete_custom_active_authority`: Cheap similar to `limit_order_cancel`

+This logic forces the user to think about the desired duration and enforces to put it rather too short than too long. 
+If a custom active is expired, or considered to be too short, extending it is easily doable. 


How about a user changing period from 2018-08-01 - 2018-09-01 to 2018-08-15 - 2018-09-15 on August 20th? The duration doesn't increase, but it effectively extends.

Ah I see. It was meant like duration refers to the time that is still left, not the actual duration (i.e. date_to - max(now, date_from)).

In your example, the reference base duration when updating id 2018-08-01 - 2018-09-01. So updating to 2018-08-15 - 2018-09-15 means adding 14 days.

now is chain state which can't be used when calculating fee with current design principle. Unless we add a suppose_now field in the operation and evaluate it against chain state, but it would be ugly.

I see, good point. The fee would then depend on execution time and not creation time.

Let's simply do it that all time period increase counts, whereas decrease is ignored.

In your example:
The change 2018-08-01 to 2018-08-15 decreases the period, ignore.
THe change 2018-09-01 to 2018-09-15 increases the period, add to fee.

Unfortunately the value before change is in chain state so can't be used as well.

Then the fee of updating should take argument size and authority size into account, but not only duration difference. Ideally,

update_fee = basic_fee + max( 0, total_new_fee - unused_old_fee ) = basic_fee + max( 0, new_fee_rate * new_duration * new_auth_size * new_argument_size - old_fee_rate * max(old_time_to-now, 0) * old_auth_size * old_argument_size )

Since all old data is in chain state, to calculate the difference, we need to put some "supposed" data in the operation, to be simpler, we can directly put the supposed delta in the operation and calculate in background.

Note: I changed format of restriction a bit (mentioned in #86 (comment)),

You basically added a modifier field, which is used to extract the implicit type conversions into explicit ones. Which is totally fine and is covered by my note in the BSIP
"This list of restrictions may be redefined to improve clarity, performance or to reduce complexity of implementation while maintaining the intended functionality"

Since all old data is in chain state, to calculate the difference, we need to put some "supposed" data in the operation, to be simpler, we can directly put the supposed delta in the operation and calculate in background.

What is best here for implementation I leave to you (the devs). One question I have: If the user overpays the transaction fee, will he (or can he) get the rest back?

One question I have: If the user overpays the transaction fee, will he (or can he) get the rest back?

In BitShares fees can always be overpaid, and the system never refund overpaid fees except the "refund order creation fee when order is cancelled" feature.

BTW I mentioned fee pooling mechanism earlier but Peter didn't like it at this early stage.

I saw that, but that takes it to a new level in terms of complexity. I think user can be expected to think before they create one.

abitmore · 2018-08-09T19:59:48Z

bsip-0040.md

+#### `contains, not_contains`
+Stateless assert, for `list` type arguments.
+- `contains`: The `argument value` must contain all items specified by `data`, but can contain more
+- `not_contains`: The `argument value` must NOT contain any of the items specified by `data`, but can contain others


TBH I don't like to use a list as parameter of a function named contains, because it's not common. A common contains function (or shorter name "has") means "A is a list, B is in the list". In some programming libs there are "contains_all" functions which fit your design better.

I have changed the wording to contains_all and contains_none. Single object contains is a special case of list-based contains, I added that that can be added if beneficial for performance.

sschiessl-bcp · 2018-08-10T12:09:12Z

Added Milestone 4: Number of executions which allows to limit number of executions

sschiessl-bcp · 2018-08-13T07:44:19Z

Anything we can learn for implementation here from EOS?

Saw this here:
https://steemit.com/eos/@ottomagic/how-to-create-a-separate-eos-permission-for-voting-using-cleos

abitmore · 2018-08-13T09:48:40Z

@sschiessl-bcp according to the post, it seems EOS implemented per-action custom permission. I didn't look into the code, so don't know whether can set extra restrictions there. Actions in EOS are like operations in BitShares, the difference is in EOS actions belong to contracts and contracts can be upgraded with transactions, while in BitShares changing logic of operations requires hard-forks.

sschiessl-bcp · 2018-08-13T09:51:36Z

@sschiessl-bcp according to the post, it seems EOS implemented per-action custom permission. I didn't look into the code, so don't know whether can set extra restrictions there. Actions in EOS are like operations in BitShares, the difference is in EOS actions belong to contracts and contracts can be upgraded with transactions, while in BitShares changing logic of operations requires hard-forks.

Ok thanks, I never looked into EOS that much yet. It is similar, but has no restrictions. Just wanted to point it out in case there is something in their implementation we can utilize

sschiessl-bcp · 2018-08-13T09:52:04Z

Merge this now, we will?

abitmore · 2018-08-13T09:56:25Z

About implementation, EOS separated the feature into 2 actions (yes I guess it's actions of a system contract), one for adding custom authorities with a key (and perhaps can add advanced authority), the other for adding a restriction (permitted action) to specified authority.

sschiessl-bcp · 2018-08-13T09:58:18Z

About implementation, EOS separated the feature into 2 actions (yes I guess it's actions of a system contract), one for adding custom authorities with a key (and perhaps can add advanced authority), the other for adding a restriction (permitted action) to specified authority.

Ah, I see. The equivalent to our approach would be to but the same authority in multiple custom active authorities. Should we consider splitting it up as well?

abitmore · 2018-08-13T10:11:43Z

Should we consider splitting it up as well?

I'll think about it when implementing transaction verification logic, to see which one is better for us. Currently I'm trying the non-split way which is better for us due to the static billing (fees) mechanism. EOS can do dynamic billing (on CPU time and etc) on contract execution, so they can implement either way.

abitmore · 2018-08-13T10:24:20Z

bsip-0040.md

+
+This BSIP will be split into parts that will be voted on separately (see Milestones section). All of the above keys are possible with Milestone 1. Milestone 2 allows stateful restrictions (e.g. allow market orders up to some amount for every month), Milestone 3 gives finer control of how to combine restrictions and Milestone 4 allows to set a number of allowed executions per authority (introduces an additional on-chain dependency). Milestone 2, 3 and 4 will be put up for voting if Milestone 1 proves successful.
+
+# Rational


abitmore · 2018-08-13T10:44:49Z

Merge this now, we will?

Will.

Stefan Schießl and others added 9 commits July 25, 2018 13:19

Custom active permissions

15fbf7f

Update bsip-0040.md

7a13f4d

Update bsip-0040.md

aed7556

Update bsip-0040.md

e928c44

Update bsip-0040.md

e8fabfd

Update bsip-0040.md

5ebcbb6

Update bsip-0040.md

a825307

Update bsip-0040.md

239b7a2

Update README.md

28f96ba

clockworkgr mentioned this pull request Jul 25, 2018

Threshold authority weights per operation. bitshares/bitshares-core#1061

Closed

6 tasks

abitmore mentioned this pull request Jul 25, 2018

Additional role/permission/authority to limit for trading bitshares/bitshares-core#693

Closed

pmconrad reviewed Jul 26, 2018

View reviewed changes

This was referenced Jul 26, 2018

Transfer only to whitelisted Accounts and trade only with whitelisted Assets bitshares/bitshares-core#1044

Open

Generate active keys with limited permissions? bitshares/bitshares-core#590

Closed

Stefan Schießl added 3 commits July 27, 2018 13:14

Comments from pmconrad

10fa5ad

add more detailed asserts description and milestones

815a50c

include more assert descrption and lisght rewriting

8591695

Stefan Schießl added 4 commits July 27, 2018 14:52

Update bsip-0040.md

824549e

Update bsip-0040.md

71e57c5

Update bsip-0040.md

54d0ee6

Update bsip-0040.md

1083311

sschiessl-bcp closed this Aug 9, 2018

sschiessl-bcp reopened this Aug 9, 2018

abitmore reviewed Aug 9, 2018

View reviewed changes

Stefan Schießl added 9 commits August 10, 2018 06:56

add neq

e9c8e9b

Clarify duration

f9c2df5

If beneficial for performance or complexity sections

bd2af37

Update bsip-0040.md

0c43bfd

Update bsip-0040.md

0f2a433

add remaining_executions

982348c

Update bsip-0040.md

64338a4

Milestone 4 in separate section

02bb9e5

Update bsip-0040.md

1d0a341

Stefan Schießl added 2 commits August 10, 2018 22:42

Update bsip-0040.md

e749319

Update bsip-0040.md

51d46da

abitmore reviewed Aug 13, 2018

View reviewed changes

Stefan Schießl added 2 commits August 13, 2018 13:00

Update bsip-0040.md

b560291

Update bsip-0040.md

b0dfaa5

xeroc approved these changes Aug 16, 2018

View reviewed changes

xeroc merged commit 81efae4 into bitshares:master Aug 16, 2018

abitmore mentioned this pull request Jul 20, 2019

BSIP40 Implementation bitshares/bitshares-core#1860

Merged


		# Abstract

		Strengthening user security is one of the main factors to elevate BitShares. Inlight of recent


		This BSIP will be split into parts that will be voted on separately (see Milestones section). All of the above keys are possible with Milestone 1. Milestone 2 allows stateful restrictions (e.g. allow market orders up to some amount for every month), Milestone 3 gives finer control of how to combine restrictions and Milestone 4 allows to set a number of allowed executions per authority (introduces an additional on-chain dependency). Milestone 2, 3 and 4 will be put up for voting if Milestone 1 proves successful.

		# Rational

Custom active permission #86

Custom active permission #86

Conversation

sschiessl-bcp commented Jul 25, 2018

abitmore commented Jul 25, 2018

sschiessl-bcp commented Jul 25, 2018

abitmore commented Jul 25, 2018 • edited Loading

sschiessl-bcp commented Jul 25, 2018

abitmore commented Jul 25, 2018

sschiessl-bcp commented Jul 26, 2018 • edited Loading

xeroc commented Jul 26, 2018 • edited Loading

Limitations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschiessl-bcp commented Jul 27, 2018 • edited Loading

clockworkgr commented Jul 27, 2018

sschiessl-bcp commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschiessl-bcp Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

abitmore Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschiessl-bcp Aug 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschiessl-bcp commented Aug 10, 2018

sschiessl-bcp commented Aug 13, 2018

abitmore commented Aug 13, 2018

sschiessl-bcp commented Aug 13, 2018

sschiessl-bcp commented Aug 13, 2018

abitmore commented Aug 13, 2018

sschiessl-bcp commented Aug 13, 2018

abitmore commented Aug 13, 2018

Choose a reason for hiding this comment

abitmore commented Aug 13, 2018

abitmore commented Jul 25, 2018 •

edited

Loading

sschiessl-bcp commented Jul 26, 2018 •

edited

Loading

xeroc commented Jul 26, 2018 •

edited

Loading

sschiessl-bcp commented Jul 27, 2018 •

edited

Loading

sschiessl-bcp Aug 10, 2018 •

edited

Loading

abitmore Aug 10, 2018 •

edited

Loading

sschiessl-bcp Aug 12, 2018 •

edited

Loading