Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable message encryption and chaincode request message consistency check #516

Merged
merged 3 commits into from
Dec 31, 2020
Merged

Enable message encryption and chaincode request message consistency check #516

merged 3 commits into from
Dec 31, 2020

Conversation

bvavala
Copy link
Contributor

@bvavala bvavala commented Dec 26, 2020 •
edited
Loading

(self-explanatory)

This PR addresses #504 (consistency check), part of #486 (message encryption), and maintains the limitations described in #478 (message size).

Also, see comment below, closes #412, and #486 can be moved to next release

@bvavala bvavala requested a review from a team December 26, 2020 17:26
@bvavala bvavala marked this pull request as draft December 26, 2020 17:27
@bvavala bvavala force-pushed the bruno.20122020.message-encryption branch from 3e50bda to 208e9e7 Compare December 26, 2020 18:38
@bvavala bvavala added this to the MVP FPC 1.0 (aka FPC Lite) milestone Dec 26, 2020
@bvavala bvavala requested a review from g2flyer December 26, 2020 18:40
@bvavala bvavala marked this pull request as ready for review December 26, 2020 18:40
@bvavala bvavala changed the title Enable message encryption and chaincode request message constistency check Enable message encryption and chaincode request message consistency check Dec 26, 2020
@bvavala bvavala force-pushed the bruno.20122020.message-encryption branch from 208e9e7 to 8485afd Compare December 28, 2020 22:26
@g2flyer g2flyer mentioned this pull request Dec 30, 2020
Open
18 tasks
@g2flyer g2flyer linked an issue Dec 30, 2020 that may be closed by this pull request
Validation that invoke arguments are correct is missing #504
Closed
g2flyer
g2flyer approved these changes Dec 30, 2020
View reviewed changes
Copy link
Contributor

@g2flyer g2flyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good and should make complete now FPC 1.0 from an end-to-end security perspective! Also works nicely in HW mode for both (automated) integration test and (manual) test-net test. A few comments/observations/suggestions, though; none of them a show-stopper. So take it or leave it.

BTW: i think this should also close #412? Also #486, i guess we could move to FPC 1.1?

client_sdk/go/fpc/crypto/crypto.go Outdated Show resolved Hide resolved
client_sdk/go/fpc/crypto/crypto.go Outdated Show resolved Hide resolved
client_sdk/go/fpc/crypto/crypto.go Outdated Show resolved Hide resolved
common/enclave/cc_data.h
@@ -45,6 +45,9 @@ class cc_data
ByteArray get_state_encryption_key();
std::string get_enclave_id();
bool sign_message(const ByteArray& message, ByteArray& signature) const;
bool decrypt_cc_message(const ByteArray& encrypted_message, ByteArray& message) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the _cc_ seems a bit inconsistent given that we don't do it for sign and encrypt ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of agree.
Thing is that encrypt_message uses just symmetric crypto and passes a key, while decrypt_cc_message uses (for now just) asymmetric crypto.
So the "inconsistency" aims actually at differentiating this aspect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that the name does reflect this distinction? (for that i would have expected something like _sym_ and _pk_ or alike). BTW: also realize now that given that you pass the key (rather taking it from object like for decrypt), it's also less obvious why it is even a method of that object? Arguably, one could also have set the return key as object field, in which case arguably the sym vs pk distinction at this level of abstraction is actually completely irrelevant? All that said, the current naming certainly shouldn't lead to any confusion, so no real issue by leaving it as-is, just a bit nitpicking ;-)

ecc_enclave/enclave/enclave.cpp Outdated Show resolved Hide resolved
ecc_enclave/enclave/enclave.cpp Show resolved Hide resolved
internal/utils/utils.go Outdated Show resolved Hide resolved
@bvavala
robust handling of constants; comments; use encrypt_message function;…
cc0b8bd 
… move get-request-from-proposal to proto utils

Signed-off-by: Bruno Vavala <[email protected]>
@bvavala
Copy link
Contributor Author

bvavala commented Dec 30, 2020

Looks very good and should make complete now FPC 1.0 from an end-to-end security perspective!

Yes, finally!!

BTW: i think this should also close #412? Also #486, i guess we could move to FPC 1.1?

Good observation, yes and yes. Added to the PR comment.

@bvavala bvavala requested a review from g2flyer December 30, 2020 14:29
@bvavala
Copy link
Contributor Author

bvavala commented Dec 30, 2020

FYI travis fails due to some reached quotas enforced by docker. Given where it failed, I would call it successful.

@g2flyer
Copy link
Contributor

g2flyer commented Dec 30, 2020

FYI travis fails due to some reached quotas enforced by docker. Given where it failed, I would call it successful.

This dockerhub limit i've also encountered (regularly) on my local tests. There i solved it by doing a docker login with my dockerhub account. Not sure how you solve it for travis. It seems to work on and off, haven't seen a concrete pattern yet. But given that we might have to move to azure pipelines or alike, probably not worth spending much time on it.

g2flyer
g2flyer approved these changes Dec 30, 2020
View reviewed changes
Copy link
Contributor

@g2flyer g2flyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new version also works fine in HW mode and changes look good

@g2flyer
Copy link
Contributor

g2flyer commented Dec 30, 2020

new version also works fine in HW mode and changes look good

BTW: as you labelled this as security-related issue, should maybe @mbrandenburger also have a look for a second pair? (Not sure when he is back). Otherwise, i'll merge ...

@bvavala
Copy link
Contributor Author

bvavala commented Dec 30, 2020

BTW: as you labelled this as security-related issue, should maybe @mbrandenburger also have a look for a second pair? (Not sure when he is back). Otherwise, i'll merge ...

I would prefer to merge this, as well the upcoming one on mock_enclave, and do additional revisions/modifications post-facto.
Also, likely I won't be available to push fixes, so either you or Marcus may want to proceed in a separate PR.

@g2flyer
Copy link
Contributor

g2flyer commented Dec 31, 2020 •
edited
Loading

I would prefer to merge this,

ok, i'll merge

as well the upcoming one on mock_enclave, and do additional revisions/modifications post-facto.
Also, likely I won't be available to push fixes, so either you or Marcus may want to proceed in a separate PR.

As mentioned on slack, i'm off work this and next week, so will do unlikely much until Jan 11th, so will do unlikely much before second week of jan

@g2flyer g2flyer merged commit d61aef0 into hyperledger:master Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g2flyer g2flyer g2flyer approved these changes

Assignees

@g2flyer g2flyer

Labels
security
Projects
None yet
Milestone
MVP FPC 1.0 (aka FPC Lite)
Development

Successfully merging this pull request may close these issues.

Validation that invoke arguments are correct is missing Design common attestation / crypto API
2 participants
@bvavala @g2flyer