shim support for channelid and mspid

[bvavala]

This PR enables the chaincode enclave to retrieve (untrusted) values related to channel id and msp id. These will be needed at enclave creation time for populating the cc parameters structure -- related code will be pushed later.
It partially addresses #404 .

[g2flyer]

Two main objections to this PR:

• if they are untrusted functions (and for the purpose you state, they have to be), they should not be part of shim.h as this is the API for the fpc chaincode and the corresponding implemention is better secure (yes, now they are not, but eventually they will have to be, based on the whole signed proposal)
• if it's only for creation, then i very much think they all should be parameter of the creation ecall and not some additional ocalls (the same reason i wanted them in the protocol flows. This is much easier for the code to understand and also reduces the code size/number of ecall/ocalls. Or otherwise, at least start from the signedproposal which eventually will need to securely implement shim.h

PS: wrote comment on shim.h first before realizing that your intention was different. So some of that comment is probably a bit out of context although still somewhat relevant ...

[g2flyer]

Do we need these functions? get_channel_id actually already exists as commented out function template above but when we last discussed the shim api this (and other functions like TxID, getTxTimestamp, getBinding ..) didn't really seem necessary as MVP. Not that it per-se hurts but if we start adding we should clear what we aim for with extension, e.g., do we want to match the complete set of functionality of go sdk? Also, a bit unfortunate to add should shortly after we published Tech Preview where we claim to have stabilized the API!

Additionally, given that we will have to re-implement these functions to get them securely, I wouldn't add new functions without any need ...

[bvavala]

Do we need these functions?

What we need is the mspid and channelid parameters.
There are at least two ways to achieve this: inject them in the enclave, or (what this PR implements) retrieve them from Fabric and (not yet implemented) set them after the parameters are unsealed.
The latter (the implemented one) should be the right way, since we can leverage the Fabric infrastructure.

get_channel_id actually already exists as commented out function template

I didn't see that... removed!

didn't really seem necessary as MVP

Other solution would require setting and passing the needed parameters from somewhere else, thus replicating functionality that Fabric already has. In this way, the problem is solved, and FPC is more compliant with Fabric.

if we start adding we should clear what we aim for with extension

We are not aiming to match the Fabric shim. FPC requires (as far as I remember): mspid, channelid, signedproposal. All these can be taken from Fabric without reinventing the wheel.

a bit unfortunate to add should shortly after we published Tech Preview where we claim to have stabilized the API!

If you mean forwards-compatible stability, then I think we should specify that that's not our objective now. In the sense that, if we find good/convenient ways to be more compatible with Fabric, we are going to add the missing APIs.
If you mean backwards-compatible stability, then this PR maintains the stability since it is only adding code/APIs.

Additionally, given that we will have to re-implement these functions to get them securely

I have modified the implementation to take this into consideration. For both mspid and channelid, the idea is to set-once then always get the value, thereby restricting malicious actions from the peer. In particular, on first get, the value is set directly from the Fabric shim. However, when we implement seal/unseal of parameters, then the expectation is to set the values directly (without calls to the Fabric shim).

The values are still unverified. This matches both the current master and the design. In fact, the verification is performed later by the enclave registry. So, the set-once will simply maintain the status quo. If the parameters are good, then the enclave can be registered and used, otherwise it cannot.

[g2flyer]

What we need is the mspid and channelid parameters.
There are at least two ways to achieve this: inject them in the enclave, or (what this PR implements) retrieve them from Fabric and (not yet implemented) set them after the parameters are unsealed.
The latter (the implemented one) should be the right way, since we can leverage the Fabric infrastructure.

I don't see your argument about "leveraging the fabric infrastructure". This infrastructure exists only on the untrusted side. So you can extract them there are at time of triggering the enclave initialization and then pass them in a single ECALL, no need for separate additional OCALLS? This is the same point i think also Marcus is making (and we had made in the past)

[g2flyer]

get_channel_id actually already exists as commented out function template

I didn't see that... removed!

Given that you want to use 1-1 call from enclave to go sdk calls for these functions, i would not think it a good idea add these functions with unvalidated return values in shim.h. Shim.h is the API for the chaincode and should only provide secure implementations. I.e., i would leave shim.h as is and provide a separate internal header file for such functions. PS: Mea culpa, i didn't see your comments on how you envision the securing of the implementation. See below "Ok, i see now where you are heading. ..." comment for some follow-up on that ...

^1: As mentioned as a stand-alone comment: while i like small commits, in this particular case i think it would be much better to decide on the ECALL/OCALL interface of the registration flows. small PRs are good if the bigger sw architecture is clear, but in this case it isn't 100% clear to me and then it's difficult to review ...

[g2flyer]

what for is this msp_id? The creators? If so, why do we need a separate function to get_creator_name which also returns that? If something else, what is it?

[mbrandenburger]

it is the peer MSP. see this https://github.com/hyperledger/fabric-chaincode-go/blob/master/shim/stub.go#L138

[mbrandenburger]

I have the feeling that, both, the peer MSP id and the channel id should be initially injected into the enclave when it is starting and stored in the enclave locally. That also would also mean that we don't need to implement these ocalls and give a peer a chance to inject "wrong" data for whatever reason.
A chaincode calling get_msp_id and get_channel_id would always receive the values that are initially set during enclave creating.

As the channel id and the msp id won't change during the lifetime of the enclave, we save some lines of code and limit also the enclave interface.

[mbrandenburger]

I think we had the in the previous version of the createEnclave see https://github.com/hyperledger-labs/fabric-private-chaincode/blob/new_design/docs/fpc-registration.puml#L105

[bvavala]

what for is this msp_id?

Marcus answered for me.

That also would also mean that we don't need to implement these ocalls and give a peer a chance to inject "wrong" data for whatever reason

Given that this is a concern, I have modified the implementation for setting these values once: either at the beginning from the Fabric shim, or from the sealed parameters.
This matches what we currently have, since the parameters are verified only later by the enclave registry, and then we keep them in sealed storage (to be implemented).

the peer MSP id and the channel id should be initially injected into the enclave

That's a possibility, which I wanted to avoid because we should replicate these values somewhere else, instead of leveraging that Fabric already provides.

A chaincode calling get_msp_id and get_channel_id would always receive the values that are initially set during enclave creating.

I have modified the code to ensure exactly this.

I think we had the in the previous version of the createEnclave see https://github.com/hyperledger-labs/fabric-private-chaincode/blob/new_design/docs/fpc-registration.puml#L105

You're right... after the changes that somebody applied to https://github.com/hyperledger-labs/fabric-private-chaincode/blob/bd95b2fb6eb42495b899b47796a3ab19dabae980/docs/design/fabric-v2%2B/fpc-registration.puml#L73 ;)
Anyway, this does not change the protocol specs.

[g2flyer]

That also would also mean that we don't need to implement these ocalls and give a peer a chance to inject "wrong" data for whatever reason

Given that this is a concern, I have modified the implementation for setting these values once: either at the beginning from the Fabric shim, or from the sealed parameters.
This matches what we currently have, since the parameters are verified only later by the enclave registry, and then we keep them in sealed storage (to be implemented).

Ok, i see now where you are heading. But in that case, please add already the CCParam data structure and initialize it already where the enclave (and is keys) are initialized, so the logic is clear how the security works. Put differently they shouldn't be set ssh-pk-style on first use but (directly) at the point the enclave and its keys are initialized. Doing that indirectly via first-use seems a bit confusing.

In current code a natural place would seem to do it in ecall_init in common/enclave/common.cpp although i actually wonder whether sharing the enclave setup with tlcc really makes sense anymore: tlcc really needs neither key nor attestation and putting the whole enclave lifecycle into ecc_enclave would make actually code reading easier? Given all that code anyway is going to change with pdo-crypto and attestation api, maybe that might be the point to retire common/enclave/common.*?

[g2flyer]

• Or otherwise, at least start from the signedproposal which eventually will need to securely implement shim.h

Actually, thinking of it a bit more, i also don't think an ocall to signedproposal makes sense as it should eventually replace the parameters of ecall_cc_invoke. Which raises the bigger question: shouldn't we in first step re-think the whole .edl in light of what we all will have to change? Not that we really want to implement everything immediately, but at least have the design plus corresponding comments in the edl) would be imho better done in a more holistic way rather than incrementally in tiny steps?

[mbrandenburger]

it is the peer MSP. see this https://github.com/hyperledger/fabric-chaincode-go/blob/master/shim/stub.go#L138

[mbrandenburger]

I have the feeling that, both, the peer MSP id and the channel id should be initially injected into the enclave when it is starting and stored in the enclave locally. That also would also mean that we don't need to implement these ocalls and give a peer a chance to inject "wrong" data for whatever reason.
A chaincode calling get_msp_id and get_channel_id would always receive the values that are initially set during enclave creating.

As the channel id and the msp id won't change during the lifetime of the enclave, we save some lines of code and limit also the enclave interface.

[mbrandenburger]

I think we had the in the previous version of the createEnclave see https://github.com/hyperledger-labs/fabric-private-chaincode/blob/new_design/docs/fpc-registration.puml#L105

[mbrandenburger]

an alternative to implement each of these shim methods as ocall, we could also implement ocall_fetch_cc_params which is called only if the cc_params are not already loaded and stored in the enclave as they are "static" values anyway.
Implementing fetch_cc_params would also better align with the current design described in the UMLs https://github.com/hyperledger-labs/fabric-private-chaincode/blob/concept-release-2.0/docs/design/fabric-v2%2B/fpc-registration.puml#L65

[mbrandenburger]

However, my personal preference is to include this data as parameter in the createEnclave call and keep inside the enclave as non-mutual data.

[g2flyer]

A few additional thoughts which got triggered by a bike ride on Saturday (and before the heat-wave, so hopefully not based on heat-induced hallucinations :-)

• adding GetMSPID/get_mspid to the shim would add a source of non-determinism to chaincodes which is a problem for us, in particular for state encryption once we go beyond designated peer (see Deterministic (de-randomized) state encryption & randomness #183). Now we could punt on the programmer to make sure their code is deterministic (or face potentially confidentiality failures) but in that case GetMSPID it is not clear what an app-writer could actually do with GetMSP? Given that the do not have any use-case which requires that function in a chaincode and there or other "standard" go fabric shim functions which we do not implement, i suggest we do not add it yet to the shim. Adding when required is easier than removing and not having it might make our lives easier when going to single peer endorsement.
  A GetChannelID/get_channel_id (with ercc/tlcc-validated return value) should not have that problem. Additionally, that seems to have more use-cases, e.g., to bootstrap application-level cross-cc/cross-ledger transactions and so probably seems a good addition to shim.h.

• regarding ocall vs ecall, i was wondering also how will we handle sealed state? The easiest approach for designated peer would just return it as response of create_enclave ecall. That said, once we go to key distribution it gets a bit more complicated. E.g., it raises the question whether we have one or two sealed blobs (my feeling is that probably two separate blobs for enclave and chaincode secrets is cleaner as there is no update involved but just two write-once blobs ..). So maybe from an abstraction perspective it might be cleaner to have a sealed state module which transparently via ocalls retrieves/stores state? Should also make it easier to add it?

[bvavala]

• adding GetMSPID/get_mspid to the shim would add a source of non-determinism to chaincodes

First, this comment is about Fabric, rather than FPC.
Second, I disagree. Both for chaincode and FPC-chaincode, as clients (should) know the code, they can determine the circumstances where that API is actually called. Some additional info are here.

Now we could punt on the programmer to make sure their code is deterministic (or face potentially confidentiality failures) but in that case GetMSPID it is not clear what an app-writer could actually do with GetMSP?

We can delegate to the programmer anyway.
Even SGX delegates to a programmer to make sure that the enclave does not write confidential data outside of its range (assuming that's a requirement). Hence, it does not prevent to implement an app that voluntarily leaks data.
Similarly for the chaincode, it is easy to implement one that uses getmspid in a such a way that no transaction can ever be committed. That chaincode is useless, but this does not prove that the API is useless too. Also, it's not a question for FPC, it's for Fabric.

Given that the do not have any use-case which requires that function in a chaincode and there or other "standard" go fabric shim functions which we do not implement, i suggest we do not add it yet to the shim.

This is not helpful.
Fabric enables a chaincode to access the local mspid, and we have identified no issue whatsoever to support it in FPC.
Hence, there is no reason to restrict the domain of possible applications by not implementing it.

Adding when required is easier than removing

I disagree. Both adding it (i.e., this PR) and removing it (i.e., essentially the negative of this PR) involve few lines of code.

and not having it might make our lives easier when going to single peer endorsement.

This is not clear.

A GetChannelID/get_channel_id (with ercc/tlcc-validated return value) should not have that problem. Additionally, that seems to have more use-cases, e.g., to bootstrap application-level cross-cc/cross-ledger transactions and so probably seems a good addition to shim.h.

Again, whether it has more (or less) use cases, it is not up to us to determine.
It's an API of the Fabric shim. Even in this case we have identified no issues whatsoever. Hence, there is no reason to restrict the domain of possible applications by not implementing it.

• regarding ocall vs ecall, i was wondering also how will we handle sealed state? The easiest approach for designated peer would just return it as response of create_enclave ecall.

I would have said yes months ago, but I changed my mind after looking at the external launcher.
A tentative approach I am thinking of is: ocall at create, to save the sealed blob; then ecall, immediately at chaincode startup by external launcher, to inject the sealed blob. So the seal is in the invoke path; while the unseal is not.
There are multiple reasons:

1. it is not useful for the admin to get the sealed blob; besides, it should be sent back to the platform when necessary
2. after createenclave, the enclave is created, and we only need to give it continuity despite failures, reboots, etc. For instance, assume createenclave completed at Peer1; if for any reason all chaincodes/Peer1 have to restart, the external launcher can inject the sealed blob on run; most importantly, the next chaincode invocation will work seamlessly.

That said, once we go to key distribution it gets a bit more complicated. E.g., it raises the question whether we have one or two sealed blobs (my feeling is that probably two separate blobs for enclave and chaincode secrets is cleaner as there is no update involved but just two write-once blobs ..). So maybe from an abstraction perspective it might be cleaner to have a sealed state module which transparently via ocalls retrieves/stores state? Should also make it easier to add it?

This aspect can be clearly addressed together with #275 .

[g2flyer]

• adding GetMSPID/get_mspid to the shim would add a source of non-determinism to chaincodes

First, this comment is about Fabric, rather than FPC.

Not really, see below.

Second, I disagree. Both for chaincode and FPC-chaincode, as clients (should) know the code, they can determine the circumstances where that API is actually called. Some additional info are here.

Hmm, the example above doesn't really apply to us either as we do not support (org-public) private data? Also remember that some of that was added together with the change in Fabric v2 which allows to have different code which does not have to be deterministic in the sense of execution, only deterministic in the sense of read/write-sets and response for a given invocation. So in Fabric v2 you could implement things that an auditor with private approval policies -- it is one of their samples, although they didn't make it explicit that this requires new v2 feature of different code at different peers nor did they implement it, at least when i looked at it a few months back. -- could do additional checks. However, that's something we can't. And if we do the encryption as right now on the fly then even the sequence of writes has to be the same, not only the read/write-set at the end. Lastly, note also that for Fabric any accidental non-determines is not necessarily a problem. As long as it is rare, nothing really breaks. In our case, non-determinism could potentially lead to confidentiality failures. That said, in an ideal world of course we develop a system which handles arbitrary non-determinism gracefully (and eventually you might have to do that also handle cases like fault-injections). However, minimizing non-determinism and laying potential traps for CC writers to fall into still seems desirable to me.

Now we could punt on the programmer to make sure their code is deterministic (or face potentially confidentiality failures) but in that case GetMSPID it is not clear what an app-writer could actually do with GetMSP?

We can delegate to the programmer anyway.
Even SGX delegates to a programmer to make sure that the enclave does not write confidential data outside of its range (assuming that's a requirement). Hence, it does not prevent to implement an app that voluntarily leaks data.
Similarly for the chaincode, it is easy to implement one that uses getmspid in a such a way that no transaction can ever be committed. That chaincode is useless, but this does not prove that the API is useless too. Also, it's not a question for FPC, it's for Fabric.

Given that the do not have any use-case which requires that function in a chaincode and there or other "standard" go fabric shim functions which we do not implement, i suggest we do not add it yet to the shim.

This is not helpful.
Fabric enables a chaincode to access the local mspid, and we have identified no issue whatsoever to support it in FPC.
Hence, there is no reason to restrict the domain of possible applications by not implementing it.

Hmm, there are quite a few other functions which we also do not implement? Are you saying we all have to add them now?

Adding when required is easier than removing

I disagree. Both adding it (i.e., this PR) and removing it (i.e., essentially the negative of this PR) involve few lines of code.

It's not about removing the code, it's removing/obsoleting a published api which i see as the potential issue. (the actual effort to remove (or add) the code to shim.h and exporting it in the library should be minimal?) I guess you heard Matt ranting about some fabric legacy APIs which are hard to change/get rid off. (I will admit, though, that we do not have the same level of legacy commitments as Fabric and won't have it soon. Although given that we explictly link to shim.h in the RFC as the interface specification, we would commit to it more bindly when we add it to master. Thinking of which: we probably also shouldn't reference master in the RFC but some specific version, e.g., right now the tech-preview!)

and not having it might make our lives easier when going to single peer endorsement.

This is not clear.

A GetChannelID/get_channel_id (with ercc/tlcc-validated return value) should not have that problem. Additionally, that seems to have more use-cases, e.g., to bootstrap application-level cross-cc/cross-ledger transactions and so probably seems a good addition to shim.h.

Again, whether it has more (or less) use cases, it is not up to us to determine.
It's an API of the Fabric shim. Even in this case we have identified no issues whatsoever. Hence, there is no reason to restrict the domain of possible applications by not implementing it.

As mentioned above, if you follow your reasoning to the conclusion we have to implement all function? We already do not implement features from Fabric, certainly not in MVP, so adding or not adding it doesn't really change anything fundamentally from the perspective of allowing or not allowing all apps fabric does? And given that we can't implement everything right now, choosing exactly the functionality we know we need for concrete use-cases (which i think is true right now: all our shim functions are required by our auction demo) seems to match Occam's Razor?

• regarding ocall vs ecall, i was wondering also how will we handle sealed state? The easiest approach for designated peer would just return it as response of create_enclave ecall.

I would have said yes months ago, but I changed my mind after looking at the external launcher.
A tentative approach I am thinking of is: ocall at create, to save the sealed blob; then ecall, immediately at chaincode startup by external launcher, to inject the sealed blob. So the seal is in the invoke path; while the unseal is not.
There are multiple reasons:

1. it is not useful for the admin to get the sealed blob; besides, it should be sent back to the platform when necessary

Definitely agree. But just to be clear, above i've talked about the create_enclave ecall not the protocol message which flows back to the client. So the ecall could return the protocol message and additionally the response message (similar to the input parameters also necessarily have to be (only) the initial protocol message?

2. after createenclave, the enclave is created, and we only need to give it continuity despite failures, reboots, etc. For instance, assume createenclave completed at Peer1; if for any reason all chaincodes/Peer1 have to restart, the external launcher can inject the sealed blob on run; most importantly, the next chaincode invocation will work seamlessly.

yep yep, definitely agree, sealed storage has to be handled local to the peer's external build/ecc itself. I think we are on the same page here.

That said, once we go to key distribution it gets a bit more complicated. E.g., it raises the question whether we have one or two sealed blobs (my feeling is that probably two separate blobs for enclave and chaincode secrets is cleaner as there is no update involved but just two write-once blobs ..). So maybe from an abstraction perspective it might be cleaner to have a sealed state module which transparently via ocalls retrieves/stores state? Should also make it easier to add it?

This aspect can be clearly addressed together with #275 .

You mean #274 (assuming we do the easier endorsement policy beyond designated peer first?) But that said, i think there is value already now to have a sealed-state library which handles the sealed state and any related separate edl and ecalls/ocalls. it automatically provides looser coupling via a clean abstraction, smaller components and implementation parts which can be concurrently worked on. i think will also make it easier to add after an initial version without sealed state. Whether we store it as one or two blobs is of course immaterial (and should also be easy to change if we have this sealed storage library/module)

[bvavala]

Closing as outdated.
Code might be useful as FPC expands support for the Fabric shim.