-
Notifications
You must be signed in to change notification settings - Fork 5
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
shorter handle representation. #68
Conversation
Is this ready for review or are we waiting for a part 2 of 2? |
the trust domains in client.conf should probably also get hex treatment rather than the go json encoder default treatment of array of bytes. That's on my list to do with #7 - having the connection between the client and frontend be over TLS with a pinned server cert. |
which is to say. this one is ready for review. |
libtalek/handle.go
Outdated
// UnmarshalText restores a handle from its compact textual representation | ||
func (h *Handle) UnmarshalText(text []byte) error { | ||
var s1, s2, ss, pk []byte | ||
if n, err := fmt.Sscanf(string(text), "%x.%x.%x.%x.%d", &s1, &s2, &ss, &pk, &h.Seqno); n < 4 || err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n<5?
libtalek/handle_test.go
Outdated
if err != nil { | ||
t.Fatalf("Could not deserialize: %v\n", err) | ||
} | ||
if !bytes.Equal(h.SharedSecret[:], h2.SharedSecret[:]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check other fields?
@@ -111,3 +113,21 @@ func (t *Topic) encrypt(plaintext []byte, nonce *[24]byte) ([]byte, error) { | |||
digest := ed25519.Sign(t.SigningPrivateKey, buf) | |||
return append(buf, digest[:]...), nil | |||
} | |||
|
|||
// MarshalText is a compact textual representation of a topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test?
addressed comments |
libtalek/handle_test.go
Outdated
@@ -55,8 +55,8 @@ func TestSerialization(t *testing.T) { | |||
if err != nil { | |||
t.Fatalf("Could not deserialize: %v\n", err) | |||
} | |||
if !bytes.Equal(h.SharedSecret[:], h2.SharedSecret[:]) { | |||
t.Fatalf("serialization log info!") | |||
if !bytes.Equal(h.SharedSecret[:], h2.SharedSecret[:]) || !bytes.Equal(h.SigningPublicKey[:], h2.SigningPublicKey[:]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the Seeds? Also I noticed that handle.SeqNo is not included in the Marshalling. Is this an oversight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seeds have tests. seqNo is 0, in these tests so doesn't matter. The assumption is just that sscanf fills, i'm not convinced we gain much more confidence with the extra verbosity
libtalek/topic_test.go
Outdated
if err != nil { | ||
t.Fatalf("Could not deserialize: %v\n", err) | ||
} | ||
if !bytes.Equal(topic.SigningPrivateKey[:], clone.SigningPrivateKey[:]) || !bytes.Equal(topic.Handle.SharedSecret[:], clone.Handle.SharedSecret[:]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, we're missing checks on some fields here. It seems like the right thing to do is for the topic and handle to both have their own Equal() method
added an |
👍 |
one of two parts of #66