Feature/solution skvs #766

chenchanglew · 2024-06-11T12:56:04Z

What this PR does / why we need it:

Implement a Rollback attack protection solution for FPC: SKVS.

Single Key-Value Storage (SKVS) is a naive approach for rollback attacks. All key-value pairs are encapsulated and stored in this approach with a single call to put_state(). During execution, the enclave must load the entire state before accessing individual key-value pairs. While this approach prevents the rollback attack, applications with large states and multiple writers will experience bad performance, as the use of a single key-value pair will cause transactions to fail due to concurrent write issues.

A user can use it by changing the chain code to SVKS chaincode
ex: skvsChaincode := fpc.NewPrivateChaincode(secretChaincode, fpc.WithSKVS())

for example we can test with skvs/main.go in secret keeper
make ECC_MAIN_FILES=cmd/skvs/main.go with_go env docker

Which issue(s) this PR fixes:
Fixes #484

Special notes for your reviewer:
Loom demonstration video: Watch here

mbrandenburger · 2024-06-25T14:40:48Z

samples/chaincode/secret-keeper-go/main.go

+	// chaincode := fpc.NewPrivateChaincode(secretChaincode)
+	skvsChaincode := fpc.NewSkvsChaincode(secretChaincode)

 	// start chaincode as a service
 	server := &shim.ChaincodeServer{
 		CCID:    ccid,
 		Address: addr,
-		CC:      chaincode,
+		CC:      skvsChaincode,


I was wondering if we want multiple main files? So we have an example to run secret keeper with and without skvs?

/samples/chaincode/secret-keeper-go/cmd/simple/main.go /samples/chaincode/secret-keeper-go/cmd/skvs/main.go

A few words in the secret-keeper readme would be nice as well.

mbrandenburger · 2024-06-25T14:53:11Z

ecc_go/chaincode/enclave_go/skvs_stub_Interface.go

+	"github.com/pkg/errors"
+)
+
+type SkvsStubInterface struct {


the name of this file should be all small caps

mbrandenburger · 2024-06-25T15:04:08Z

ecc_go/chaincode/enclave_go/skvs_stub.go

+	return &skvsStub{enclaveStub}
+}
+
+func (e *skvsStub) ChaincodeInvoke(stub shim.ChaincodeStubInterface, chaincodeRequestMessageBytes []byte) ([]byte, error) {


I have the feeling that we should remove this wrapper and instead just inject a provider function for the stub that we can set externally.

no sure about this one, might need to ask more insight

mbrandenburger · 2024-12-18T09:07:16Z

@chenchanglew can you please rebase in resolve the conflict within ecc_go/build.mk

mbrandenburger · 2024-12-18T09:07:59Z

@munapower can you also please have look at this PR. Thanks!

munapower · 2025-01-03T20:01:42Z

@munapower can you also please have look at this PR. Thanks!

@mbrandenburger @chenchanglew not sure what I am supposed to test. @chenchanglew did you resolve the conflict Marcus mentions? What am I supposed to test? Should I follow the Readme or what?

mbrandenburger · 2025-01-07T16:15:35Z

samples/chaincode/secret-keeper-go/Makefile

+DEFAULT= cmd/naive/main.go
+SKVS_PATH = cmd/skvs/main.go
+
+MAIN_GO_PATH ?=$(DEFAULT)
+
+include $(TOP)/ecc_go/build.mk


We should add a small section in the readme of this demo to explain how to build the chaincode with these different options.

For example:

MAIN_GO_PATH=cmd/naive/main.go make or MAIN_GO_PATH=cmd/skvs/main.go make

There is nothing else the user need to change in order to use skvs, right?

this is just build for different solutions,
the real change need to do is here

secretChaincode, _ := contractapi.NewChaincode(&chaincode.SecretKeeper{}) // chaincode := fpc.NewPrivateChaincode(secretChaincode) skvsChaincode := fpc.NewSkvsChaincode(secretChaincode)

mbrandenburger · 2025-01-08T10:26:54Z

ecc_go/chaincode/singleKVS.go

+func NewSkvsChaincode(cc shim.Chaincode) *chaincode.EnclaveChaincode {
+	logger.Info("Creating new SKVS Chaincode")
+	ecc := &chaincode.EnclaveChaincode{
+		Enclave:   enclave_go.NewSkvsStub(cc),
+		Validator: endorsement.NewValidator(),
+		Extractor: &chaincode.ExtractorImpl{},
+		Ercc:      &ercc.StubImpl{},
+	}
+	return ecc
+}


I am wondering if it would be better to add an option to the NewPrivateChaincode method to instantiate the with SKVS rather than creating this new constructor.

It could look like ...

func NewPrivateChaincode(cc shim.Chaincode, options ...func(*chaincode.EnclaveChaincode)) *chaincode.EnclaveChaincode { ecc := &chaincode.EnclaveChaincode{ Enclave: enclave_go.NewSkvsStub(cc), Validator: endorsement.NewValidator(), Extractor: &chaincode.ExtractorImpl{}, Ercc: &ercc.StubImpl{}, } for _, o := range options { o(ecc) } return ecc } func WithSKVS() func(*chaincode.EnclaveChaincode) { return func(ecc *chaincode.EnclaveChaincode) { ecc.Enclave = enclave_go.NewSkvsStub(cc) } }

and in the chaincode main.go, the developer would write something like that ...

// naive chaincode := fpc.NewPrivateChaincode(secretChaincode) // with SKVS chaincode := fpc.NewPrivateChaincode(secretChaincode, fpc.WithSKVS)

See this article https://golang.cafe/blog/golang-functional-options-pattern.html

WDYT?

mbrandenburger

Thanks Zac for the refactoring. I've found a few things we should still fix. Please have a look.

mbrandenburger · 2025-05-01T14:30:12Z

ecc_go/chaincode/private.go

 	ecc := &chaincode.EnclaveChaincode{
-		Enclave:   enclave_go.NewEnclaveStub(cc),
+		Enclave:   enclave_go.NewSkvsStub(cc),


I believe here we want to keep the "normal" NewEnclaveStub

mbrandenburger · 2025-05-01T14:32:00Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

@@ -62,22 +62,18 @@ func (s *SkvsStubInterface) InitSKVS() error {
 		}
 	}

-	logger.Warningf("SKVS Init finish, allDataOld: %s, allDataNew: %s", s.allDataOld, s.allDataNew)


I would suggest that we remove these loggers as they may print sensitive data.

mbrandenburger · 2025-05-01T14:32:57Z

ecc_go/chaincode/enclave_go/skvs_stub.go

+)
+
+func NewSkvsStub(cc shim.Chaincode) *EnclaveStub {
+	logger.Warning("==== SKVS NewSkvsStub ====")


Suggested change

logger.Warning("==== SKVS NewSkvsStub ====")

mbrandenburger · 2025-05-01T14:34:32Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+}
+
+func NewSkvsStubInterface(stub shim.ChaincodeStubInterface, input *pb.ChaincodeInput, rwset *readWriteSet, sep StateEncryptionFunctions) *SkvsStubInterface {
+	logger.Warning("==== Get New Skvs Interface =====")


Suggested change

logger.Warning("==== Get New Skvs Interface =====")

mbrandenburger · 2025-05-01T14:36:51Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+	skvsStub := SkvsStubInterface{fpcStub, map[string][]byte{}, map[string][]byte{}, "SKVS"}
+	err := skvsStub.InitSKVS()
+	if err != nil {
+		logger.Warningf("Error!! Initializing SKVS failed")


If we have an error ... shouldn't we better through a panic? We cannot really go on and continue, right?

mbrandenburger · 2025-05-01T14:37:08Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+	skvsStub := SkvsStubInterface{fpcStub, map[string][]byte{}, map[string][]byte{}, "SKVS"}
+	err := skvsStub.InitSKVS()
+	if err != nil {
+		logger.Warningf("Error!! Initializing SKVS failed")


Suggested change

logger.Warningf("Error!! Initializing SKVS failed")

panic("Initializing SKVS failed")

mbrandenburger · 2025-05-01T14:37:21Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+}
+
+func (s *SkvsStubInterface) InitSKVS() error {
+	logger.Warningf(" === Initializing SKVS === ")


Suggested change

logger.Warningf(" === Initializing SKVS === ")

mbrandenburger · 2025-05-01T14:38:58Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+	if len(encValue) == 0 {
+		logger.Warningf("SKVS is empty, Initiating.")


should we also handle this error differently? Can we actually continue if encValue is empty?

yes, we can continue if the encValue is empty.

mbrandenburger · 2025-05-01T14:41:18Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+func (s *SkvsStubInterface) InitSKVS() error {
+	logger.Warningf(" === Initializing SKVS === ")
+
+	// get current state, this will only operate once


Do you think it makes sense to protect the entire Init method with sync.Once?
https://pkg.go.dev/sync#Once

I think this function is safe even it runs multiple time, since the
encValue, err := s.GetPublicState(s.key) will always fetch the latest value, if someone try to do something malicious by calling this init function they will just get the latest public state.
Plus I already change the init function into a private function thus I think it should be okay without the sync.Once.

mbrandenburger · 2025-05-01T14:41:37Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+		if err != nil {
+			return err
+		}
+		logger.Warningf("SKVS has default value, loading current value.")


Suggested change

logger.Warningf("SKVS has default value, loading current value.")

logger.Debug("SKVS has default value, loading current value.")

mbrandenburger · 2025-05-20T20:05:09Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+		allDataOld:       map[string][]byte{},
+		allDataNew:       map[string][]byte{},


Suggested change

allDataOld: map[string][]byte{},

allDataNew: map[string][]byte{},

allDataOld: make(map[string][]byte),

allDataNew: make(map[string][]byte),

mbrandenburger · 2025-05-20T20:06:07Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+	}
+	err := skvsStub.initSKVS()
+	if err != nil {
+		panic("Initializing SKVS failed")


Suggested change

panic("Initializing SKVS failed")

panic(fmt.Sprintf("Initializing SKVS failed, err: %v", err))

mbrandenburger · 2025-05-20T20:11:40Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+func (s *SkvsStubInterface) GetState(key string) ([]byte, error) {
+	value, found := s.allDataOld[key]
+	if !found {
+		return nil, errors.New("skvs allDataOld key not found")


Mhh, shouldn't we return nil, nil in that case?

// If the key does not exist in the state database, (nil, nil) is returned. GetState(key string) ([]byte, error)

https://github.com/hyperledger/fabric-chaincode-go/blob/main/shim/interfaces.go#L80C1-L81C38

mbrandenburger · 2025-05-20T20:12:12Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+
+	"github.com/hyperledger/fabric-chaincode-go/shim"
+	pb "github.com/hyperledger/fabric-protos-go/peer"
+	"github.com/pkg/errors"


Suggested change

"github.com/pkg/errors"

mbrandenburger · 2025-05-20T20:12:35Z

ecc_go/chaincode/enclave_go/skvs_stub_interface.go

+func (s *SkvsStubInterface) GetState(key string) ([]byte, error) {
+	value, found := s.allDataOld[key]
+	if !found {
+		return nil, errors.New("skvs allDataOld key not found")


Suggested change

return nil, errors.New("skvs allDataOld key not found")

return nil, nil

Signed-off-by: chenchanglew <[email protected]>

mbrandenburger

Thanks! LGTM

chenchanglew requested a review from a team as a code owner June 11, 2024 12:56

chenchanglew force-pushed the feature/solution-skvs branch from 05c5cd2 to 8fb8b84 Compare June 11, 2024 13:04

mbrandenburger reviewed Jun 25, 2024

View reviewed changes

chenchanglew force-pushed the feature/solution-skvs branch 2 times, most recently from dbe4851 to 754f02d Compare November 26, 2024 18:08

chenchanglew requested a review from mbrandenburger November 26, 2024 18:09

mbrandenburger requested a review from munapower December 18, 2024 09:07

mbrandenburger requested changes Jan 8, 2025

View reviewed changes

chenchanglew force-pushed the feature/solution-skvs branch from 754f02d to dca2e20 Compare April 28, 2025 20:51

chenchanglew requested a review from mbrandenburger April 29, 2025 15:04

mbrandenburger added this to the Rollback protection extension milestone Apr 29, 2025

mbrandenburger requested changes May 1, 2025

View reviewed changes

chenchanglew requested a review from mbrandenburger May 13, 2025 23:36

chenchanglew force-pushed the feature/solution-skvs branch from f695c07 to 07e6fa9 Compare May 14, 2025 22:58

mbrandenburger reviewed May 20, 2025

View reviewed changes

chenchanglew and others added 8 commits May 27, 2025 16:35

add singleKVS wrapper

6273974

Signed-off-by: chenchanglew <[email protected]>

able to save into single key

4929293

Signed-off-by: chenchanglew <[email protected]>

change to only getState Once

9ecb879

Signed-off-by: chenchanglew <[email protected]>

change storing type and add del state

114d803

Signed-off-by: chenchanglew <[email protected]>

refactor: clean up code

a13d1a3

Signed-off-by: chenchanglew <[email protected]>

lint: make checks

ed22ff2

Signed-off-by: chenchanglew <[email protected]>

chore: change filename to small letters

dc8bdb2

Signed-off-by: chenchanglew <[email protected]>

chore: refactor SkvsStubInterface

61d3af6

Signed-off-by: chenchanglew <[email protected]>

chenchanglew added 5 commits May 27, 2025 16:35

feat: change secret-keeper-go makefile

46e0ffb

Signed-off-by: chenchanglew <[email protected]>

fix: edit makefile

3006ec9

Signed-off-by: chenchanglew <[email protected]>

refactor: use functional options pattern to apply skvs option

873aeae

Signed-off-by: chenchanglew <[email protected]>

chore: remove logging and cleanup

b99495f

Signed-off-by: chenchanglew <[email protected]>

fix: resolves comments

f6c2ef5

Signed-off-by: chenchanglew <[email protected]>

chenchanglew force-pushed the feature/solution-skvs branch from 07e6fa9 to f6c2ef5 Compare May 27, 2025 14:36

chenchanglew requested a review from mbrandenburger May 27, 2025 15:00

mbrandenburger approved these changes May 28, 2025

View reviewed changes

munapower approved these changes May 28, 2025

View reviewed changes

mbrandenburger merged commit db4ff06 into hyperledger:main May 29, 2025
4 checks passed

	logger.Warningf("Error!! Initializing SKVS failed")
	panic("Initializing SKVS failed")

		if len(encValue) == 0 {
		logger.Warningf("SKVS is empty, Initiating.")

	logger.Warningf("SKVS has default value, loading current value.")
	logger.Debug("SKVS has default value, loading current value.")

		allDataOld: map[string][]byte{},
		allDataNew: map[string][]byte{},

	panic("Initializing SKVS failed")
	panic(fmt.Sprintf("Initializing SKVS failed, err: %v", err))

	return nil, errors.New("skvs allDataOld key not found")
	return nil, nil

Feature/solution skvs #766

Feature/solution skvs #766

Uh oh!

Conversation

chenchanglew commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrandenburger commented Dec 18, 2024

Uh oh!

mbrandenburger commented Dec 18, 2024

Uh oh!

munapower commented Jan 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrandenburger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrandenburger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chenchanglew commented Jun 11, 2024 •

edited

Loading