-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Api design #420
Api design #420
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much for a first round of comments. Sorry adding quite a few ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely a step forward.
As mentioned, the document should however avoid taking final decision on implementations such as internal data structures.
Also, many things (most related to key distribution) could be marked as out of scope, with the intent of dealing with them after MVP.
docs/interfaces.md
Outdated
[out] uint8_t *metadata); | ||
|
||
// verify msp identities | ||
public bool validate_msp_id( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Given the request routing, I am assuming that this is originally called by ecc_enclave, but I cannot figure out where this is needed. If this is meant to validate the mspid in cc_params, then it is not needed. The reason is that the parameter is checked by ERCC on registration.
- What is actually required is a mechanism to validate a transaction proposal (or perhaps just it creator).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this function to validate_identity, which can be used to implement get_creator and check if the creator is a valid identity with respect to the consortium defined in this channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed it to validate_identity, which can be used to implement getCreator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it should be enough for now.
However, this document (part 2) corroborates my second point: we need a mechanism for checking the proposal (including its creator, and thus this API).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we definitely need to validate the signed proposal. Using the proposed validate_identity
we should be able to conclude that the creator -- as expressed by serializedIdentity
which contains the msp-id and the x509 cert with pk, name and role [might help, though, explicitly mention that in the comment around validate_identity
-- is a valid party grounded in the specified MSP-id root-of-trust and the rest of the signature verification of the proposal we can do locally to ecc based on the resulted validated public key? Is there any other step in verifying the signedproposal in the enclave we need tlcc
assistance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(see the link I have attached)
The endorsing peer verifies also that (i) the proposal has not been submitted in the past, (ii) the submitter respects the channel policy. In theory, TLCC should do this. In practice, we can rely on the validators for (i), and delegate (ii, or more precisely, any authentication/authorization) to the enclave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i see now what you mean (didn't initially get what you meant by "part 2"). Good points.
Regarding the replay protection, at first sight it certainly seems mainly a waste of resource you save by checking, as during submission of a replayed transaction it should fail due to rw-set conflicts? This assumes, though, it is state-changing, maybe there could be issues of transactions which might be non-state-changing dependent on state and somebody could try to replay such a transaction at a later time when it would be state-changing? So integrity-wise there might or might not be issue? For FPC maybe confidentiality could add additional twists: As an original tx/query submitter can always recreate a new query, so from overt channel (results to querier) perspective a replay shouldn't help? If there would be covert channel, then a resubmit could potentially help in throttling attacks but that seems rather esoteric? So i would somewhat agree that we can punt on replay protection to validators/rw-conflict (although i have not a good feeling yet how much this this-tx-does-not-change-state-now-but-could-potentially-later scenario is an issue or not.)
regarding authorization, though, it becomes a bit more sensitive as we are talk also about confidentiality (for queries), not only integrity. So maybe the validated_identity
should maybe return which permissions the client has and ecc should immediately abort if client doesn't have read
privs and abort on first write-attempt if it doesn't have write
permissions? Of course, we could also punt on that but would have to document that reader/writer polices could not be trusted to provide confidentiality of information protected fpc cc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
during submission of a replayed transaction it should fail due to rw-set conflicts?
Probably yes, in a concurrent execution: send proposal P1 and replay it immediately; you get two separate endorsements/transactions where the updated keys likely conflict with each other.
Maybe not, in a serialized execution: send proposal P1 (e.g., incrementCounter), commit transaction; replay P1, commit transaction; here the counter will be incremented by two with a single proposal.
The check that Fabric does should be here. It's based a transaction id value computed over the proposal and stored (at some point) on the ledger.
So maybe the validated_identity should maybe return which permissions the client has and ecc should immediately abort if client doesn't have read privs and abort on first write-attempt if it doesn't have write permissions?
In theory, yes.
However, this is all Fabric logic that TLCC should implement. Following Yacov's recommendation, we can simplify.
Of course, we could also punt on that but would have to document that reader/writer polices could not be trusted to provide confidentiality of information protected fpc cc?
In practice, this is (part of) the solution. The other part (in my answer above) is to tell a developer how to leverage the fpc chaincode to implement an equivalent mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
during submission of a replayed transaction it should fail due to rw-set conflicts?
[..]
Maybe not, in a serialized execution: send proposal P1 (e.g., incrementCounter), commit transaction; replay P1, commit transaction; here the counter will be incremented by two with a single proposal.
Oops, of course, this is a much simpler examples where replay works (badly) than my contrieved sometimes-state-changes example. (Not sure what i was thinking in the morning, i guess i was still have up in the mountains ... :-)
The check that Fabric does should be here. It's based a transaction id value computed over the proposal and stored (at some point) on the ledger.
This code seems to be executed by the endorsers. You mentioned above also "rely on the validators for (i)": Do the validators automatically also cross-check this? If so, i think we can punt on them as good peers would ignore replays from a integrity perspective and from a confidentiality perspective there seems only esoteric direct leakage (and not indirect via integrity corruption, which would be caught by validators?) If standard validators would not do it, this (contrary to authorizaton) seems a bit a tough to say we do not support replay protetion and punt it to the app developer? If we can live with some false-positives for replays, we could handle replay protection with a bloom-filter. Actually, for channel & ercc transactions we cannot punt to app developers at all? So we more or less have something inside tlcc? We currently have these three opaque kvs_t files as part of tlcc internal state but seems we might want to be more explicitl. And of course this ties now this to (some) resolutions of #402 ... :-(
So maybe the validated_identity should maybe return which permissions the client has and ecc should immediately abort if client doesn't have read privs and abort on first write-attempt if it doesn't have write permissions?
In theory, yes.
However, this is all Fabric logic that TLCC should implement. Following Yacov's recommendation, we can simplify.
That's what i meant with punt below. But i agree that besides explicit telling you cannot rely on ledger-specified authorization rules (which we would have to do at a minimum) ...
Of course, we could also punt on that but would have to document that reader/writer polices could not be trusted to provide confidentiality of information protected fpc cc?
In practice, this is (part of) the solution. The other part (in my answer above) is to tell a developer how to leverage the fpc chaincode to implement an equivalent mechanism.
... pointing out how to do it themselves is even better. Certainly, per-app authorization policies should be easy to handle via get_creator_name
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments: most, though, are responses to ongoing discussions, so you will have to look in the history (and potentially unfolded "hidden conversation") to get them all. All my old comments which are addressed, i've resolved, so only somewhat-open should show up (but as mentioned, potentially buried behind hidden conversations ...)
arrgh, wrong radio box again: this was not supposed to be approved. Not all of my comments are crucial but a number of them should hopefully, when addressed,help clarity, and i think the channel_id vs channel_hash it might be good to have some quick discussion on a call? |
d3b1762
to
448ab73
Compare
Below a dump regarding the tlcc-ecc session. Sorry, nothing integrated into text from this PR. Hopefully, it is at least parseable .. :-) Table of ContentsECC - TLCC Tunnel Design Outlinepublic API
control flow of implementation
sgx_dh_* background
|
FYI: to make sure we do not forget updating the UML diagrams as mentioned in comments here, i've created a corresponding issue #432 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- add current component APIs - component state representation Signed-off-by: bur <bur@zurich.ibm.com>
cb00a8d
to
247b172
Compare
Signed-off-by: bur <bur@zurich.ibm.com>
247b172
to
c66177c
Compare
What this PR does / why we need it:
This PR proposes the FPC component API and state design based on the most recent UML diagrams. The goal is to stablize the APIs and internal state before current code base can be refactored to align with the FPC design as specified by the UML diagrams.
Which issue(s) this PR fixes:
Addresses #410 but excludes chaincode enclave / tlcc binding as this is covered by #434
Special notes for your reviewer:
Does this PR introduce a user-facing changes and/or breaks backward compatability?: