Extend FPC Shim support #637

mbrandenburger · 2021-11-08T17:44:48Z

add get_signed_proposal
add get_tx_id
add get_channel_id
add get_creator

Signed-off-by: Marcus Brandenburger bur@zurich.ibm.com

mbrandenburger · 2021-11-09T14:18:31Z

TBD what to do with the transient data?

mbrandenburger · 2021-11-10T08:23:59Z

TBD what to do with the transient data?

transient data support will be separate PR.

bvavala

Two parts: 1) shim extensions and 2) shim internals.

Overall, the shim extensions look good. Those can provide the chaincode with useful information which was either previously attested, or extracted from the signed proposal.

Missing: comments and documentation regarding the channel id and, particularly, the creator. The creator is not (and cannot be) validated against the ledger/MSP -- the enclave does not have trustworthy data to do so.

The shim internals need some more work.
First, some data is extracted from the signed proposal, but the signed-proposal signature is not checked. Although documented, this could open to unintended consequences.
Second, the channel id and creator are extracted from the proposal, but the former is already passed to (and attested by) the enclave at init time, while the latter already has a shim API for the name. In both cases, consistency is not checked. Again, this could open to unintended consequences.

bvavala · 2021-11-15T19:23:09Z

ecc_enclave/enclave/shim.cpp

+    tx_id = ctx->tx_id;
+}
+
+void get_creator(ByteArray& creator, shim_ctx_ptr_t ctx)


There is already a get_creator_name API above.
I believe these two functions return different yet related things. Is there any check for consistency?

I changed the implementation of get_creator_name to extract the dn directly from the creator cert provided with the signed proposal instead of performing the ocall.

bvavala · 2021-11-15T20:15:02Z

ecc_enclave/enclave/enclave.cpp

+        COND2LOGERR(!b, PB_GET_ERROR(&istream));
+
+        ctx.tx_id = std::string(channel_header.tx_id);
+        ctx.channel_id = std::string(channel_header.channel_id);


The channel id is already available in the cc_params blob that is passed at init time, and attested!
Here there is no consistency check with that value -- they must be the same.

right! added

bvavala · 2021-11-15T20:21:34Z

ecc_enclave/enclave/enclave.cpp

+        ctx.creator = ByteArray(signature_header.creator->bytes,
+            signature_header.creator->bytes + signature_header.creator->size);
+
+        // TODO check signed proposal signature


This missing signature check is a major missing part, giving that this code accepts input data from the signed proposal.

signature check added

bvavala · 2021-11-15T20:30:08Z

ecc_enclave/enclave/enclave.cpp

+        b = pb_decode(&istream, common_SignatureHeader_fields, &signature_header);
+        COND2LOGERR(!b, PB_GET_ERROR(&istream));
+
+        ctx.creator = ByteArray(signature_header.creator->bytes,


Here creator is not checked for consistency with the get_creator_name -- see also other comment.

right - I figured out how to extract creator_name directly from the signed proposal, so I jusggest just to remove the ocall. WIP

- add get_signed_proposal - add get_tx_id - add get_channel_id - add get_transient_data Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

mbrandenburger · 2021-12-01T14:20:38Z

I just experienced that this code is not compatible with Idemix.

With current FPC, Idemix is working as long as you not call get_creator_name. The problem with Idemix is that the serialized identity does not contain a x509 cert (in pem format), instead it contains a serialized idemix identity (as protobuf msg). As said, this is only a problem with get_creator_name, where we try to extract the cert subject.

That is, the code in this PR fails when it tries to parse the x509 as it is actually an idemix identity.

mbrandenburger · 2021-12-01T14:52:17Z

That is, the code in this PR fails when it tries to parse the x509 as it is actually an idemix identity.

Maybe not an issue for now! I suggest to tackel this in a follow-up PR.

mbrandenburger · 2021-12-06T09:22:16Z

@bvavala are you still looking at this?

bvavala · 2021-12-07T16:12:14Z

comments addressed - not tested in hw mode

mbrandenburger changed the title ~~WIP Extend FPC Shim support~~ Extend FPC Shim support Nov 10, 2021

mbrandenburger marked this pull request as ready for review November 10, 2021 08:27

mbrandenburger requested a review from a team as a code owner November 10, 2021 08:27

mbrandenburger requested a review from bvavala November 10, 2021 08:27

mbrandenburger added comp/shim This issue is related to the FPC shim exposed to FPC Chaincode feature labels Nov 10, 2021

bvavala requested changes Nov 15, 2021

View reviewed changes

mbrandenburger force-pushed the txid-support branch from 01d3acd to af2a914 Compare November 29, 2021 17:24

mbrandenburger added 7 commits November 30, 2021 16:07

Extend FPC Shim support

6fddfe7

- add get_signed_proposal - add get_tx_id - add get_channel_id - add get_transient_data Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

fixup! Extend FPC Shim support

f1b093e

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

fixup! Extend FPC Shim support

c998c4a

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

fixup! Extend FPC Shim support

0222cb3

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

fixup! Extend FPC Shim support

daa3882

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

fixup! Extend FPC Shim support

81d0a80

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

fixup! Extend FPC Shim support

d25bf2b

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

mbrandenburger force-pushed the txid-support branch from af2a914 to d25bf2b Compare November 30, 2021 15:07

mbrandenburger requested a review from bvavala November 30, 2021 15:09

fixup! Extend FPC Shim support

95ed7a4

Signed-off-by: Marcus Brandenburger <bur@zurich.ibm.com>

mbrandenburger closed this Dec 1, 2021

mbrandenburger reopened this Dec 1, 2021

bvavala merged commit a949ca9 into hyperledger:main Dec 7, 2021

mbrandenburger mentioned this pull request Dec 8, 2021

Add stress test for many invocations and fix memory leak #641

Merged

mbrandenburger deleted the txid-support branch December 9, 2021 08:54

mbrandenburger mentioned this pull request Jan 28, 2022

Secure get_creator implementation #357

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend FPC Shim support #637

Extend FPC Shim support #637

mbrandenburger commented Nov 8, 2021 •

edited

mbrandenburger commented Nov 9, 2021

mbrandenburger commented Nov 10, 2021

bvavala left a comment •

edited

bvavala Nov 15, 2021

mbrandenburger Nov 30, 2021

bvavala Nov 15, 2021

mbrandenburger Nov 29, 2021

bvavala Nov 15, 2021

mbrandenburger Nov 29, 2021

bvavala Nov 15, 2021

mbrandenburger Nov 29, 2021

mbrandenburger commented Dec 1, 2021

mbrandenburger commented Dec 1, 2021 •

edited

mbrandenburger commented Dec 6, 2021

bvavala commented Dec 7, 2021

Extend FPC Shim support #637

Extend FPC Shim support #637

Conversation

mbrandenburger commented Nov 8, 2021 • edited

mbrandenburger commented Nov 9, 2021

mbrandenburger commented Nov 10, 2021

bvavala left a comment • edited

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

mbrandenburger commented Dec 1, 2021

mbrandenburger commented Dec 1, 2021 • edited

mbrandenburger commented Dec 6, 2021

bvavala commented Dec 7, 2021

mbrandenburger commented Nov 8, 2021 •

edited

bvavala left a comment •

edited

mbrandenburger commented Dec 1, 2021 •

edited