Vrf integration #19

liamsi · 2016-07-15T13:59:34Z

Use coname's vrf implementation to compute the private index

Should resolve Use the VRF for computing the private index #14
commented out parts of a test which is supposed to work with a specific hash (but not with vrf); might be changed with Stress test adding data to the tree #16 and Refactor testing code #18
replaced t.Error in combination with return with t.Fatal

vqhuy · 2016-07-15T14:55:32Z

merkletree.go

@@ -81,7 +83,8 @@ func (m *MerkleTree) Get(key string) (MerkleNode, []ProofNode) {
 }

 func (m *MerkleTree) Set(key string, value []byte) error {
-	index := computePrivateIndex(key)
+	vrfPriv := m.policies.vrfPrivate()


Shouldn't this be vrfPrivKey for consistency?

liamsi · 2016-07-15T20:27:45Z

There were quite some changes on master hence the merge conflicts.
In the current, changed design, we would need to pass the key from the PAD (which now contains the policy via the currentSTR) to the places were computePrivateIndex is called.

I wonder if it wasn't actually possible to remove the need of the actual key (which at the moment seems to be the actual username/"key-value") in the userLeafNode and directly call merkleTree.Set(key, value) with the actual precomputed private-index as a key instead of computing it inside of merkleTree.Set(...) via computePrivateIndex.
This would make it easier to compute the index (at leaster since the recent changes). Also (and maybe more important) I thought the actual usernames were not necessary and even for privacy reasons shouldn't directly be stored in the userLeafNodes? @masomel @c633 @arlolra

Hope I didn't mix up things here (and sorry for so many questions).

masomel · 2016-07-15T20:39:58Z

Also (and maybe more important) I thought the actual usernames were not necessary and even for privacy reasons shouldn't directly be stored in the userLeafNodes?

In our original design, we were more concerned with protecting a user's privacy against other users, and less with also protecting the user's privacy against the server. Maybe this is a bad assumption in practice. Another reason we left the actual usernames in the userLeafNodes is to allow the server to change vrfPrivateKey and reconstruct the whole tree without any user/client intervention. But we had a longer discussion about the privacy of usernames and keys in #1 (comment). I hope this clears up some of your questions!

masomel · 2016-07-15T20:54:24Z

I will add that you're definitely right about the actual usernames not being necessary to support lookups and auth path verifications. But I think supporting vrfPrivateKey changes on the server side helps protect users' privacy in a different way, as we discuss in §6.3 "Randomizing the order of directory entries" in the paper.

liamsi · 2016-07-15T21:00:54Z

Thanks @masomel! OK, makes sense.

So to keep it that way we need to either

store the (private-)key directly in the MerkleTree struct now
or
to pass the vrf-key from the PADs policy down to the call of MerkleTree's Set/Get
or
Change the Set method's signature to

// add index argument to be able to compute the index from outside/from the PAD
func (m *MerkleTree) Set(index []byte, key string, value []byte) error

and the Get signature to

// replace key argument with index
func (m *MerkleTree) Get(index []byte) *AuthenticationPath

I think 3) is the best option.

liamsi · 2016-07-15T21:05:29Z

With option 3) the vrf-computation would happen here instead of here and here right now.

vqhuy · 2016-07-16T02:08:01Z

There were quite some changes on master hence the merge conflicts.
In the current, changed design, we would need to pass the key from the PAD (which now contains the policy via the currentSTR) to the places were computePrivateIndex is called.

It is exactly what I was thinking! I think it makes sense to go with option 3), and computePrivateIndex should be a method of PAD.

masomel · 2016-07-16T04:46:29Z

@liamsi ce91df5 looks good to me!

arlolra · 2016-07-21T03:12:48Z

pad.go

 }

 func (pad *PAD) LookupInEpoch(key string, epoch uint64) (*AuthenticationPath, error) {
 	str := pad.GetSTR(epoch)
 	if str == nil {
 		return nil, ErrorSTRNotFound
 	}
-	ap := str.tree.Get(key)
+	lookupIndex := pad.computePrivateIndex(key)


This doesn't look like it's using the right private key. If possible, we should add a test for this case.

I'm not sure what you mean. Maybe the key variable name is a little misleading here? The (private-)key which is used to compute the priv. is this one: https://github.com/coniks-sys/libmerkleprefixtree-go/pull/19/files#diff-9c53128bbf22cd8275aacdbf0ae82035R18

Look at the implementation of computePrivateIndex,

vrfPivKey := pad.currentSTR.policies.vrfPrivate()

meaning that it gets the private key from the current epoch.

But this is a lookup in a specific epoch (the str := pad.GetSTR(epoch))

So, it seems like it needs to be,

str.policies.vrfPrivate()

And I'm assuming that the private key change in the policy means we need to recompute all the indices.

OK. I see. You're right. Thanks for the clarification! Will fix that (and add a test).

And I'm assuming that the private key change in the policy means we need to recompute all the indices.

Will dig into that tomorrow.

And I'm assuming that the private key change in the policy means we need to recompute all the indices.

Yes. This would be a good feature to expose to the developer as this also has the nice side-effect of re-randomizing the order of leaves in the tree (as discussed in #19 (comment)).

But this is a lookup in a specific epoch (the str := pad.GetSTR(epoch))
So, it seems like it needs to be, str.policies.vrfPrivate()

I think it makes sense to make computePrivateIndex a method of STR, thoughts?

Makes sense.

arlolra · 2016-07-21T03:16:53Z

The approach here looks good. Just a few comments to address.

vqhuy · 2016-07-21T04:45:49Z

@liamsi @arlolra @masomel What do you think about 24085a3844fd97a4417bae3a80c391f84cc1a5b8?

liamsi · 2016-07-21T21:39:57Z

TODOs (for myself)

merge vrf_proof branch
use correct private-key: Vrf integration #19 (comment)
resolve serialization: Vrf integration #19 (comment)
investigate (re)computation of indices (also Vrf integration #19 (comment) and TBs Vrf integration #19 (comment))
tests

liamsi · 2016-07-21T21:52:10Z

@liamsi @arlolra @masomel What do you think about 24085a3?

I think that's useful/helpful. Thanks a lot for chiming in @c633! I added some comments. Let me know what you think (if you want to keep as it is or consider some of the minor changes). I'll merge it into this branch, OK?

vqhuy · 2016-07-22T00:58:18Z

I agree with all your comments!

liamsi · 2016-07-22T07:38:10Z

I agree with all your comments!

OK. I'll merge your branch and do the changes.

arlolra · 2016-07-22T14:03:19Z

merkletree/pad.go

@@ -114,8 +114,9 @@ func (pad *PAD) GetSTR(epoch uint64) *SignedTreeRoot {
 func (pad *PAD) TB(key string, value []byte) (*TemporaryBinding, error) {
 	//FIXME: compute private index twice
 	//it would be refactored after merging VRF integration branch
-	index, _ := pad.computePrivateIndex(key)
-	tb := pad.currentSTR.sig
+	cStr := pad.currentSTR


Help me think this through, because this looks problematic.

Currently, when you call pad.Update(policies) it generates the next STR as if the passed in policies were the ones being used. But, if the private key changes, all these temporary bindings being handed out no longer have the right index.

I think PAD needs a policies field reflecting the current policies in place. currentSTR may be badly named in that it is really just the most recently signed tree root, whereas pad.tree is what's current and will be reflected in the next STR. Then, when you call Update(policies), it should generate the STR with the policies that were actually used (pad.policies), and then swap the pad.policies with the ones being passed in to be used going forward. It can then reshuffle the pad.tree if the private key changed.

Hopefully that makes sense.

Currently, when you call pad.Update(policies) it generates the next STR as if the passed in policies were the ones being used. But, if the private key changes, all these temporary bindings being handed out no longer have the right index.

Makes sense. Will change that.

I think PAD needs a policies field reflecting the current policies in place. currentSTR may

OK, I have to think about that (hopefully after the talks/program finished). But on first sight it makes sense to add a policies field to PAD.

currentSTR may be badly named in that it is really just the most recently signed tree root, whereas pad.tree is what's current and will be reflected in the next STR.

What do you think about renaming it to lastSTR or latestSTR?

Then, when you call Update(policies), it should generate the STR with the policies that were actually used (pad.policies), and then swap the pad.policies with the ones being passed in to be used going forward. It can then reshuffle the pad.tree if the private key changed.

Hopefully that makes sense.

Sounds reasonable.

Totally makes sense, it's a very good point. Thanks!

What do you think about lastSTR or latestSTR?

latestSTR sounds ok. While we're at it, maybe do s/generateNextSTR/SignTreeRoot/ as well.

OK, thanks.

I think PAD needs a policies field reflecting the current policies in place. currentSTR may

OK, I have to think about that (hopefully after the talks/program finished). But on first sight it makes sense to add a policies field to PAD.

Didn't have the time to think it through (yet). But do we still need the policies field in STR then (for serializing, or sth else)?

But do we still need the policies field in STR then (for serializing, or sth else)?

I think we do. Anything that gets signed needs to remain so that it can be verified later. But also, in this case, when you do the lookup from a previous epoch, you still need to use the key from the old policy.

So, I should have mentioned, computePrivateIndex should probably stay on the pad but we'll need a computePrivateIndexInEpoch as well. Or, something like that. You get the idea.

also, in this case, when you do the lookup from a previous epoch, you still need to use the key from the old policy.

I think this is done in e82f666f1930a475550a8c3eb00194932c003a19

Then, when you call Update(policies), it should generate the STR with the policies that were actually used (pad.policies), and then swap the pad.policies with the ones being passed in to be used going forward. It can then reshuffle the pad.tree if the private key changed.

Besides the reshuffling part this is also done in the latest commit. Please review. I have some questions regarding the design of the reshuffling (below).

liamsi · 2016-07-24T09:20:25Z

Currently, the reshuffling of the pad.tree is still missing. So after a (vrf) key-change LookUpInEpoch will succeed (as it uses the index of the str of that epoch) but a plain LookUp will fail if the key was inserted before (or immediately after) a vrf-key-change (as it will use the wrong vrf-key/indices)

To make this work @arlolra suggested a re-computation of the indices (#19 (comment)). As far as I understand it the only way to re-shuffle the pad.tree (from outside the package merkletree) is to actually know all the keys in order to re-compute their indices, right? This would mean to have a way to get all keys from one MerkleTree.
If we wanted to re-compute the tree from within the merkletree package (where one could get all keys for instance by traversing the tree), we would need to pass the vrf-private-key to the MerkleTree (so that it can recompute its indices without exposing the keys for which the re-computation of indices will happen)

@c633 @arlolra @masomel Any thoughts on which is the better/safer approach?

liamsi · 2016-07-24T12:45:50Z

I thought a little about my above question: I think exposing the 'key's from the tree isn't the way to go, as it's to easy (as an user of this library) to get it wrong (and for instance expose the keys somehow or misuse it in another way).
The other option (give the tree the vrf-key/the vrf-function) also comes with downsides: The clear relation between the tree and the pad gets a little less clear (both would compute the private indices for keys).
The easiest way to implement the latter would be to store the keys in a private array and iterate over them to get their values (with the old index) and to insert both in a fresh tree with the new index.

vqhuy · 2016-07-24T13:00:51Z

I thought a little about my above question: I think exposing the 'key's from the tree isn't the way to go, as it's to easy (as an user of this library) to get it wrong (and for instance expose the keys somehow or misuse it in another way).

It seems I didn't understand your idea completely. Since the tree and pad are in the same package (merkletree), I don't know how the developers can get "exposing keys" if we don't export them?

The easiest way to implement the latter would be to store the keys in a private array and iterate over them to get their values and to insert both in a fresh tree with the new index.

I prefer this way.

vqhuy · 2016-07-24T14:58:51Z

The easiest way to implement the latter would be to store the keys in a private array and iterate over them to get their values (with the old index) and to insert both in a fresh tree with the new index.

I thought a little about this implementation. There are some concerns here:

If we store the keys in a private array, I presumably we should remove the key field in userLeafNode struct.
We also need to store the 'value's along with the keys. Back to Merkle tree #1 (comment), @masomel said that "(We do have a protocol for also keeping the value private from the server, though it's not the default, so I'm not sure if it should be built into the tree design)". Should we use that protocol?
Should we separate this into another issue, and merge current changes first?

arlolra · 2016-07-24T17:40:54Z

To make this work @arlolra suggested a re-computation of the indices (#19 (comment)). As far as I understand it the only way to re-shuffle the pad.tree (from outside the package merkletree) is to actually know all the keys in order to re-compute their indices, right? This would mean to have a way to get all keys from one MerkleTree.

Could you add a method of MerkleTree,

WalkTree(m *MerkleTree) (call func(*node)) {
   // visit all the leaf nodes and call func
}

Then you could,

ReShuffle(pad *PAD) {
  newTree := NewMerkleTree()
  pad.tree.WalkTree(func(node) {
     newIndex = pad.computePrivateKey(node.key)
     newTree.Set(newIndex, node.key, node.value)
  })
  pad.tree = newTree
}

arlolra · 2016-07-24T17:52:14Z

merkletree/pad.go

+	if str == nil {
+		return ErrorSTRNotFound
+	}
+	index, _ := pad.computePrivateIndexInEpoch(key, str.epoch)


This needs to use pad.policies, not the latestSTR.

There are two distinct times that we need to compute a private index,

When setting a key. This happens before a root is signed, and therefore cannot get the key from an STR.

When looking up a key. This always happens from a STR, and therefore uses the key from the policy in the STR.

So, it's fine to make computePrivateIndex a method of STR, but then you'll also need another method to handle the Set case.

Or, you can make it a method of PAD but one of the arguments needs to be a key.

Thanks again for your feedback! Changed in ad750fa

liamsi · 2016-07-24T19:45:34Z

It seems I didn't understand your idea completely. Since the tree and pad are in the same package (merkletree), I don't know how the developers can get "exposing keys" if we don't export them?

You are absolutely right. I actually confused the packages while thinking about it ... Thanks for your feedback and sorry for the noise!

The easiest way to implement the latter would be to store the keys in a private array and iterate over them to get their values and to insert both in a fresh tree with the new index.

I prefer this way.

OK, traversing the tree (as @arlolra suggested in #19 (comment)) maybe more efficient and it certainly makes storing the keys twice obsolete. I think, I'll go this way now.

liamsi · 2016-07-24T20:22:33Z

We also need to store the 'value's along with the keys. Back to #1 (comment), @masomel said that "(We do have a protocol for also keeping the value private from the server, though it's not the default, so I'm not sure if it should be built into the tree design)". Should we use that protocol?

@c633 @masomel depending on the protocol and its complexity this would be a nice (optional?) addition

Should we separate this into another issue, and merge current changes first?

I think the above should be a separate issue. Regarding the recomputation of the tree/indices I think it's fine to keep it in this issue (as it is definitely related to the vrf-integration, also I feel all open questions are resolved now). On the other hand we thought of this as changing very few lines only, yo if you want to keep it in small separate issues I wouldn't vote against it.

arlolra · 2016-07-24T20:48:36Z

merkletree/pad.go

@@ -41,29 +45,42 @@ func NewPAD(policies Policies, key crypto.SigningKey, len int64) (*PAD, error) {
 }

 // if policies is nil, the previous policies will be used
-func (pad *PAD) generateNextSTR(policies Policies, m *MerkleTree, epoch uint64) {
+func (pad *PAD) signTreeRoot(policies Policies, m *MerkleTree, epoch uint64) {


The policies argument here is unnecessary, since it's always going to be pad.policies, which the pad has access to.

liamsi · 2016-07-24T22:46:28Z

I think this ready for another review. Sorry, that this took longer than expected... And thanks everyone for all the valuable feedback so far.

arlolra · 2016-07-25T00:31:26Z

merkletree/pad.go

@@ -31,6 +34,7 @@ func NewPAD(policies Policies, key crypto.SigningKey, len int64) (*PAD, error) {
 	pad := new(PAD)
 	pad.key = key
 	pad.tree, err = NewMerkleTree()
+	pad.policies = policies
 	if err != nil {
 		return nil, err
 	}


Can just do pad.updateInternal(nil, 0) on L43 now

vqhuy · 2016-07-25T04:50:32Z

Regarding the recomputation of the tree/indices I think it's fine to keep it in this issue (as it is definitely related to the vrf-integration, also I feel all open questions are resolved now).

Never mind. I'm okay with this.

vqhuy · 2016-07-25T05:00:37Z

merkletree/merkletree.go

+	visitULNsInternal(m.root, callBack)
+}
+
+func visitULNsInternal(nodePtr interface{}, callBack func(*userLeafNode)) {


I would change nodePtr interface{} into nodePtr MerkleNode. I also want to change L43 and L117, too. So if it makes sense to you ...

Makes sense! I'll change the other lines for consistency.

Yeah, thanks!

liamsi · 2016-07-25T08:03:32Z

Incorporated all comments (so far) here 77a868d and changed to crypto/subtle for the pivate-key comparison here a2cf957

vqhuy · 2016-07-25T11:47:14Z

Thanks! It looks great.

* Should resolve #14

masomel · 2016-07-25T18:51:37Z

OK, traversing the tree (as @arlolra suggested in #19 (comment)) maybe more efficient and it certainly makes storing the keys twice obsolete. I think, I'll go this way now.

Seems fine with me.

@c633 @masomel depending on the protocol and its complexity this would be a nice (optional?) addition

Our idea was that users seeking to keep their key(s) or other data private from the server as well as other users would encrypt the value blob in the userLeafNode with a symmetric key that only those clients that the user authorizes (i.e. gives the symmetric key to) can decrypt. This way, the value still looks like some random blob and it's up to the authorized clients to parse this blob correctly. The main downside I see in this protocol is how the clients distribute this symmetric key and how they revoke this key whenever they need to revoke some client's access to the value. But I think if we wanted to add this protocol as an additional privacy feature, it would probably have to be on the client side anyway.

This is a protocol we sketched out as part of a small follow-on project, and we never tested it out or anything, so I'm open to improvements/suggestions. Either way, I don't think that adding this protocol as part of this PR makes sense right now.

arlolra · 2016-07-25T19:06:21Z

Nice work @liamsi

vqhuy reviewed Jul 15, 2016
View reviewed changes

arlolra reviewed Jul 21, 2016
View reviewed changes

masomel force-pushed the master branch from 9f95265 to ae15fa3 Compare July 22, 2016 01:10

arlolra reviewed Jul 22, 2016
View reviewed changes

arlolra reviewed Jul 24, 2016
View reviewed changes

arlolra reviewed Jul 25, 2016
View reviewed changes

vqhuy reviewed Jul 25, 2016
View reviewed changes

Use coname's vrf implementation to compute the private index

3677878

* Should resolve #14

arlolra force-pushed the vrf_integration branch from a2cf957 to 3677878 Compare July 25, 2016 18:30

arlolra merged commit 3677878 into master Jul 25, 2016

arlolra deleted the vrf_integration branch July 25, 2016 19:05

Vrf integration #19

Vrf integration #19

Conversation

liamsi commented Jul 15, 2016

Choose a reason for hiding this comment

liamsi commented Jul 15, 2016 • edited Loading

masomel commented Jul 15, 2016 • edited Loading

masomel commented Jul 15, 2016

liamsi commented Jul 15, 2016 • edited Loading

liamsi commented Jul 15, 2016

vqhuy commented Jul 16, 2016 • edited Loading

masomel commented Jul 16, 2016

Choose a reason for hiding this comment

liamsi Jul 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arlolra commented Jul 21, 2016

vqhuy commented Jul 21, 2016

liamsi commented Jul 21, 2016 • edited Loading

liamsi commented Jul 21, 2016 • edited Loading

vqhuy commented Jul 22, 2016

liamsi commented Jul 22, 2016

Choose a reason for hiding this comment

liamsi Jul 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Jul 22, 2016 • edited Loading

Choose a reason for hiding this comment

arlolra Jul 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Jul 24, 2016 • edited Loading

liamsi commented Jul 24, 2016 • edited Loading

vqhuy commented Jul 24, 2016

vqhuy commented Jul 24, 2016

arlolra commented Jul 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Jul 24, 2016

liamsi commented Jul 24, 2016 • edited Loading

Choose a reason for hiding this comment

liamsi commented Jul 24, 2016

arlolra Jul 25, 2016 • edited Loading

Choose a reason for hiding this comment

vqhuy commented Jul 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi commented Jul 25, 2016

vqhuy commented Jul 25, 2016

masomel commented Jul 25, 2016

arlolra commented Jul 25, 2016

liamsi commented Jul 15, 2016 •

edited

Loading

masomel commented Jul 15, 2016 •

edited

Loading

liamsi commented Jul 15, 2016 •

edited

Loading

vqhuy commented Jul 16, 2016 •

edited

Loading

liamsi Jul 21, 2016 •

edited

Loading

liamsi commented Jul 21, 2016 •

edited

Loading

liamsi commented Jul 21, 2016 •

edited

Loading

liamsi Jul 22, 2016 •

edited

Loading

liamsi Jul 22, 2016 •

edited

Loading

arlolra Jul 22, 2016 •

edited

Loading

liamsi commented Jul 24, 2016 •

edited

Loading

liamsi commented Jul 24, 2016 •

edited

Loading

arlolra commented Jul 24, 2016 •

edited

Loading

liamsi commented Jul 24, 2016 •

edited

Loading

arlolra Jul 25, 2016 •

edited

Loading