support for digital signatures + integration tests work (again) #501

bvavala · 2020-12-18T09:10:56Z

This PR add:

new protobufs for better handling digital signatures and rwset
implements digital signature computation and verification
implements rwset serialization and replay
makes integration tests work again

Notes:

lots of code, mainly in the enclave, needs refactoring and improved error handling
fabric (state) calls in the enclave are void, but should be modified to return any failure
Fabric's kv store consistency level has to be investigated, to ensure that FPC chaincodes match expected behavior of regular chaincodes (e.g., do reads always return the latest committed value first? or the most recent local write first? are writes of the same key collapsed into a single one, or result into multiple versions?)

…egrations; signature computation and validation; rwset serialization and replay; fixed echo test Signed-off-by: Bruno Vavala <[email protected]>

…ort for composite-key routines) Signed-off-by: Bruno Vavala <[email protected]>

Signed-off-by: Bruno Vavala <[email protected]>

g2flyer

Looks overall very good. Build requires a simple patch (see below) but then works. However, for me, in both SIM and HW mode, the auction_test fails for two cases (echo and deployment test succeed). Maybe related to the state consistency assumptions? (Looking at auction_test it could also have been due to attempting concurency which actually would be a problem for DRAW detection when reordering happens. But you forced a --waitForEvent inside peer.cli, so this should not be the issue?0

A number of inline comments. As mentioned there (but maybe a bit hidden). mock_enclave also has to be changed with the new message formats and pdo signature

From b0c16f4370607a027edc433b24e3b5e056f3c06d Mon Sep 17 00:00:00 2001
From: michael steiner <[email protected]>
Date: Fri, 18 Dec 2020 11:28:22 -0800
Subject: [PATCH] build fix

Signed-off-by: michael steiner <[email protected]>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 43753d5..50fd55d 100644
--- a/Makefile
+++ b/Makefile
@@ -8,8 +8,8 @@ include $(TOP)/build.mk

 SUB_DIRS = protos common internal ercc ecc_enclave ecc fabric client_sdk examples utils integration # docs

-FPC_SDK_DEP_DIRS = protos utils/fabric common ecc_enclave ecc
-FPC_PEER_DEP_DIRS = protos common ercc fabric ecc_enclave ecc
+FPC_SDK_DEP_DIRS = protos common ecc_enclave ecc utils/fabric
+FPC_PEER_DEP_DIRS = protos common ercc ecc_enclave ecc fabric
 # FPC_PEER_DEP_DIRS has to include protos, ecc, ecc_enclave, common and ercc only if we run chaincode in external builder directly on host and not indirectly via docker
 FPC_PEER_CLI_WRAPPER_DEP_DIRS = utils/fabric

--
2.29.2

g2flyer · 2020-12-18T19:33:48Z

common/crypto/pdo-crypto-c-wrapper.h

+    uint32_t max_hash_len,
+    uint32_t* actual_hash_len);
+
+bool verify_signature(uint8_t* public_key, uint32_t public_key_len, uint8_t* message, uint32_t message_len, uint8_t* signature, uint32_t signature_len);


for mock_enclave.go (and for symmetry) you will also need the generate signature?

common/error.h

g2flyer · 2020-12-18T19:42:24Z

ecc/chaincode/enclave/enclave.go

@@ -43,14 +41,16 @@ func NewEnclaveStub() StubInterface {
 }

 func (e *EnclaveStub) Init(chaincodeParams, hostParams, attestationParams []byte) ([]byte, error) {
+	const credentialsBufferMaxLen = 16 * 1024


This number sounds like magic. Can you add a comment on how you got to this number?

added comment

hmm, the comment doesn't really mention on how you got to that number. That it was an estimate i already had figured out before :-)

g2flyer · 2020-12-18T19:47:05Z

ecc/chaincode/enclave/enclave.go

@@ -106,6 +106,8 @@ func (e *EnclaveStub) GetEnclaveId() (string, error) {

 // ChaincodeInvoke calls the enclave for transaction processing
 func (e *EnclaveStub) ChaincodeInvoke(stub shim.ChaincodeStubInterface, crmProtoBytes []byte) ([]byte, error) {
+	const scresmProtoBytesMaxLen = 1024 * 100 // Let's be really conservative ...


See above, would be useful to add some comments on how we came to this number. Maybe also add a comment in the protobufs pointing to here so if we change there we also adapt number here?

added comment, if there isn't enough space...it will fail. In the future we should make it adaptive.

ecc/chaincode/ecc.go

fabric/bin/peer.sh

ecc_enclave/enclave/enclave.cpp

ecc_enclave/enclave/shim.cpp

Signed-off-by: Bruno Vavala <[email protected]>

g2flyer · 2020-12-18T20:54:47Z

Ah, notice that you also just found the build issue (note my commit tries to make the order of directories overall consistent across the different SUB_DIR definitions as these were also slightly different) and looking at the code i also realized the issue with auction test was composite keys ...

bvavala · 2020-12-18T21:16:43Z

Ah, notice that you also just found the build issue (note my commit tries to make the order of directories overall consistent across the different SUB_DIR definitions as these were also slightly different) and looking at the code i also realized the issue with auction test was composite keys ...

We really should support the fabric shim functions for the composite keys. FPC is now redefining the machinery and translating it for Fabric.

Signed-off-by: Bruno Vavala <[email protected]>

g2flyer · 2020-12-18T22:24:06Z

Ah, notice that you also just found the build issue (note my commit tries to make the order of directories overall consistent across the different SUB_DIR definitions as these were also slightly different) and looking at the code i also realized the issue with auction test was composite keys ...

We really should support the fabric shim functions for the composite keys. FPC is now redefining the machinery and translating it for Fabric.

Well, shim.h does define composite keys somewhat consistent with go shim. Just that the implementaion merges it in a singe ecall for get/put. If you look at the full FPC design and the related ECC<->TLCC channel with the corresponding verification function, i already split that back.

That said, we anyway don't support them for FPC lite right now. We could, of course, by demultiplexing them onto a single key, similar to doing so to support multiple keys. Not sure whether from a priority perspective it is on the top of the list though? I.e., given that an application can also handle it reasonably well at the application level (and us implementing in shim would still loose the data-parallel aspect of fabric), i would wait until there is push-back. Seems to me much less fundamental than support of seal (otherwise you simply won't survive a peer restart) or multi-enclave (robustness but also better security/fairness), neither of which an application writer can do anything at the app level

g2flyer

builds and runs now smoothly. I'm fine also with doing mock_enclave in a separate pr, in particular as (a) already in the current state we get again a working integration test and (b) doing this change for mock_enclave also bring in all the stuff mentioned in #502, so better done separately.

bvavala · 2020-12-19T04:40:55Z

Well, shim.h does define composite keys somewhat consistent with go shim. Just that the implementaion merges it in a singe ecall for get/put. If you look at the full FPC design and the related ECC<->TLCC channel with the corresponding verification function, i already split that back.

That's correct, but just part of the story.
If you also look at shim.go, particularly here and here, you'll see that the code embeds the recognition of composite keys (IsFPCCompositeKey) and it performs a translation by splitting them accordingly (SplitFPCCompositeKey).
Back to the auction example, an FPC composite key is .somePrefix.MyAuction1., which is translated to the Fabric composite key somePrefixMyAuction1.

g2flyer · 2020-12-19T05:54:43Z

That's correct, but just part of the story.
If you also look at shim.go, particularly here and here, you'll see that the code embeds the recognition of composite keys (IsFPCCompositeKey) and it performs a translation by splitting them accordingly (SplitFPCCompositeKey).
Back to the auction example, an FPC composite key is .somePrefix.MyAuction1., which is translated to the Fabric composite key somePrefixMyAuction1.

Not clear to me why why we added the '.' separator, it should also work without with the pre-fix match? but also not clear whether it has any impact on the developer? Looking at fabric go stub it seems that they (a) keep composite keys in a completely different namespace of non-composite-keys and (b) use MinUnicodeRuneValue as separator. Now the way we use the stub, we do inherit the namespace separation, i.e., (a) and as our separator is valid only during the transfer between enclave and go, it shouldn't really matter?

The main problem though would only be if the developer would use a non-composite key which uses the dot, thinking it would be separate?

https://github.com/hyperledger/fabric-chaincode-go/blob/master/shim/stub.go

bvavala · 2020-12-19T06:04:54Z

Not clear to me why why we added the '.' separator, it should also work without with the pre-fix match?

I speculate that originally the separator was something like \00, so this was translated to a ..
Now, I agree, handling Fabric composite keys should not be an issue given their current format.

The main problem though would only be if the developer would use a non-composite key which uses the dot, thinking it would be separate?

Yes, more generally: a developer must know about that (i) to use it, (ii) to avoid mistakes when choosing key strings, (iii) to avoid using Fabric's composite keys.

g2flyer · 2020-12-19T06:54:31Z

The main problem though would only be if the developer would use a non-composite key which uses the dot, thinking it would be separate?

Actually, this paragraph (and subsequent link) were left overs which i forgot to clean up and the paragraph above actually argues, based on the fabric go implementation that ...

Yes, more generally: a developer must know about that (i) to use it, (ii) to avoid mistakes when choosing key strings, (iii) to avoid using Fabric's composite keys.

... this is actually not a concern

bvavala added 2 commits December 17, 2020 23:27

new protobufs signed response, rwset; pdo c cryptowrappers; proto int…

bc5982d

…egrations; signature computation and validation; rwset serialization and replay; fixed echo test Signed-off-by: Bruno Vavala <[email protected]>

fix bugs, auction partially working (inconsistencies and missing supp…

7ce70f7

…ort for composite-key routines) Signed-off-by: Bruno Vavala <[email protected]>

bvavala requested a review from a team December 18, 2020 09:10

bvavala marked this pull request as draft December 18, 2020 09:11

add wait on invoke __endorse; auction test fixed

26ba7a2

Signed-off-by: Bruno Vavala <[email protected]>

bvavala marked this pull request as ready for review December 18, 2020 10:08

bvavala marked this pull request as draft December 18, 2020 20:07

bvavala added 2 commits December 18, 2020 12:08

fix handling of composite key writes

d46f309

Signed-off-by: Bruno Vavala <[email protected]>

fix compile order issue

4df41e6

Signed-off-by: Bruno Vavala <[email protected]>

g2flyer suggested changes Dec 18, 2020

View reviewed changes

fix include bug

92afe5c

Signed-off-by: Bruno Vavala <[email protected]>

bvavala mentioned this pull request Dec 18, 2020

Refactoring and improved error handling Go/cpp code following #501 #502

Open

4 tasks

comments and renaming

4138a9f

Signed-off-by: Bruno Vavala <[email protected]>

bvavala marked this pull request as ready for review December 18, 2020 22:28

g2flyer self-requested a review December 18, 2020 23:03

g2flyer approved these changes Dec 18, 2020

View reviewed changes

g2flyer merged commit 50a54ea into hyperledger:master Dec 18, 2020

This was referenced Dec 18, 2020

Validate current assumption of ledger behavior #503

Open

Read-set is missing versioning #446

Closed

Change enclave signature to be over proposal rather only args #460

Closed

bvavala mentioned this pull request Dec 19, 2020

Implement read set for composite key get_state #506

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for digital signatures + integration tests work (again) #501

support for digital signatures + integration tests work (again) #501

bvavala commented Dec 18, 2020 •

edited

Loading

g2flyer left a comment

g2flyer Dec 18, 2020

g2flyer Dec 18, 2020

bvavala Dec 18, 2020

g2flyer Dec 18, 2020

g2flyer Dec 18, 2020

bvavala Dec 18, 2020 •

edited

Loading

g2flyer commented Dec 18, 2020

bvavala commented Dec 18, 2020

g2flyer commented Dec 18, 2020

g2flyer left a comment

bvavala commented Dec 19, 2020

g2flyer commented Dec 19, 2020

bvavala commented Dec 19, 2020 •

edited

Loading

g2flyer commented Dec 19, 2020

support for digital signatures + integration tests work (again) #501

support for digital signatures + integration tests work (again) #501

Conversation

bvavala commented Dec 18, 2020 • edited Loading

g2flyer left a comment

Choose a reason for hiding this comment

g2flyer Dec 18, 2020

Choose a reason for hiding this comment

g2flyer Dec 18, 2020

Choose a reason for hiding this comment

bvavala Dec 18, 2020

Choose a reason for hiding this comment

g2flyer Dec 18, 2020

Choose a reason for hiding this comment

g2flyer Dec 18, 2020

Choose a reason for hiding this comment

bvavala Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

g2flyer commented Dec 18, 2020

bvavala commented Dec 18, 2020

g2flyer commented Dec 18, 2020

g2flyer left a comment

Choose a reason for hiding this comment

bvavala commented Dec 19, 2020

g2flyer commented Dec 19, 2020

bvavala commented Dec 19, 2020 • edited Loading

g2flyer commented Dec 19, 2020

bvavala commented Dec 18, 2020 •

edited

Loading

bvavala Dec 18, 2020 •

edited

Loading

bvavala commented Dec 19, 2020 •

edited

Loading