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.

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

Implement 1-RTT Exporter. #130

Closed
wants to merge 2 commits into from
Closed

Conversation

ekr
Copy link
Collaborator

@ekr ekr commented Jun 19, 2017

No description provided.

@ekr
Copy link
Collaborator Author

ekr commented Jun 19, 2017

@bifurcation: I think this is ready but I'd like a review from @martinthomson first.

Copy link
Contributor

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

This appears to be correct.

There are lots of unrelated changes here. I'm surprised that mint was not already formatted correctly though. Isn't that the whole point of CI?

@@ -785,6 +800,7 @@ type ServerStateWaitFinished struct {
handshakeHash hash.Hash
clientTrafficSecret []byte
serverTrafficSecret []byte
exporterSecret []byte
Copy link
Contributor

Choose a reason for hiding this comment

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

The duplication here is starting to look pretty bad. Any reason why these state variables can't be bound up into a struct that can be reused? This is probably a question for @bifurcation who created this miss in the first place.

conn.go Outdated

func (c *Conn) ComputeExporter(label string, context []byte, keyLength int) ([]byte, error) {
_, ok := c.hState.(StateConnected)
if !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ok/connected/, as above

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, if you want to factor out the early exporter, then checking that the appropriate secret is non-nil will achieve the same goal.

@@ -729,3 +727,20 @@ func (c *Conn) SendKeyUpdate(requestUpdate bool) error {
func (c *Conn) GetHsState() string {
return reflect.TypeOf(c.hState).Name()
}

func (c *Conn) ComputeExporter(label string, context []byte, keyLength int) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm: the plan is to add a ComputeEarlyExporter function for the early exporter and then to factor out the necessary parts of this function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah

@@ -729,3 +727,20 @@ func (c *Conn) SendKeyUpdate(requestUpdate bool) error {
func (c *Conn) GetHsState() string {
return reflect.TypeOf(c.hState).Name()
}

func (c *Conn) ComputeExporter(label string, context []byte, keyLength int) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I just learned something about go today. Apparently string is essentially the same as []byte. I don't know what to do about that though.

conn_test.go Outdated
@@ -274,6 +274,12 @@ func assertKeySetEquals(t *testing.T, k1, k2 keySet) {
assertByteEquals(t, k1.key, k2.key)
}

func computeExporter(t *testing.T, c *Conn, label string, length int) []byte {
res, err := c.ComputeExporter(label, []byte{}, length)
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests should exercise non-zero-length contexts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't expose non-zero length contexts. Do you think we need them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, never mind. Am idiot.

@bifurcation
Copy link
Owner

Replaced by #135

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