improved document handling and signature storing #27

sschulz-t · 2021-02-22T15:16:19Z

This implements #26
See https://github.com/GSMA-CPAS/BWRP/wiki/Off-chain-document-exchange for more info

Note: this breaks the api and requires the updated chaincode as well.
Please use the following tag for both: packaged_2021_02_21

NOTE2: this breaks the Rest API!
Please have a look at the api doc with regard to /private-documents POST & GET:

https://github.com/GSMA-CPAS/BWRP-blockchain-adapter/blob/ssch-storeandsign/api/doc/Apis/ContractApi.md#uploadprivatedocument

https://github.com/GSMA-CPAS/BWRP-blockchain-adapter/blob/ssch-storeandsign/api/doc/Apis/ContractApi.md#fetchprivatedocumentreferenceids

zkong-gsma · 2021-02-22T17:10:57Z

can i ask some feedback on why is the "chaincode" layer implementing the whole "blockchainRef" object?
especially the "type"

this will become more confusing. as the idea of the "type" is for

Yes, additional Data such as "time" and stuffs can be further "recorded" if required. but the whole idea of the "type"

is so that, for instance we implement an "API" to Read from network. the common-adapter knows how to route to the "correct" network.

smeyerzu

Looks good to me.

@zkong-gsma I personally like the idea that all the adapters correctly implement the blockchainRef object instead of providing their own format which then needs to be modified by the common-adapter.

sschulz-t · 2021-02-23T13:16:08Z

Let me copy the output example where you can see the BlockchainRef from GSMA-CPAS/BWRP-common-adapter#23 to here:

{
  "fromMSP": "ORG1",
  "toMSP": "ORG2",
  "payload": "ZGF0YTEyMzQ=",
  "payloadHash": "4508767822a67b7a051a8fe50250897c38c044ab10d9070d740a05a21deb4499",
  "blockchainRef": {
    "type": "hlf",
    "txID": "0xfe8948c4-ea59-4f21-b3c8-fddd18504e15",
    "timestamp": "2021-02-22T16:21:10+01:00"
  },
  "referenceID": "698c452e861d42a1948aa8c1de33584e34f8b55064d797762c2b71b2eb420c82"
}

The idea of returning the type from within the blockchain adapter and chaincode is that the requesting countepart e.g. the common adapter does not need know which kind of blockchain adapter it talks to (once there are more than one in the future).
The common adapter can query any adapter it can connect to and will get a proper reply and can forward it.
The proposed implementation builds the full response on chaincode level so that the blockchain adapter does not need fiddle around with changing keys and create a new representation. This way it can simply forward the returned dataset.

zkong-gsma · 2021-02-23T13:27:40Z

The idea that the “type” should be configured on the Common-adapter layer is that, it needs to know the “routing” to the correct network. Especially for instance if we say. “GetSignature” API, which network are we going to “get it from”. For instance, we want to run a parallel “2x” Hyperledger Node or channel. For whatever reason. The “Common-adapter” will need to know which “adapter” to get the signature from. From: Simon Schulz <notifications@github.com> Sent: Tuesday, February 23, 2021 1:16 PM To: GSMA-CPAS/BWRP-blockchain-adapter <BWRP-blockchain-adapter@noreply.github.com> Cc: Zhen Kong <ZKong@gsma.com>; Mention <mention@noreply.github.com> Subject: Re: [GSMA-CPAS/BWRP-blockchain-adapter] improved document handling and signature storing (#27) “This email has been received from an external source – please review before actioning, clicking on links, or opening attachments” Let me copy the output example where you can see the BlockchainRef from GSMA-CPAS/BWRP-common-adapter#23<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FGSMA-CPAS%2FBWRP-common-adapter%2Fissues%2F23&data=04%7C01%7Czkong%40gsma.com%7C499de90c488c4baf6ea808d8d7fd391b%7C72a4ff82fec3469daafbac8276216699%7C0%7C0%7C637496829875140556%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4lBDUtCWuSiKFIZ3%2F5rRwjjOBN4FnC8k1rEtkdYuip0%3D&reserved=0> to here: { "fromMSP": "ORG1", "toMSP": "ORG2", "payload": "ZGF0YTEyMzQ=", "payloadHash": "4508767822a67b7a051a8fe50250897c38c044ab10d9070d740a05a21deb4499", "blockchainRef": { "type": "hlf", "txID": "0xfe8948c4-ea59-4f21-b3c8-fddd18504e15", "timestamp": "2021-02-22T16:21:10+01:00" }, "referenceID": "698c452e861d42a1948aa8c1de33584e34f8b55064d797762c2b71b2eb420c82" } The idea of returning the type from within the blockchain adapter and chaincode is that the requesting countepart e.g. the common adapter does not need know which kind of blockchain adapter it talks to (once there are more than one in the future). The common adapter can query any adapter it can connect to and will get a proper reply and can forward it. The proposed implementation builds the full response on chaincode level so that the blockchain adapter does not need fiddle around with changing keys and create a new representation. This way it can simply forward the returned dataset. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FGSMA-CPAS%2FBWRP-blockchain-adapter%2Fpull%2F27%23issuecomment-784195147&data=04%7C01%7Czkong%40gsma.com%7C499de90c488c4baf6ea808d8d7fd391b%7C72a4ff82fec3469daafbac8276216699%7C0%7C0%7C637496829875140556%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SloqcePZYz2cri%2B31CAU6a4%2FTEk7u4wNr5uhsk%2FcK3Y%3D&reserved=0>, or unsubscribe<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAEJMGRADZA266S2IXK47WBTTAOTCRANCNFSM4YAWLABA&data=04%7C01%7Czkong%40gsma.com%7C499de90c488c4baf6ea808d8d7fd391b%7C72a4ff82fec3469daafbac8276216699%7C0%7C0%7C637496829875150554%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WQsKWWWHaojOtJMvIkmozZ8tcGqZ9TxPGeJmFlU2RPc%3D&reserved=0>. .

zkong-gsma · 2021-02-24T10:34:49Z

Please provide a "branch" where We can start "common-adapter" integration please.

sschulz-t · 2021-02-24T10:36:28Z

Please provide a "branch" where We can start "common-adapter" integration please.

The branch is already there: ssch-storeandsign
The same branch name is used in chaincode.

zkong-gsma · 2021-02-24T10:41:45Z

Please provide a "branch" where We can start "common-adapter" integration please.

The branch is already there: ssch-storeandsign
The same branch name is used in chaincode.

My point is, If i develop and check/test against a "ssch-storeandsign"(a temporary branch) which will then be deleted later after you have perform a "PR", or at the same time a different "branch" in parallel is in the Pipe to merge into a "master"(final) branch. does that mean i have to again confirm/made changes if somehow it breaks functionality with the "master" branch?

sschulz-t · 2021-02-24T10:58:45Z

My point is, If i develop and check/test against a "ssch-storeandsign"(a temporary branch) which will then be deleted later after you have perform a "PR", or at the same time a different "branch" in parallel is in the Pipe to merge into a "master"(final) branch. does that mean i have to again confirm/made changes if somehow it breaks functionality with the "master" branch?

Sounds like you do not need a branch. You want something "stable". So you could pick the commit hash. E.g.
blockchainadapter: 5a493fd
chaincode: ed470df0396da0fb2f3b486762eaaacd23d77e50

zkong-gsma · 2021-02-24T12:05:31Z

BTW, kindly provide information that the "Webhook" subscription names has been updated from

"STORE:DOCUMENTHASH" to "STORE:PAYLOADLINK"

took me sometime to figure it out.

zkong-gsma · 2021-02-24T12:27:49Z

It seems there is a BUG.

If i subscribe using "STORE:DOCUMENTHASH" it says "error"

# curl -H"Content-Type: application/json" -X POST http://localhost:8081/webhooks/subscribe -d'{"eventName": "STORE:DOCUMENTHASH", "callbackUrl": "http://common-adapter-tmus:3000/api/v1/contracts/event/"}'

{"message":"Error: unknown eventName STORE:DOCUMENTHASH specified"}

If i subscribe using "STORE:PAYLOADLINK" it says another"error"

# curl -H"Content-Type: application/json" -X POST http://localhost:8081/webhooks/subscribe -d'{"eventName": "STORE:PAYLOADLINK", "callbackUrl": "http://common-adapter-tmus:3000/api/v1/contracts/event/"}'

{"message":"request.body.eventName should be equal to one of the allowed values: STORE:SIGNATURE, STORE:DOCUMENTHASH","errors":[{"path":".body.eventName","message":"should be equal to one of the allowed values: STORE:SIGNATURE, STORE:DOCUMENTHASH","errorCode":"enum.openapi.validation"}]}

sschulz-t · 2021-02-24T12:35:35Z

Thanks. good find! I just updated this PR with a fix. You can use commit 29c85d6 for testing.

zkong-gsma · 2021-02-25T12:18:24Z

Can i kindly ask the Reason of this change?

it has broke everything on common-adapter.

Original,
https://github.com/GSMA-CPAS/BWRP-chaincode/blob/master/hybrid/offchain.go

and the new
https://github.com/GSMA-CPAS/BWRP-chaincode/blob/ssch-storeandsign/hybrid/offchain.go

why are we adding an "additional" ":" to the "storageKey", this fundamentally changes all things.

api/openapi.yaml

zkong-gsma · 2021-02-25T16:45:36Z

Can i kindly ask the Reason of this change?

it has broke everything on common-adapter.

Original,
https://github.com/GSMA-CPAS/BWRP-chaincode/blob/master/hybrid/offchain.go

and the new
https://github.com/GSMA-CPAS/BWRP-chaincode/blob/ssch-storeandsign/hybrid/offchain.go

why are we adding an "additional" ":" to the "storageKey", this fundamentally changes all things.

Can we have a feedback please. This is a big "change" that if we change it, we cannot go back.

Currently it has bottle neck us from releasing.

informartin · 2021-02-25T17:02:23Z

Can i kindly ask the Reason of this change?

This change was part of the streamlining process. So far, there was no definition how concatenated strings could be represented. The idea was that having a delimiter permits to display the content of the string in a more transparent manner, i.e., it will be obvious where components of the string have been concatenated.

The change was presented two or three weeks ago as part of the signature exchange process. It was also open for discussion on slack: https://blockchainwho-dxu1553.slack.com/archives/C013UJLJKQA/p1612969039018200?thread_ts=1612969018.017900&cid=C013UJLJKQA

After merging the streamlined process into the wiki, it became part of the implementation. You can find the description here: https://github.com/GSMA-CPAS/BWRP/wiki/Document-Signature-Process

sschulz-t · 2021-02-26T13:08:49Z

Can i kindly ask the Reason of this change?
it has broke everything on common-adapter.

I was wondering why the change in the storage key broke stuff in the common adapter as this should all be abstracted by the blockchain adapter. Is it just the test cases?

And its not ideal to code it first at the block-chain adapter before the impacts are analysed on other layers and discussed – as the interfaces are technical contracts between the layers and any change in those technical contracts has implications

First of all, this was not implemented at first.
We mentioned that we were analyzing some issues concerning the on-chain transactions some weeks ago.
Then we came up with diagrams and pseudocode with a proposed solution. This was presented on 9th of February.
Then we started to work on a proposed implementation. This implementation is the basis for this PR.

And its not ideal to code it first at the block-chain adapter before the impacts are analysed on other layers and discussed – as the interfaces are technical contracts between the layers and any change in those technical contracts has implications

But that's how a PR works. You propose a change by using code. Usually this happens only based on a code proposal as this is the easiest way to review the changes in detail. For this PR we additionally presented the idea and inner workings beforehand by using diagrams and pseudocode on the meeting on 9th of February and in the wiki.
The resulting proposed implementation is then submitted as a PR. Which can be then analyzed, changed, adapted and finally rejected or accepted.

When you submit a PR to a bigger opensource project it is quite common that your PR is not accepted. Sometimes you have to do changes in order to get it into the master, sometimes you can even fail if e.g. the community does not think this feature is necessary and the code is not used. Then the code is dropped - which happens every day.

I could understand your complains if this would have been a silent push to master without further notice which is not the case.

I would propose that we create a Wiki page where we start a discussion on how to work with PRs in the future.
for example I asked you to disable pushes to master for the chaincode and blockchain adapter repo and to set the PR reviewer count to two. This way no one can push changes to the master and two different people have to review it. I would recommend we do this on all Repos in the future.

zkong-gsma · 2021-02-26T13:41:30Z

I was wondering why the change in the storage key broke stuff in the common adapter as this should all be abstracted by the blockchain adapter. Is it just the test cases?

This is down to the "design" of the blockchain-adapter in the first place.

Your "incoming" Notification is based on the "StorageKey". Especially the "Signature Notification" In the "StorageKey" form, we cannot associate it with any existing "object".

Also as mentioned that a "storageKey" is a one way Hash, that we cannot decode back to "MSP" + ":" + "referenceId" to find out the associated Local Object.

Also, since StorageKey is not return by the "GET /private-documents", when we "store" the object locally, we would have pre-generated and associate the "generated" From/To Storage Keys with the "Object".

So when Signature Event arrives , with only "StorageKeys", we can compare.

Lets argue that even if we do not "pre-generate"/"store" the StorageKeys, but generate on the Fly.
Without knowing now the new "HASH" includes a ":". the Received "Event notification" StorageKeys is generated with an extra ":" and we will never be able to match it locally, unless we "realized" and update the logic accordingly.

Back to the point. The ":" delimiter do not give much useful advantage in the first place. And in the current situation, i don't feel like there is any space for discussion.

I have put forward my reason that the ":" do not add any advantage or serve much purpose, and it seems that because it has been decided and coded, the next decision will then be "lets makes things simple", since its already in place. lets just use it.

First of all, this was not implemented at first.
We mentioned that we were analyzing some issues concerning the on-chain transactions some weeks ago.
Then we came up with diagrams and pseudocode with a proposed solution. This was presented on 9th of February.
Then we started to work on a proposed implementation. This implementation is the basis for this PR.

I welcome that Approach. however all those highlighting "internal" changes. I admit That its my fault that I "did not spot" the difference of SHA256(<MSP> + <referenceID>) vs SHA256(<MSP> + ":" + <referenceID>) as if on a high level the "content" has not changed, (not for instance that we are adding additional "salt" as an example). Also i don't remember there is any mentioned of, by the way we propose to add a "delimeter" to the SHA256() function in general.

What is not proposed/discussing on the 9th are are additional changes to.

Entire "Payload" Response.
Update to “request” payload structure of {“toMSP”:XXX,”data”:XXX} to {“toMSP”:XXX,”payload”:XXX}
Changes of the “EVENT NAME” from "STORE:DOCUMENTHASH" to "STORE:PAYLOADLINK"

Yes, arguably the Update of the "entire" payload response was for a different ticket/issue. Still there is no discussion

I could understand your complains if this would have been a silent push to master without further notice which is not the case.

So, if i have not voiced any of this.
This PR is titled "improved document handling and signature storing", and there are no explicit mentioned of any of these changes, this PR gets Approved.

shouldn't that then qualify as a "silent" push to master?

sschulz-t · 2021-02-26T14:14:49Z

Also, since StorageKey is not return by the "GET /private-documents", when we "store" the object locally, we would have pre-generated and associate the "generated" From/To Storage Keys with the "Object".

Ok we should find a fix for that. I created an issue here: #29
This is blockchain specific and should be handled by the blockchain adapter.

So, if i have not voiced any of this.
This PR is titled "improved document handling and signature storing", and there are no explicit mentioned of any of these changes, this PR gets Approved.
shouldn't that then qualify as a "silent" push to master?

Was it accepted? no. not yet. again, this is a PR.

This PR sums up several cleanup and inconsistency fixes. As mentioned before we did put a lot of effort into the diagrams to have everything clean and consistent (e.g. speaking of payloadHash etc everywhere).
I must admit that this PR would probably be better as multiple small and single PRs so it is easier to review the changes.
The more mature this project gets this will become more and more important and I will try my best. This is why I strongly recommend to enable 2 reviewers per PR for all repos. Lets have a discussion on that here:
GSMA-CPAS/BWRP#33

I have put forward my reason that the ":" do not add any advantage or serve much purpose, and it seems that because it has been decided and coded, the next decision will then be "lets makes things simple", since its already in place. lets just use it.

I see a benefit in having it in there. You do not.
If we keep it, it might help some people or maybe not. If we remove it we gain nothing. So why should we not add it?
But if you insist I can remove it even though I think we loose something. Any other opinions on that?

zkong-gsma · 2021-02-26T14:42:38Z

Ok we should find a fix for that. I created an issue here: #29
This is blockchain specific and should be handled by the blockchain adapter.

We can start discussion some solution there.

Was it accepted? no. not yet. again, this is a PR.

If I had not provided any feedback and just adopt the change on common-adapter, would this then been accepted?
(and if that happens, we would not have discover the new issue above per-say)

This PR sums up several cleanup and inconsistency fixes. As mentioned before we did put a lot of effort into the diagrams to have everything clean and consistent (e.g. speaking of payloadHash etc everywhere).
I must admit that this PR would probably be better as multiple small and single PRs so it is easier to review the changes.
The more mature this project gets this will become more and more important and I will try my best. This is why I strongly recommend to enable 2 reviewers per PR for all repos. Lets have a discussion on that here:
GSMA-CPAS/BWRP#33

This is exactly highlighting the Point i am making. Solution has been "coded" prior to any discussion/suggestion/planning from the wider group. (earlier reference of JUST DO IT)

So i have to look into the source code to just understand what changes has been made. Not a changes that has not been mentioned/agreed that i know where to look straight into.

This is exactly how any why additional unnecessary "debugging" time was spent just to find out what has changed.

I see a benefit in having it in there. You do not.
If we keep it, it might help some people or maybe not. If we remove it we gain nothing. So why should we not add it?
But if you insist I can remove it even though I think we loose something. Any other opinions on that?

These are exactly the "discussion" that we should be having before a "change" to the "code" that now breaks the flow. as your self mention, "if I insist" (per-say) you can revert that. This is exactly the unnecessary "undo" we can avoid if we would have first discussed about the change.

.
.
.

The problem real problem i am highlighting has nothing to do with PR per-say. its more of the fact that a decision of change is made without additional input from the wider group and straight into a PR. and wait to hopefully get "approval".

This is where the problem develops. Where if there is a "change" cannot be "agreed", (because it was never agreed in the first place) There will be a waste of effort to develop the code in the first place and "time" to debate. (it might a MAJOR change. not so much of a "rename")

sschulz-t · 2021-02-26T15:10:52Z

This is exactly the unnecessary "undo" we can avoid if we would have first discussed about the change.

At the risk of repeating myself this is a PR. There is nothing to revert or undo.
This PR was created and you were asked to test it and come back to me if you have questions. It was neither merged/silently pushed or whatever.

In my opinion a PR with a proposed code as a basis for a discussion is way better than just some loosely formulated text presented in a meeting. The diagrams are a bit better, but still, the best basis for detailed discussion is a proposed implementation. Throwing away code for a rework based on a discussion on that is no big deal. Take the code a better/clearer way of describing an idea.
Discussing all the details in normal language will waste more time and can lead to misunderstandings or things get lost.

As said before this PR should have been ideally split into multiple smaller PRs. I am open for suggestions for improvement of the pull request handling.

Maybe we have different understandings on what a PR is?
For me this is a discussion basis with a proposed implementation. It is not fixed/integrated into the master yet and usually I ask others to review and test it (e.g. #22)
Another good example for how I see a PR is this one: GSMA-CPAS/BWRP-chaincode#15
Dhruv proposed some changes, I had a look on it and proposed some modifications. Once everyone was happy it was merged.
I think you use it more like a "merge" collector/button to collect and push changes to the master?
E.g. GSMA-CPAS/BWRP-common-adapter#33

So how do we proceed here? Maybe we can collect some more feedback from the wider group?

sschulz-t · 2021-03-10T11:50:17Z

@zkong-gsma, @csarthou: Any progress on the common adapter integration and testing?
Let me know if you need any help with the changes necessary for the integration of this PR, I would be happy to help you on that.
Please let me know when this can be merged into the master.

zkong-gsma · 2021-03-10T11:52:45Z

we are fine. It would help if you "freeze" the changes. Its hard for us to work toward something that keeps changing.

do you have a "latest" version that we should be testing against. that will not get "Changed" again without any notice?

sschulz-t · 2021-03-10T12:02:10Z

we are fine. It would help if you "freeze" the changes. Its hard for us to work toward something that keeps changing.

do you have a "latest" version that we should be testing against. that will not get "Changed" again without any notice?

You can refer to the following branch that keeps track of the PR state:
https://github.com/GSMA-CPAS/BWRP-blockchain-adapter/tree/ssch-storeandsign

The last change is from 14 days ago which added a bugfix for an issue you reported (see 29c85d6).
This is a PR and l will only add bugfix commits to it for any issues that you or any of us find which needs to be fixed before the merge.

Do you have an estimate for me when the integration is finished, we need to integrate a forecast into out sprint planning.

zkong-gsma · 2021-03-10T12:29:49Z

The last change is from 14 days ago which added a bugfix for an issue you reported (see 29c85d6).
This is a PR and l will only add bugfix commits to it for any issues that you or any of us find which needs to be fixed before the merge.

are you waiting for us to "black box" testing as well? what is stopping this commit of finalizing?

Do you have an estimate for me when the integration is finished, we need to integrate a forecast into out sprint planning.

similar to previous question. What is blocking your planning? you do not have a dependency on common-adapter.

sschulz-t · 2021-03-10T12:34:46Z

So are you okay with merging this to master? If you approve it, I can merge it to master.
I kept the PR open in order to collect your feedback and fix any issues that you find during integration

zkong-gsma · 2021-03-10T12:38:29Z

So are you okay with merging this to master? If you approve it, I can merge it to master.
I kept the PR open in order to collect your feedback and fix any issues that you find during integration

My only feedbacks are the ones already discuss, which are all the changes to the Northbound API Spec and behaviour which are already rejected.

I have no feedback on any "internal" items.

zkong-gsma · 2021-03-12T17:45:25Z

After the discussion from GSMA-CPAS/BWRP-development-setup#19

I just found out some feedback that i think need to be addressed.

The Introduction of "storageLocation" based on

const compositeKeyDefinition string = "owner~type~key~txid"

storageLocation, err := ctx.GetStub().CreateCompositeKey(compositeKeyDefinition, []string{invokingMSPID, storageType, key, txID})

will cause trouble when "revalidating" a Payload..

There are 3 "issues".

1)
When exchanging document "POST private-document"

At no point In Any flow that we are checking the MOST important part of this solution. "The hash" value written onto the ledger (transaction)

all the "HASH" validation are "offline" and its meaning less.

BWRP-blockchain-adapter/server/hyperledger/blockchain_service.js

Lines 247 to 273 in 29c85d6

    
           return self.createReferenceID(network, contract).then( (referenceID) => { 
        
             // EVALUATE store document on fromMSP (local) 
        
             return self.storePrivateDocument(network, contract, fromMSP, partnerMSP, referenceID, payload).then( (storedPayloadHash) => { 
        
               // check hash 
        
               if (storedPayloadHash != payloadHash) { 
        
                 console.log('ERROR: '+fromMSP + ' stored invalid hash: ' + storedPayloadHash + ' != ' + payloadHash); 
        
                 return Promise.reject(ErrorCode.createErrorCode('ERROR_INTERNAL', fromMSP + ' stored invalid hash')); 
        
               } 
        
               // EVALUATE store document on partnerMSP (remote) 
        
               return self.storePrivateDocument(network, contract, partnerMSP, partnerMSP, referenceID, payload).then( (storedPayloadHashPartner) => { 
        
                 // check hash 
        
                 if (storedPayloadHashPartner != payloadHash) { 
        
                   console.log('ERROR: '+partnerMSP + ' stored invalid hash: ' + storedPayloadHashPartner + ' != ' + payloadHash); 
        
                   return Promise.reject(ErrorCode.createErrorCode('ERROR_INTERNAL', partnerMSP + ' stored invalid hash')); 
        
                 } 
        
                 // calculate reference payload Link: 
        
                 return self.createReferencePayloadLink(network, contract, referenceID, payloadHash).then( (link) => { 
        
                   // publish the link on the ledger 
        
                   return self.publishReferencePayloadLink(contract, link.key, link.value).then( (blockchainRef) => { 
        
                     // now its safe to access the document! 
        
                     return self.fetchPrivateDocument(referenceID); 
        
                   }); 
        
                 }); 
        
               }); 
        
             });

The summary of Flow.

HOME blockchain-adapter. pre calculated "expected hash" in temporary memory.
HOME blockchain-adapter send to HOME Chaincode to store to offchain-db. Returns Hash calculated by HOME Chaincode.
(compare the return HASH with "expected hash" This is Home chaincode. We do not expect any foul play).
HOME blockchain-adapter send to REMOTE Chaincode to store to offchain-db. Returns Hash calculated by REMOTE Chaincode.
(compare the return HASH with "expected hash" This is Remote chaincode. Home is sure that "Remote" receive the expected Payload.).
Finally Write the "anonymized" HASH which is hash(referenceId <concat> payloadHash) on to yet another "anonymized" "key" hash(referenceId)

"where" is the problem Here?

HOME" is sure of PARTNER having the exact Same copy of his Data.
But PARTNER is blind. and cannot confirm That the Payload is indeed intended. (there is nothing to verify)

Even by then, When a Event is "triggered", the Common-adapter performs a "GET /private-doument" We are sill not able to verify anything other that what was received via "chaincode" earlier. (the Hash value we have in the Offchain DB are generated locally. so, it will always match.)

So, why are we even "performing" step 4 Which is the KEY proof of the validity of this document. yet we are not checking against it?

So we need the next step (item 2)

2)
WE need a way to "read" the "stored" Proof from the "ledger". As it stands, We only "write" to it. We do not have a way to read it out to perform any checks.

This is also a dependency from Case 1.

3)
A new problem now arise now that the "compositeKey" Location for both Signature and Payload is unified.

For Signature, Its expected that we want to split the "signature" by using composite key for Both MSP.

Hence

https://github.com/GSMA-CPAS/BWRP-chaincode/blob/ed470df0396da0fb2f3b486762eaaacd23d77e50/hybrid/offchain.go#L340-L377

we are selecting using compositeKeys but with "explicit" "MSPS". We expects results from both HOME and PARTNER MSP.

Since now
https://github.com/GSMA-CPAS/BWRP-chaincode/blob/ed470df0396da0fb2f3b486762eaaacd23d77e50/hybrid/offchain.go#L492-L496

Storing the Hash value for a Document includes MSP creator. we will also need to know YET another "key" to be able to "find" the value from the network.

which goes back to the discussion
GSMA-CPAS/BWRP#35

Currently, we only have "msps", we would then need to Attempt to "select" both MSPS and expects one of them to return a result.

sschulz-t · 2021-03-15T09:06:30Z

1)
When exchanging document "POST private-document"
...
2)
WE need a way to "read" the "stored" Proof from the "ledger". As it stands, We only "write" to it. We do not have a way to read it out to perform any checks.

Good find! Thanks! Right now the Payloadlink is only checked during signature verification.
We should check that during the FetchPrivateDocument() call as well! I have a fix for that and will add it to this PR when i tested it.

A new problem now arise now that the "compositeKey" Location for both Signature and Payload is unified.

This way VerifySignatures
https://github.com/GSMA-CPAS/BWRP-chaincode/blob/ed470df0396da0fb2f3b486762eaaacd23d77e50/hybrid/offchain.go#L583
can now verify all signatures with on chain data.
A single call to this function will give you a list with all signatures including the valid/invalid flag.
You do not have to pass the payload any more.

zkong-gsma · 2021-03-15T10:02:06Z

A new problem now arise now that the "compositeKey" Location for both Signature and Payload is unified.

This way VerifySignatures
https://github.com/GSMA-CPAS/BWRP-chaincode/blob/ed470df0396da0fb2f3b486762eaaacd23d77e50/hybrid/offchain.go#L583
can now verify all signatures with on chain data.
A single call to this function will give you a list with all signatures including the valid/invalid flag.
You do not have to pass the payload any more.

I think there is some miscommunications here.

I didn't mentioned any issue with signature verification. They are all in good hands.

I am referring to Payload Verification, that we now need to also provide a "MSP" as a attribute to be able to verify. And we do not have a concreate "source" of "Msp" from the "object" to be used.

sschulz-t · 2021-03-15T10:04:48Z

I think there is some miscommunications here.

I didn't mentioned any issue with signature verification. They are all in good hands.

I am referring to Payload Verification, that we now need to also provide a "MSP" as a attribute to be able to verify. And we do not have a concreate "source" of "Msp" from the "object" to be used.

Your quote refers to your issue 3).

Issues 1+2 will be addressed by a fix to the chaincode PR which I will upload in some minutes.

sschulz-t · 2021-03-15T10:07:25Z

please see GSMA-CPAS/BWRP-chaincode@da3dc44
for a fix of issues 1) and 2)

zkong-gsma · 2021-03-15T10:20:11Z

please see GSMA-CPAS/BWRP-chaincode@da3dc44
for a fix of issues 1) and 2)

to my point. At this current stage . during a "GET /private-document" Processing after receiving a Webhook is all fine.

What issue 3) is referring to is Later Stage when a "Frontend" (webui) want to show the "Proofs" of if a "PAYLOAD" is valid. IE.

web-ui performs a "GET /contractst/{contractId}?type=RAW" = [PAYLOAD]

[missing feature], Read value/transaction from blockchian with "referenceId" = [HASH-CHECK]

From the Web-UI.

Show/display

payloadHash = hash(PAYLOAD)

LocalHash = hash (referenceId ":" payloadHash)

IF (LocalHash == [HASH-CHECK])  Payload valid.

so, when a "web-ui" performs a "GET /referenceID" (this is the missing feature mentioned above) we will need to provide not just a "referenceId", but a "MSP" as well.
we will have to randomly provide 2 MSPs available within the "contract->header->msps".

sschulz-t · 2021-03-15T12:52:59Z

Ok good point. Let's expose the function via the API. I will create a separate PR for this.
The issue will be tracked here: #31

For the fromMSP/toMSP issue on signature requests access: there is another issue that tracks this feature
#30
I am still not sure if we loose some security by dropping the creator from the composite key. I need to find time to think about all consequences.

sschulz-t added 11 commits December 8, 2020 16:06

add github action for automated build and deployment of docker files

c1417a1

initial test for verifySignatures

5aa79d7

added sgnature validation and reworked test_query.sh

1ccda74

(re)add accidently removed get function

0ca6663

wip towards new store and sign

4646677

wip

1a50768

wip

f8dfadf

fix signature verification

94d5862

fix signature verification, lint

0994256

implement new document store and signature store (see #26)

3582f50

fix data->payload. fix test_query exit bug

5a493fd

sschulz-t requested review from informartin and smeyerzu February 22, 2021 15:16

sschulz-t mentioned this pull request Feb 22, 2021

Add blockchain transaction id to response GSMA-CPAS/BWRP-common-adapter#23

Closed

smeyerzu approved these changes Feb 23, 2021

View reviewed changes

fix subscription type

29c85d6

informartin reviewed Feb 25, 2021

View reviewed changes

api/openapi.yaml Show resolved Hide resolved

sschulz-t mentioned this pull request Feb 26, 2021

storage key creation api #29

Closed

informartin mentioned this pull request Mar 1, 2021

Remove creator from API #30

Closed

informartin approved these changes Mar 1, 2021

View reviewed changes

zkong-gsma mentioned this pull request Mar 9, 2021

Existing contract not fetched GSMA-CPAS/BWRP-common-adapter#36

Open

zkong-gsma mentioned this pull request Mar 12, 2021

Common-adapter release v0.4 with working settlement generation (from calculation engine) GSMA-CPAS/BWRP-development-setup#19

Open

sschulz-t mentioned this pull request Mar 15, 2021

Feature: Expose verifyReferencePayloadLink via API #31

Closed

fix #32, add test case for signerMSP

3a3e6b8

This was linked to issues Mar 16, 2021

Bug Found when Attempting to implement New blockchain-adapter. #32

Closed

Remove creator from API #30

Closed

sschulz-t merged commit 76006a6 into master Mar 17, 2021

sschulz-t deleted the ssch-storeandsign branch March 17, 2021 10:01

smeyerzu mentioned this pull request Mar 24, 2021

Valid sign expose #15

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improved document handling and signature storing #27

improved document handling and signature storing #27

sschulz-t commented Feb 22, 2021

zkong-gsma commented Feb 22, 2021

smeyerzu left a comment

sschulz-t commented Feb 23, 2021

zkong-gsma commented Feb 23, 2021 via email

zkong-gsma commented Feb 24, 2021

sschulz-t commented Feb 24, 2021

zkong-gsma commented Feb 24, 2021

sschulz-t commented Feb 24, 2021

zkong-gsma commented Feb 24, 2021

zkong-gsma commented Feb 24, 2021

sschulz-t commented Feb 24, 2021

zkong-gsma commented Feb 25, 2021

zkong-gsma commented Feb 25, 2021

informartin commented Feb 25, 2021

sschulz-t commented Feb 26, 2021

zkong-gsma commented Feb 26, 2021

sschulz-t commented Feb 26, 2021

zkong-gsma commented Feb 26, 2021

sschulz-t commented Feb 26, 2021

sschulz-t commented Mar 10, 2021

zkong-gsma commented Mar 10, 2021

sschulz-t commented Mar 10, 2021 •

edited

Loading

zkong-gsma commented Mar 10, 2021

sschulz-t commented Mar 10, 2021

zkong-gsma commented Mar 10, 2021

zkong-gsma commented Mar 12, 2021

sschulz-t commented Mar 15, 2021

zkong-gsma commented Mar 15, 2021

sschulz-t commented Mar 15, 2021

sschulz-t commented Mar 15, 2021

zkong-gsma commented Mar 15, 2021 •

edited

Loading

sschulz-t commented Mar 15, 2021

improved document handling and signature storing #27

improved document handling and signature storing #27

Conversation

sschulz-t commented Feb 22, 2021

zkong-gsma commented Feb 22, 2021

smeyerzu left a comment

Choose a reason for hiding this comment

sschulz-t commented Feb 23, 2021

zkong-gsma commented Feb 23, 2021 via email

zkong-gsma commented Feb 24, 2021

sschulz-t commented Feb 24, 2021

zkong-gsma commented Feb 24, 2021

sschulz-t commented Feb 24, 2021

zkong-gsma commented Feb 24, 2021

zkong-gsma commented Feb 24, 2021

sschulz-t commented Feb 24, 2021

zkong-gsma commented Feb 25, 2021

zkong-gsma commented Feb 25, 2021

informartin commented Feb 25, 2021

sschulz-t commented Feb 26, 2021

zkong-gsma commented Feb 26, 2021

sschulz-t commented Feb 26, 2021

zkong-gsma commented Feb 26, 2021

sschulz-t commented Feb 26, 2021

sschulz-t commented Mar 10, 2021

zkong-gsma commented Mar 10, 2021

sschulz-t commented Mar 10, 2021 • edited Loading

zkong-gsma commented Mar 10, 2021

sschulz-t commented Mar 10, 2021

zkong-gsma commented Mar 10, 2021

zkong-gsma commented Mar 12, 2021

sschulz-t commented Mar 15, 2021

zkong-gsma commented Mar 15, 2021

sschulz-t commented Mar 15, 2021

sschulz-t commented Mar 15, 2021

zkong-gsma commented Mar 15, 2021 • edited Loading

sschulz-t commented Mar 15, 2021

sschulz-t commented Mar 10, 2021 •

edited

Loading

zkong-gsma commented Mar 15, 2021 •

edited

Loading