Remove USIG epoch handling from core code #14

sergefdrv · 2018-07-26T14:24:58Z

The replicas need to agree so that each peer has a single instance of USIG able to produce valid UIs. This is required to prevent a faulty replica from sending conflicting messages to different nodes in the consensus network. The current approach of capturing the USIG epoch value from the first valid UI received from a peer relies on a number of assumptions:

all replicas are initially correct,
each replica uses its unique USIG key pair per consensus protocol instance,
the first UI is generated using a single USIG instance per replica,
those first UIs are received and processed by all correct replicas before any replica becomes faulty in a sense that it starts generating and sending UIs using another USIG instance initialized with the same sealed key pair.

Those assumptions might be too strong in some environments. In that case, there should be a way for all correct replicas to agree on a single USIG instance identity per each replica and use that identity to verify UIs of received consensus messages.

Considering this, it seems to be most reasonable to move the current approach of achieving the agreement on USIG identity outside of the core protocol implementation.

This series does some refactoring to make it easier and eventually moves the USIG epoch handling from the core protocol implementation to the sample implementation of the external authentication interface.

sergefdrv · 2018-07-26T14:34:40Z

@luthlee Could you please review this PR?

sergefdrv · 2018-07-26T14:42:26Z

@ynamiki Somehow I don't seem to be able to request review from luthlee...

sergefdrv · 2018-07-26T14:47:06Z

@ynamiki I wonder why this PR doesn't go through CI?

sergefdrv · 2018-07-26T15:40:00Z

@Naoya-Horiguchi @ynamiki Who of you would also like to review this PR?

nhoriguchi · 2018-07-27T00:09:41Z

uh, I pushed review button in order to approve the first commit of this series, but a whole pull request was approved which is not yet intended. I don't see how to cancel my approval, but anyway I still continue to review to this.

ynamiki · 2018-07-27T00:46:04Z

Somehow I don't seem to be able to request review from luthlee...

@sergefdrv Reviewers need to have read access to the repository. We have two options for give the access: 1) add to organization nec-blockchain (if luthlee is a member of your lab), or 2) add as an outside collaborator of this repository nec-blockchain/minbft. Which is preferred?

ynamiki · 2018-07-27T01:17:15Z

I wonder why this PR doesn't go through CI?

@sergefdrv I have update settings in CircleCI to build forked pull requests. It will work at next commit (I have tested with #15).

nhoriguchi · 2018-07-27T04:04:27Z

@sergefdrv, I don't get into details (I need learn existing code more..), I have a few feedbacks:

patch descriptions are nicely written and I felt easy to understand what you try to do.
you state in comment around usig_create_ui() "the epoch and counter values in little-endian byte order", I guess this is because it's required by hardware. If so, that's worth explaining in the comment.
time.Sleep() inserted for simulation mode had better be conditional by checking environment variable SGX_MODE.

sergefdrv · 2018-07-27T08:40:51Z

The commits are shown in the pull request by GitHub in a weird way because of https://help.github.com/articles/why-are-my-commits-in-the-wrong-order/, isaacs/github#386.

sergefdrv · 2018-07-27T09:03:06Z

I rebased the commits with --ignore-date and they appear in the right order on GitHub. isaacs/github#386 (comment)

sergefdrv · 2018-07-27T09:08:18Z

We have two options for give the access: 1) add to organization nec-blockchain (if luthlee is a member of your lab), or 2) add as an outside collaborator of this repository nec-blockchain/minbft. Which is preferred?

@ynamiki Please go ahead with option (1). She is from our lab 🙂

sergefdrv · 2018-07-27T09:27:21Z

you state in comment around usig_create_ui() "the epoch and counter values in little-endian byte order", I guess this is because it's required by hardware. If so, that's worth explaining in the comment.

That is not really the requirement. It was just easier to do so. SGX is only available on Intel CPUs. Those are always little-endian. The signature payload is conveniently constructed as a "packed" C structure (see https://github.com/sergefdrv/minbft/blob/bf79a36638d99873ae5c28141c8410c7e4771abe/usig/sgx/enclave/usig.c#L30). That is why those numbers are in little endian. Changing to big endian would require adding more code in enclave, which is better to keep at minimun.

time.Sleep() inserted for simulation mode had better be conditional by checking environment variable SGX_MODE.

SGX_MODE determines the enclave build mode, but only at build time. It is not reliable indicator at runtime. For instance, bin/keytool generate mentioned in https://github.com/nec-blockchain/minbft#generating-keys can be invoked in a separate shell with no SGX_MODE set.

I was thinking of building both version of enclave and store them in separate files. Then one could chose the version to use at runtime. What do you think?

sergefdrv · 2018-07-27T09:29:25Z

@Naoya-Horiguchi Please do not merge this PR until @luthlee has finished reviewing it.

luthlee · 2018-07-27T08:36:42Z

usig/sgx/sgx-usig_test.go


+	usigID, err := MakeID(usig.Epoch(), usigPubKey)
+	require.NoError(t, err)
+


Unit test for the ParseID()

Thanks to pointing that out. I'll add a test for this.

luthlee · 2018-07-27T09:00:10Z

sample/authentication/crypto.go

-// interface by utilizing SGX USIG to create/verify authentication
-// tags.
+// usigKeyFingerprint is a SHA256 hash of the USIG public key.
+type usigKeyFingerprint [sha256.Size]byte


Maybe you want to use a shorter hash output or truncate the output? SGX ECDSA is 256-bit, so the public key is 512-bit long. Your fingerprint only saves half the space.

I'm thinking to truncate this to 64-bit fingerprint.

luthlee · 2018-07-27T09:36:33Z

sample/authentication/crypto.go

+	//
+	// Those assumptions might be too strong in some environments.
+	// In that case, there should be a way for all correct
+	// replicas to agree on a single USIG instance identity per


If there is a correct bootstrap/initialization stage, replicas are not required to be all correct.
Therefore, "in that case, a correct bootstrap is required for all replicas to first agree on...".

Exactly, there would be no need for all replicas to be initially correct. That is why only the correct replicas need to agree; up to f replicas could be faulty from the beginning and do not agree. I didn't want to emphasize how it should be achieved. It could be achieved at bootstrap phase or maybe some other way. But I'm considering to change the wording to: "In that case, all correct replicas are required to agree ..." Would that make sense?

sergefdrv · 2018-07-27T09:53:18Z

time.Sleep() inserted for simulation mode

What we could also do is to make a pull request to change the way SGX simulation mode sets the seed value. If it gets accepted, we could get rid of this hack when the fix gets released. intel/linux-sgx#246 (comment)

sergefdrv · 2018-07-27T10:00:46Z

I don't see how to cancel my approval

@Naoya-Horiguchi You could try this https://blog.github.com/2016-10-12-dismissing-reviews-on-pull-requests/

ynamiki · 2018-07-27T11:16:54Z

Please go ahead with option (1). She is from our lab 🙂

I have invited @luthlee to nec-blockchain.

SGX USIG public key in SGX_ECDSA key spec is a normal ECDSA key. Reuse normal ECDSA key spec implementation to parse it. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

Current implementation of USIG authentication scheme does only support SGX USIG. This is not easy to make generic because USIG identity passed to VerifyUI is going to be highly depend on particular USIG implementation. Current implementation uses serialized public key as USIG identity. This does not reflect actual SGX USIG identity and is going to change. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

We might want to have a dummy USIG implementation that would not require SGX SDK to build it, but that should implement usig.USIG interface rather than api.Authenticator. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

USIGEnclave instance is guaranteed to have an enclave instance created. So the key sealing should never fail. If that happens, there's nothing to do more; just give up and panic. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

This field is in fact a digital signature over a message digest, epoch and counter values. It is an implementation detail that this signature solely represent SGX USIG certificate. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

Currently there is no way to get the epoch value out of a SGX USIG instance without creating a new UI, but that have a side effect of increasing its counter value. This functionality is going to be useful for composing a full USIG identity which is USIG public key combined with the epoch value of the particular USIG instance. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

Now the epoch value can be directly retrieved once the enclave is initialized, there is no need to do that each time a new UI is assigned. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

This structure keeps only two fields now. Moreover, it may be confused with actual UI structure defined in Go code. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

USIGEnclave is a wrapper around SGX enclave, it should not deal with USIG API entities. USIG type, on the other hand, is implementation of USIG interface. Thus, it is more appropriate to move UI construction there. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

The signature verification is a nontrivial separate step in verifying a USIG UI. It makes sense to encapsulate it into a separate function, close to the code which produces the signature and determines its format. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

The public key alone doesn't really serve as USIG identity since multiple instances of USIG enclave can be created on the same machine initialized with the same sealed key pair. Those instances will share the public key. However, each instance will have its unique epoch value. So the epoch value combined with the public key make up the actual USIG identity. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

MinBFT protocol does not actually define USIG epoch value. The notion of the epoch value is specific to current SGX USIG implementation and is used to determine full SGX USIG instance identity. These details should not be handled by the core of MinBFT protocol. Move this to the sample implementation of authentication external interface. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

The epoch value is an implementation detail of SGX USIG. The core protocol should not be aware of this. Encapsulate the epoch value into USIG certificate to make the sample USIG authentication be able to extract it from the UI and update the captured epoch value. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

Big-endian byte order is conventionally used to marshal values to be exchanged through the network. Follow that convention. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

USIG-based authentication scheme is actually assumed to guarantee agreement on a single USIG instance per replica node among the peers. State this requirement clearly in an API documentation comment. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

This is needed to ensure that each instance of USIG enclave gets initialized with different random number generation seed if the enclave is built in simulation mode. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

sergefdrv · 2018-07-30T08:56:18Z

@luthlee I have tried to address your comments. Sorry, I didn't know how GitHub handles it and force-pushed the changed series here. So there seem to be no way to compare with the previous revision. Next time I will follow some other approach as discussed in #12 .

sergefdrv · 2018-08-02T14:43:07Z

@Naoya-Horiguchi Would you have more comments?

because updated version will be posted.

nhoriguchi · 2018-08-03T01:35:25Z

I was thinking of building both version of enclave and store them in separate files. Then one could chose the version to use at runtime. What do you think?

I think that if this is easy enough, you can do it.

What we could also do is to make a pull request to change the way SGX simulation mode sets the seed value. If it gets accepted, we could get rid of this hack when the fix gets released. intel/linux-sgx#246 (comment)

That's the best scenario for us, so I hope your suggestion will be accepted.

sergefdrv · 2018-08-03T08:26:19Z

@Naoya-Horiguchi Building both HW and simulation version of enclave would require significant change in enclave Makefile. This looks like a separate issue to address later.

As of changing SGX SDK code, it doesn't look like they are going to change it soon. We could propose the change ourselves. This is rather easy change, but somebody would need to take care of this.

sergefdrv · 2018-08-03T08:28:17Z

@Naoya-Horiguchi Actually, changing the way the enclave is build wouldn't eliminate this workaround with 1 second delay. We would still use simulation mode in CI, for example. So the best way would be to propose the change to SGX SDK.

sergefdrv · 2018-08-03T08:31:53Z

@Naoya-Horiguchi

because updated version will be posted

Sorry, the content of this pull request has been updated since your comment #14 (comment). Next time I will follow the approach as discussed in #12 .

nhoriguchi · 2018-08-06T00:48:55Z

@sergefdrv thank you for the explanation. It seems to me that the RDRAND approach pointed out in the SGX SDK thread might be fine because code change is minimum (maybe calling sgx_read_rand() to generate a seed?) and we might avoid additional library dependency as in nanosecond approach. RDRAND is available since Ivy Bridge CPU, so most of our platform should have it.

sergefdrv · 2018-08-06T08:33:38Z

@Naoya-Horiguchi

It seems to me that the RDRAND approach pointed out in the SGX SDK thread might be fine

I'm not sure if RDRAND would always be available in a could-based CI.

because code change is minimum (maybe calling sgx_read_rand() to generate a seed?)

We do not generate the seed ourselves, this is what SGX SDK does. We use sgx_read_rand() https://github.com/nec-blockchain/minbft/blob/master/usig/sgx/enclave/usig.c#L171 and sgx_ecc256_create_key_pair() https://github.com/nec-blockchain/minbft/blob/master/usig/sgx/enclave/usig.c#L119. The point is that any random number SGX SDK provides in simulation mode is based on a seed value captured at enclave creation time. It is currently the local time in seconds. That is SGX SDK implementation.

and we might avoid additional library dependency as in nanosecond approach.

I'm not sure what do you mean by additional library dependency. I was suggesting if some of us could prepare a patch for SGX SDK to use higher-precision timestamp for seeding the pseudo random number generation in simulation mode.

RDRAND is available since Ivy Bridge CPU, so most of our platform should have it.

I think anyone should be able to try our MinBFT implementation in simulation mode without any problem.

nhoriguchi · 2018-08-06T09:10:32Z

@sergefdrv sorry for my lack of words. My previous comment was intended to tell about how we fix seed generation in SGX SDK as you said. I think we have 2 options: using nanosecond timestamp as a seed, and using sgx_read_rand() as a seed instead of second timestamp.

and we might avoid additional library dependency as in nanosecond approach.
I'm not sure what do you mean by additional library dependency.

sorry again. I just meant adding '#include " may be needed, but actually that introduces no problem.

I think anyone should be able to try our MinBFT implementation in simulation mode without any problem.

OK, so nanosecond approach is better.

I was suggesting if some of us could prepare a patch for SGX SDK to use higher-precision timestamp for seeding the pseudo random number generation in simulation mode.

If you are OK, can I try this?

sergefdrv · 2018-08-06T09:31:55Z

@Naoya-Horiguchi

I think we have 2 options: using nanosecond timestamp as a seed, and using sgx_read_rand() as a seed instead of second timestamp.

I'm not sure if sgx_read_rand() would be available at the point of enclave simulation initialization. sgx_read_rand() is provided by SGX SDK for enclave trusted code, whereas PRNG seed value is initialized in untrusted code. My suggestion is to change https://github.com/intel/linux-sgx/blob/54cae063cd0d21be5fab28c0d4b81b073b5d0914/sdk/simulation/urtssim/enclave_creator_sim.cpp#L232 to something using clock_gettime() with CLOCK_MONOTONIC.

I was suggesting if some of us could prepare a patch for SGX SDK to use higher-precision timestamp for seeding the pseudo random number generation in simulation mode.

If you are OK, can I try this?

Please go ahead, I'll be focussed on implementing view change in MinBFT.

sergefdrv · 2018-08-06T09:33:23Z

@Naoya-Horiguchi Would it be anything to change in this pull request, or can we merge it?

nhoriguchi · 2018-08-06T09:41:16Z

@sergefdrv I'm fine to merge this series, thank you for your effort!

sergefdrv force-pushed the remove-ui-epoch-handling-from-core-code branch from 1c704ad to e9aecea Compare July 26, 2018 15:12

nhoriguchi previously approved these changes Jul 26, 2018

View reviewed changes

sergefdrv force-pushed the remove-ui-epoch-handling-from-core-code branch 2 times, most recently from 4f0f3f6 to bf79a36 Compare July 27, 2018 09:01

luthlee reviewed Jul 27, 2018

View reviewed changes

Sergey Fedorov added 9 commits July 27, 2018 14:39

sample/authentication: Parse SGX USIG public key as normal ECDSA key

8b8bef2

SGX USIG public key in SGX_ECDSA key spec is a normal ECDSA key. Reuse normal ECDSA key spec implementation to parse it. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

usig/fake: Remove unused outdated code

46c8261

We might want to have a dummy USIG implementation that would not require SGX SDK to build it, but that should implement usig.USIG interface rather than api.Authenticator. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

usig/sgx: Panic on key sealing failure

3fff571

USIGEnclave instance is guaranteed to have an enclave instance created. So the key sealing should never fail. If that happens, there's nothing to do more; just give up and panic. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

usig/sgx: Rename usig_ui.{cert,signature}

8514c65

This field is in fact a digital signature over a message digest, epoch and counter values. It is an implementation detail that this signature solely represent SGX USIG certificate. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

usig/sgx: Remove epoch parameter from create_ui ECall

6a916e2

Now the epoch value can be directly retrieved once the enclave is initialized, there is no need to do that each time a new UI is assigned. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

usig/sgx: Remove usig_ui structure from enclave shim

db5f2c8

This structure keeps only two fields now. Moreover, it may be confused with actual UI structure defined in Go code. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

Sergey Fedorov added 8 commits July 27, 2018 14:39

usig/sgx: Refine comment for usig_init function in enclave shim

e4b0d1d

Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

usig: Marshal USIG UI elements in big-endian byte order

7ad63ed

Big-endian byte order is conventionally used to marshal values to be exchanged through the network. Follow that convention. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

sergefdrv force-pushed the remove-ui-epoch-handling-from-core-code branch from bf79a36 to 44780cf Compare July 27, 2018 12:39

sergefdrv mentioned this pull request Jul 27, 2018

Make Rule of Merging Pull Request #12

Closed

luthlee approved these changes Aug 2, 2018

View reviewed changes

sergefdrv merged commit f5a973b into hyperledger-labs:master Aug 6, 2018

sergefdrv deleted the remove-ui-epoch-handling-from-core-code branch August 6, 2018 09:50


		usigID, err := MakeID(usig.Epoch(), usigPubKey)
		require.NoError(t, err)

Remove USIG epoch handling from core code #14

Remove USIG epoch handling from core code #14

Uh oh!

Conversation

sergefdrv commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergefdrv commented Jul 26, 2018

Uh oh!

sergefdrv commented Jul 26, 2018

Uh oh!

sergefdrv commented Jul 26, 2018

Uh oh!

sergefdrv commented Jul 26, 2018

Uh oh!

nhoriguchi commented Jul 27, 2018

Uh oh!

ynamiki commented Jul 27, 2018

Uh oh!

ynamiki commented Jul 27, 2018

Uh oh!

nhoriguchi commented Jul 27, 2018

Uh oh!

sergefdrv commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergefdrv commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergefdrv commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergefdrv commented Jul 27, 2018

Uh oh!

sergefdrv commented Jul 27, 2018

Uh oh!

luthlee Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

sergefdrv Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

luthlee Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

sergefdrv Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

luthlee Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

sergefdrv Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

sergefdrv commented Jul 27, 2018

Uh oh!

sergefdrv commented Jul 27, 2018

Uh oh!

ynamiki commented Jul 27, 2018

Uh oh!

sergefdrv commented Jul 30, 2018

Uh oh!

sergefdrv commented Aug 2, 2018

Uh oh!

nhoriguchi commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergefdrv commented Aug 3, 2018

Uh oh!

sergefdrv commented Aug 3, 2018

Uh oh!

sergefdrv commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nhoriguchi commented Aug 6, 2018

Uh oh!

sergefdrv commented Aug 6, 2018

Uh oh!

nhoriguchi commented Aug 6, 2018

Uh oh!

sergefdrv commented Jul 26, 2018 •

edited

Loading

sergefdrv commented Jul 27, 2018 •

edited

Loading

sergefdrv commented Jul 27, 2018 •

edited

Loading

sergefdrv commented Jul 27, 2018 •

edited

Loading

nhoriguchi commented Aug 3, 2018 •

edited

Loading

sergefdrv commented Aug 3, 2018 •

edited

Loading