Added prefix for DARC storage

[pierluca]

There's a weakness in Dela (https://github.com/dedis/dela/issues/258) whereby the (existing) smart contracts store key=values without any prefix for the keys in a shared key/value store. Effectively this would allow any malicious user to overwrite other smart contracts' data.

This fixes it for the Distributed Access Rights Controls (DARC) functionality.

[pierluca]

Random thought @jbsv (and @nkcr if you're still peeking 😜):

Right now, we're fixing #258 by adding yet another custom prefix for each contract. As I'm doing this, I realize that we already have a unique identifying value for each contract, we just happen to use it exclusively for access control. Should we drop the "custom prefix" (i.e., "VALU", "DARC", etc.), limit the access key (aKey) variable to 4 bytes (down from 32 bytes) and then use it as a prefix in the storage ? This would avoid the proliferation of constants that should be known for each contract. Arguably, it would even make sense to make the "aKey" public for each contract, since it'd then also be used as a prefix in the K/V store.

Any thoughts ?

[lanterno]

Random thought @jbsv (and @nkcr if you're still peeking 😜):

Right now, we're fixing #258 by adding yet another custom prefix for each contract. As I'm doing this, I realize that we already have a unique identifying value for each contract, we just happen to use it exclusively for access control. Should we drop the "custom prefix" (i.e., "VALU", "DARC", etc.), limit the access key (aKey) variable to 4 bytes (down from 32 bytes) and then use it as a prefix in the storage ? This would avoid the proliferation of constants that should be known for each contract. Arguably, it would even make sense to make the "aKey" public for each contract, since it'd then also be used as a prefix in the K/V store.

Any thoughts ?

I was thinking about that while reading https://github.com/dedis/dela/pull/260 and wondering if there could be a generic way to add the prefix. but then the prefixKey would need to be centralized, and have access to more than just the key. so, probably there is 3 problems to solve here

1. Always adding a prefix while storing a key, and always removing it while reading it.
2. Centralize the prefixing for all modules that would be interfacing the store - would it extend the existing store module?
3. change prefixKey to have access to the contextual environment of the key (maybe contract metadata of some sort?)

(I'm still starting to learn more about dela, so excuse me if my lack of knowledge makes this comment not really useful.)

[pierluca]

You're absolutely right @lanterno. We avoided solution n° 2 as we want the K/V store to be as unaware of its usage as possible. We could add an adapter that implements this prefixing abstraction, but for now we went with just solving n° 1, which is pretty simple and self-sufficient. A prefixing adapter in front of the storage would probably be cleaner though.

[coveralls]

Pull Request Test Coverage Report for Build 5832016077

• 66 of 69 (95.65%) changed or added relevant lines in 11 files are covered.
• No unchanged relevant lines lost coverage.
• Overall first build on access_prefix at 98.721%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core/ordering/cosipbft/contracts/viewchange/viewchange.go	12	15	80.0%
<!--	Total:	66	69	95.65%	-->

Totals
Change from base Build 5794484000:	98.7%
Covered Lines:	14664
Relevant Lines:	14854

---

💛 - Coveralls

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

62.9% Coverage
2.2% Duplication

[nkcr]

Scoping the store accesses with a contract prefix is a reasonable solution.

Ultimately, it would be nice to have an access control by sotre elements (i.e. keys) instead of by contracts. It would give more flexibility such as the ability to share keys between contracts. We are not really far from that actually. This would require to have a custom darc implementation that matches key prefix to R/W/D actions or whatever rules we want to express.

Also, the check could be done at the module level (the "native" smart contract) instead of individually from each contract definition.

Something along those lines:

// execution/native/native.go

// Execute implements execution.Service. It uses the executor to process the
// incoming transaction and return the result.
func (ns *Service) Execute(snap store.Snapshot, step execution.Step) (execution.Result, error) {
    name := string(step.Current.GetArg(ContractArg))

    contract := ns.contracts[name]
    if contract == nil {
        return execution.Result{}, xerrors.Errorf("unknown contract '%s'", name)
    }

    res := execution.Result{
        Accepted: true,
    }

    // should come from the service
    var accessSrv access.Service

    // smart contracts are provided a secured snap that checks actions on it
    secureSnap := newSecureSnap(snap, accessSrv, step.Current.GetIdentity())

    err := contract.Execute(secureSnap, step)
    if err != nil {
        res.Accepted = false
        res.Message = err.Error()
    }

    return res, nil
}

func newSecureSnap(snap store.Snapshot, accessSrv access.Service,
    identity access.Identity) store.Snapshot {

    return secureSnap{...}
}

type secureSnap struct {
    snap      store.Snapshot
    accessSrv access.Service
    identity  access.Identity
}

func (snap secureSnap) Get(key []byte) ([]byte, error) {
    creds := newSnapCred(key, "read")

    err := snap.accessSrv.Match(snap.snap, creds, snap.identity)
    if err != nil {
        return nil, xerrors.Errorf("verification failed: %v", err)
    }

    return snap.snap.Get(key)
}

func (snap secureSnap) Set(key []byte, value []byte) error {
    creds := newSnapCred(key, "write")

    err := snap.accessSrv.Match(snap.snap, creds, snap.identity)
    if err != nil {
        return xerrors.Errorf("verification failed: %v", err)
    }

    return snap.snap.Set(key, value)
}

func (snap secureSnap) Delete(key []byte) error {
    creds := newSnapCred(key, "delete")

    err := snap.accessSrv.Match(snap.snap, creds, snap.identity)
    if err != nil {
        return xerrors.Errorf("verification failed: %v", err)
    }

    return snap.snap.Delete(key)
}

func newSnapCred(key []byte, action string) snapCred {
    return snapCred{
        key:    key,
        action: action,
    }
}

// snapCred defines credentials for operations on a snap. The credentials match
// the "read", "write", "delete" actions defined on the 4 first bytes of a key.
// For example here the rule on a single key prefix:
//
//   -- 0xabcdef12
//     -- "read"
//        -- Alice
//        -- Bob
//     -- "write"
//        -- Bob
//     -- "delete"
//        -- Bob
//
type snapCred struct {
    key    []byte
    action string
}

func (sc snapCred) GetID() []byte {
    return append([]byte{}, sc.key[:4]...)
}

func (cs snapCred) GetRule() string {
    return cs.action
}

It could be optimized for example by performing the checks in batches. We could also express rules differently with "*" or "R/W", or by including the contract to express rules like "User Bob can write key X for contract A but only read key X for contract B":

-- 0xabcdef12
  -- contractA:read
    -- Bob
  -- contractB:write
    -- Bob