Skip to content

Public STR and TB fields for verification#35

Closed
arlolra wants to merge 6 commits into
masterfrom
str-tb
Closed

Public STR and TB fields for verification#35
arlolra wants to merge 6 commits into
masterfrom
str-tb

Conversation

@arlolra
Copy link
Copy Markdown
Member

@arlolra arlolra commented Jul 25, 2016

Reopening for discussion here since I messed up #34

Comment thread merkletree/str.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't LibVersion(), HashID() and EpochDeadlinePolicy() be in policy.go?

Copy link
Copy Markdown
Member

@vqhuy vqhuy Jul 26, 2016

Choose a reason for hiding this comment

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

These can change in epochs, thus I think it should be here, corresponding with policies of a specific STR. How does that sound? Never mind. I fixed it in 1d49b1e8b922409ad6f0b24fc6e7befee70978cc

@masomel
Copy link
Copy Markdown
Member

masomel commented Jul 25, 2016

Alternatively to having public fields in the TB and STR, what I had in mind when I opened #24 was to expose a Verify() function. What do you think of db24bfa?

@masomel
Copy link
Copy Markdown
Member

masomel commented Jul 25, 2016

In my branch, I decided to leave the public fields that were added in 812caad since I'm not sure if the client will still need to access those fields using the getters.

@vqhuy
Copy link
Copy Markdown
Member

vqhuy commented Jul 26, 2016

Alternatively to having public fields in the TB and STR, what I had in mind when I opened #24 was to expose a Verify() function. What do you think of db24bfa?

I got your idea. But the signing key cannot be used here, since I presume that the Verify() function should be called in the client-side only.

In my branch, I decided to leave the public fields that were added in 812caad since I'm not sure if the client will still need to access those fields using the getters.

Actually, I think the key server would use those getters to return the STR/TB to the client. The client doesn't use those getters directly.

@vqhuy vqhuy force-pushed the str-tb branch 2 times, most recently from 79e4d49 to c0a4f9e Compare July 26, 2016 10:06
@masomel
Copy link
Copy Markdown
Member

masomel commented Jul 27, 2016

the signing key cannot be used here, since I presume that the Verify() function should be called in the client-side only.

Yes, the idea would be for the client to call Verify(). I chose to use the SigningKey since that's what crypto.Verify() uses as well. If SigningKey is the server's key pair, I agree that Verify() shouldn't take it as a param, but then neither should the verification function in crypto.

Actually, I think the key server would use those getters to return the STR/TB to the client. The client doesn't use those getters directly.

Ok, I see.

@vqhuy
Copy link
Copy Markdown
Member

vqhuy commented Jul 28, 2016

Yes, the idea would be for the client to call Verify(). I chose to use the SigningKey since that's what crypto.Verify() uses as well. If SigningKey is the server's key pair, I agree that Verify() shouldn't take it as a param, but then neither should the verification function in crypto.

Sorry for that. I removed it in 3da95ef40a19094ad9f95f556676649c1deb2daf

@masomel
Copy link
Copy Markdown
Member

masomel commented Jul 28, 2016

Sorry for that. I removed it in 3da95ef

Yeah, with all the signature verification wrappers in place, I suppose we no longer need to export crypto's verify(). I don't think we need to remove it entirely if we want something like VerifySTR() to work. I gave this a shot in d46c806. Please let me know what you think!

@masomel
Copy link
Copy Markdown
Member

masomel commented Jul 28, 2016

I suppose we no longer need to export crypto's verify(). I don't think we need to remove it entirely if we want something like VerifySTR() to work.

Argh, this was silly of me. I forgot crypto is no longer a subpackage of merkletree. Sorry for the noise.

@vqhuy vqhuy force-pushed the str-tb branch 5 times, most recently from 90682a2 to e0adade Compare July 29, 2016 15:58
@masomel masomel force-pushed the master branch 3 times, most recently from 25a6925 to 2f704cf Compare July 29, 2016 20:03
@masomel
Copy link
Copy Markdown
Member

masomel commented Jul 29, 2016

Like we've decided to keep the VerifyX() functions in the lib as we discussed in d46c806#commitcomment-18452858, so I'll go ahead and merge in the str-tb-verify-idea branch and close this PR.

@masomel masomel closed this in c0a350d Jul 29, 2016
@masomel masomel deleted the str-tb branch July 29, 2016 23:25
masomel pushed a commit that referenced this pull request Jul 29, 2016
- Public STR and TB fields for verification
- Change key param in verify funcs to public key
- Make Sign and Verify functions of their key types
- Closes #24
- Closes #35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants