Serve multiple tenants from a single node #1105

trung · 2020-12-12T04:17:52Z

Support multiple tenants in a single node while preserving data isolation
- Introduce --multitenancy flag to enable the feature
Use RPC Security Plugin to support tenancy protection
- Design access token scope to provide granular access controls
Introduce AccountExtraData to provide flexibility in managing additional data for account state. AccountExtraData is composed of PrivacyMetadata and managed parties

Also fixes #1115

core/state/state_object.go

core/state_transition.go

core/blockchain.go

core/state_transition.go

core/vm/evm.go

extension/api.go

internal/ethapi/api.go

multitenancy/decision_manager.go

multitenancy/types.go

rpc/server.go

multitenancy/decision_manager.go

nmvalera · 2021-01-12T16:46:24Z

internal/ethapi/api.go

+		fields["logs"] = filteredLogs
+		receiptClone := &types.Receipt{PostState: receipt.PostState, Status: receipt.Status, Logs: filteredLogs}
+		fields["logsBloom"] = types.CreateBloom(types.Receipts{receiptClone})
+	}


It looks like "status" field is never overridden in case of multi-tenancy. So if a user accesses a receipt of a tx that failed and that was sent to a contract the user has no access to then the user would still be able to deduce that the transaction failed from the receipt. Is my understanding correct here? If correct, I think the user should instead see a success status in the receipt (same as if the user was accessing receipt through a node that was not a participant of the transaction)

It looks like "contractAddress" is never overridden. So if a user tries to access a receipt of a successful contract deployment and the user has no access to the deployed contract then the user would still be able to deduce that the deployment actually succeeded from the receipt. Is my understanding correct here? If correct, I think the user should instead see an empty "contractAddress" field in this case (same as if the user was accessing receipt through a node that was not a participant of the transaction)

An edge case is for failed deployment tx. I don't see any way to hide that the tx failed to non-authorized users.

Without multitenancy, contractAddress is always visible even if user was not a participant.

and 1. seem having the same context, do you foresee any further security concern after knowing if a contract is accessible or not? I would argue to keep the original tx status to reflect the correct status of the transaction execution. Without multitenancy, party-protection transactions also produce fail status for transactions sent by non-party.

Without multitenancy, a non participant would see an empty contractAddress and not the effective deployment address (since a non-participant has no access to the tx payload for deployment). Or am I missing something here?

In a non-multitenant world, a non-participant always sees a success status for private transactions. Or am I missing something here?

core/vm/evm.go

internal/ethapi/api.go

ricardolyn · 2021-01-18T10:11:56Z

core/state/state_object.go

+// Note:
+// - update copy() method
+// - update DecodeRLP and EncodeRLP when adding new field
+type AccountExtraData struct {


to improve the structure, I think would be better to move AccountExtraData to a separate file (e.g core/state/account_extra_data.go). moreover, it could be applied the same approach to PrivacyMetadata.

ricardolyn · 2021-01-18T10:24:27Z

core/vm/interpreter.go

@@ -217,7 +217,9 @@ func (in *EVMInterpreter) Run(contract *Contract, input []byte, readOnly bool) (
 		if in.evm.quorumReadOnly && operation.writes {
 			return nil, fmt.Errorf("VM in read-only mode. Mutating opcode prohibited")
 		}
-
+		if err := in.evm.captureOperationMode(operation.writes); err != nil {


should we add the Quorum section comments?

ricardolyn · 2021-01-18T10:29:18Z

internal/ethapi/api.go

+// Quorum
+//
+// Retrieve private payload and construct the original transaction
+func buildPrivateTransaction(tx *types.Transaction) (*types.Transaction, error) {


those functions seem not directly related to the api.go and to avoid more conflicts on geth upgrades, could be moved to a separate file.

ricardolyn · 2021-01-18T12:17:29Z

core/state_transition.go

@@ -304,6 +314,28 @@ func (st *StateTransition) TransitionDb() (ret []byte, usedGas uint64, failed bo
 	}
 	// End Quorum - Privacy Enhancements

+	// do the affected contract managed party checks
+	if msg, ok := msg.(PrivateMessage); ok && isQuorum && st.evm.SupportsMultitenancy && msg.IsPrivate() {
+		if len(managedPartiesInTx) > 0 {


non-blocking: would make sense to move blocks of code like this to be done in other external functions to make less impact on geth code?

ricardolyn · 2021-01-18T12:17:49Z

core/vm/evm.go

+			evm.affectedContracts[address] = newAffectedType(MessageCall, ModeUnknown)
+			// for multitenancy, we cache the expectation of AffectedMode to be used later
+			// when an opcode is executed.
+			if evm.SupportsMultitenancy && evm.AuthorizeMessageCallFunc != nil {


same question here

nmvalera · 2021-01-18T18:11:15Z

multitenancy/decision_manager.go

+// based on what is entitled in the proto.PreAuthenticatedAuthenticationToken
+// and what is asked in ContractSecurityAttribute list.
+// Note: place holder for future, this is to protect Value Transfer between accounts.
+type AccountAccessDecisionManager interface {


AccessDecisionManager naming is not very standard also this is not really a manager since it only does checks.

I think AccountAuthorizationChecker would be clearer.

nmvalera · 2021-01-18T18:12:11Z

multitenancy/decision_manager.go

+// ContractAccessDecisionManager performs authorization checks for contract
+// based on what is entitled in the proto.PreAuthenticatedAuthenticationToken
+// and what is asked in ContractSecurityAttribute list.
+type ContractAccessDecisionManager interface {


Same goes here: renaming ContractAccessDecisionManager to ContractAuthorizationChecker would be clearer.

Actually re-reading this what is the upside of splitting into 2 interfaces?

Could we not have a single interface:

type Checker interface { IsAccountAuthorized(...) IsContractAuthorized(...) }

?

nmvalera · 2021-01-18T18:19:34Z

multitenancy/decision_manager.go

+type AuthorizeCreateFunc func() bool
+
+// AuthorizeMessageCallFunc returns if a contract is authorized to be read / write
+type AuthorizeMessageCallFunc func(contractAddress common.Address) (authorizedRead bool, authorizedWrite bool, err error)


AuthorizeMessageCallFunc is misleading: it returns authorizations to be checked elsewhere in the codebase which I don't think should happen.

Authorization checks logic should be centralized in the multitenancy package that then exposes IsAuthorized functions to the rest of the codebase.

Actually something quite misleading overall is also that we not clearly differentiate between multi-tenancy

at block processing time

at simulation time

I think this should be captured in some interfaces declared under multitenancy package and then imported.

nmvalera · 2021-01-18T18:25:35Z

multitenancy/types.go

+
+// ContractSecurityAttribute contains security configuration ask
+// which are defined for a secure contract account
+type ContractSecurityAttribute struct {


I am not a fan of SecurityAttribute naming which is not standard for authentication stuff.

What about "Claim"?

multitenancy/types.go

nmvalera · 2021-01-18T18:34:20Z

core/state/state_object.go

+// Note:
+// - update copy() method
+// - update DecodeRLP and EncodeRLP when adding new field
+type AccountExtraData struct {


nmvalera · 2021-01-25T13:36:49Z

core/vm/evm.go

@@ -258,7 +249,7 @@ func ModeOf(isWrite bool) AffectedMode {
 }

 func (mode AffectedMode) IsAuthorized(actualMode AffectedMode) bool {
-	return mode&actualMode != actualMode
+	return (mode&ModeUpdated == ModeUpdated) && (mode&actualMode != actualMode)


Why do we need to do also check for ModeUpdated?

That is to indicate that the expectation has been set. We need this new mode as we set default to ModeUnknown when capturing affected contracts.

nmvalera · 2021-01-25T13:40:04Z

core/vm/evm.go

@@ -705,6 +696,7 @@ func getDualState(evm *EVM, addr common.Address) StateDB {

 	if evm.PrivateState().Exist(addr) {
 		state = evm.PrivateState()
+		evm.captureAffectedContract(addr, ModeUnknown)


nmvalera · 2021-01-25T14:03:25Z

core/vm/evm.go

+//
+// updateAffectedMode is usied in simulation/eth_call for multitenancy, it sets the expectation of AffectedMode
+// to be verified later when an opcode is executed.
+func (evm *EVM) updateAffectedMode(address common.Address) error {


updateAffectedMode looks to do a lot could we instead do:

(mode AffectedMode) Update(authorizedRead, authorizedWrite bool) AffectedMode that updates the mode

Update (evm *EVM) captureAffectedContract(...) *AffectedType (returning the Affected type but not doing updateAffectedMode so not returning any error)

In getDualState do

affectedType, err := evm.captureAffectedMode(...) ... if evm.AuthorizeMessageCallFunc != nil { authorizedRead, authorizedWrite, err := evm.AuthorizeMessageCallFunc(address) ... affectedType.mode.Update(authorizedRead, authorizedWrite) }

nmvalera · 2021-01-25T15:38:15Z

core/vm/evm.go

+	if !evm.SupportsMultitenancy || evm.AuthorizeMessageCallFunc == nil {
+		return nil
+	}
+	affectedType, found := evm.affectedContracts[address]


On line 860 you are already looking for evm.affectedContracts[address] we probably do not need to do it a second time here.

nmvalera · 2021-01-25T15:39:23Z

core/vm/evm.go

+	log.Debug("AffectedMode changed", "address", address.Hex(), "old", affectedType.mode, "new", authorizedMode, "read", authorizedRead, "write", authorizedWrite)
+	// if we don't authorize either read/write, it's unauthorized access
+	// and we need to inform EVM
+	if !authorizedRead && !authorizedWrite {


Why not checking this right after authorizedRead, authorizedWrite, err := evm.AuthorizeMessageCallFunc(address) on line 871?

Actually would it even make sense to return an error from evm.AuthorizeMessageCallFunc() in case both authorizedRead and authorizedWrite are false ?

So then we can directly do
affectedType.mode = affectedType.mode.Update(authorizedRead, authorizedWrite)

PrivacyMetadata was added in Privacy Enhancements. It is linked to each private contract account and stored in a trie to allow rollback, especially for contract extension. This change is to generalize the extra data that supplements the account data by introducing AccountExtraData. The first extra data is PrivacyMetadata. An additional improvement is to make sure AccountExtraData is also linked to public accounts for future needs.

add privateFrom to contract extension automated transactions

…e privacy metadata

… type

Co-authored-by: Nicolae Leonte <nicolae.leonte.go@gmail.com>

… seperating the loop

…ing log topics

…gs/events to be retrieved)

…same change in tessera)

…ing simulation/eth_call and block processing

nmvalera

LGTM

trung requested review from nicolae-leonte-go and vsmk98 December 14, 2020 21:06

nicolae-leonte-go reviewed Dec 15, 2020

View reviewed changes

core/state/state_object.go Show resolved Hide resolved

nicolae-leonte-go reviewed Dec 15, 2020

View reviewed changes

core/state_transition.go Outdated Show resolved Hide resolved

nicolae-leonte-go previously approved these changes Dec 15, 2020

View reviewed changes

trung dismissed nicolae-leonte-go’s stale review via 77a89e0 December 16, 2020 15:46

trung force-pushed the multitenant branch from 8a6e8c2 to 5908486 Compare December 16, 2020 17:27

trung mentioned this pull request Dec 17, 2020

Serve multi tenants from a single node Consensys/quorum-acceptance-tests#72

Merged

nmvalera reviewed Dec 18, 2020

View reviewed changes

MadelineMurray mentioned this pull request Dec 20, 2020

Doc multi-tenancy Consensys/doc.goquorum#57

Closed

nmvalera reviewed Jan 14, 2021

View reviewed changes

trung force-pushed the multitenant branch 6 times, most recently from 6eed243 to a6ab773 Compare January 15, 2021 18:46

ricardolyn reviewed Jan 18, 2021

View reviewed changes

trung force-pushed the multitenant branch 2 times, most recently from f18d1c4 to 642ffad Compare January 22, 2021 20:24

nmvalera reviewed Jan 25, 2021

View reviewed changes

trung force-pushed the multitenant branch 2 times, most recently from 3020699 to 7a9f183 Compare January 25, 2021 15:17

nmvalera reviewed Jan 25, 2021

View reviewed changes

trung force-pushed the multitenant branch from 7a9f183 to 8025009 Compare January 25, 2021 16:33

trung and others added 5 commits January 26, 2021 09:41

support multi tenancy leveraging JSON RPC security plugin

77b7792

populate RPC call context with preauthenticated token

b4e9868

fix eth_sendTransaction for multitenancy

7265193

Add test case to check for overlap of scopes.

d14cc38

trung and others added 26 commits January 26, 2021 10:36

update RLP encoding for AccountExtraData

58718a6

Remove API version from endpoints that haven't changed.

ac100c8

improve RLP encoding for AccountExtraData

7529d33

contractExtension changes for multitenancy

be8d3af

add sender to send/receive tessera responses

8ec0377

add privateFrom to contract extension automated transactions

update state object to return an error when the contract does not hav…

12978e7

…e privacy metadata

add docs and small refactoring

d41c219

improve decision manager with READ permission

53fab4c

improve privacy when reading managed parties

d7b8291

update permision for extension status check

2269727

only set managed parties if multitenancy is enabled

e9b232c

refactor to inject EthAPIBackend cleanly

99d39d3

fix unit tests and lint

f355375

populate index for newly created contracts regardless the transaction…

d6be9b2

… type

improve error handling when refactoring simulation logic for reusability

d7679b4

Co-authored-by: Nicolae Leonte <nicolae.leonte.go@gmail.com>

add documentation to reflect review feedbacks

07af361

refactor to reflect the review feedbacks about variadic arguments and…

64ad3b5

… seperating the loop

return empty logs instead of 'not authorized' to avoid attackers prob…

508a3b0

…ing log topics

improve simulation with runtime multitenancy checks

725b9e3

add contract level access controls for logs and receipts

d743c0b

fix lint issue

13ea1b3

add logsBloom calculation after filtering logs

ac84340

remove receiptAddress access checks (to allow relevant transaction lo…

596bcbc

…gs/events to be retrieved)

rename sender to senderKey in send/receive responses (reflecting the …

bb6b023

…same change in tessera)

modified per review feedbacks

5014b2b

add logic to perform authorization check for contract code access dur…

f165b8f

…ing simulation/eth_call and block processing

trung force-pushed the multitenant branch from 8025009 to f165b8f Compare January 26, 2021 16:00

nmvalera self-requested a review January 26, 2021 19:52

nmvalera approved these changes Jan 26, 2021

View reviewed changes

nmvalera merged commit 8bd0643 into Consensys:master Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve multiple tenants from a single node #1105

Serve multiple tenants from a single node #1105

trung commented Dec 12, 2020 •

edited

Loading

nmvalera Jan 12, 2021

trung Jan 14, 2021 •

edited

Loading

nmvalera Jan 18, 2021

ricardolyn Jan 18, 2021

nmvalera Jan 18, 2021

ricardolyn Jan 18, 2021

ricardolyn Jan 18, 2021

ricardolyn Jan 18, 2021

ricardolyn Jan 18, 2021

nmvalera Jan 18, 2021

nmvalera Jan 18, 2021

nmvalera Jan 18, 2021

nmvalera Jan 18, 2021

nmvalera Jan 18, 2021

nmvalera Jan 18, 2021

nmvalera Jan 18, 2021

nmvalera Jan 25, 2021

trung Jan 25, 2021

nmvalera Jan 25, 2021

nmvalera Jan 25, 2021

nmvalera Jan 25, 2021

nmvalera Jan 25, 2021

nmvalera left a comment

Serve multiple tenants from a single node #1105

Serve multiple tenants from a single node #1105

Conversation

trung commented Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

trung Jan 14, 2021 • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmvalera left a comment

Choose a reason for hiding this comment

trung commented Dec 12, 2020 •

edited

Loading

trung Jan 14, 2021 •

edited

Loading