Cipher suite interface to replace Kyber by Gilthoniel · Pull Request #592 · dedis/onet

Gilthoniel · 2019-10-28T10:09:07Z

This adds a cipher suite interface that will be used instead of
Kyber abstractions to allow any kind of asymmetric cryptography.

It also implements the interface by using the Ed25519 scheme
provided by the language.

Related to #579

This adds a cipher suite interface that will be used instead of Kyber abstractions to allow any kind of asymmetric cryptography. It also implements the interface by using the Ed25519 scheme provided by the language. Related to #579

Gilthoniel · 2019-10-28T10:09:46Z

First reduced PR for #587

ciphersuite/ciphersuite.go

tharvik · 2019-10-28T11:58:05Z

ciphersuite/ciphersuite.go

+}
+
+// NewRawPublicKey returns an instance of a public key.
+func NewRawPublicKey(name Name, data []byte) *RawPublicKey {


As every call on RawPublicKey do not modify it, you can use a value receiver.

I'll see with the other engineers but some guidelines tell you to use the pointer receiver everywhere when some need for consistency.

That was my guideline - but I have been proven wrong ;) We were trying to get away from this for v3, but then couldn't. So if you still think it's wrong to always use pointer structs in New methods, please go ahead and open an issue for v4 to actually return the more appropriate structures.

One place where it will collide with a lot of code is the 'old' protobuf-structures, where the pointer indicates an optional value.

ciphersuite/ciphersuite.go

ciphersuite/ed25519.go

ciphersuite/registry.go

nkcr

I like this ciphersuite package, thanks for the nice job! :)
Nice unit tests too 👍

nkcr · 2019-10-28T13:24:50Z

ciphersuite/ciphersuite.go

+// registered using the name of the suite.
+//
+// Public keys and signatures may need to be transmitted over the network and
+// interfaces cannot be used as is. That is why every the different elements


Suggested change

// interfaces cannot be used as is. That is why every the different elements

// interfaces cannot be used as is. That is why every different elements

nkcr · 2019-10-28T13:25:01Z

ciphersuite/ciphersuite.go

+
+// encodedNameLengthSize defines the size in bytes of the name length
+// when marshaling cipher data.
+const encodedNameLengthSize = 32 / 8


ok, but why 32?

Something I wanted to discuss. 8 or 16 bits integer is probably big enough for a cipher name.

We'll see tomorrow morning

Or you can use binary.Uvarint, to pack tighter, reduce code complexitiy and have unlimited size.

nkcr · 2019-10-28T14:04:49Z

ciphersuite/ciphersuite.go

+	size, err = w.Write([]byte(d.Name()))
+	n += int64(size)
+	if err != nil {
+		return n, xerrors.Errorf("writing name: %v", err)


Not fan of those shortened error messages... Makes our program stammers the error message instead of having a comprehensive-nicely-readable description. :/

it's only an opinion, nothing has to be changed

Well the fact that we are wrapping the errors create a chain that should explain what happened. Like here a calling function would have something like marshaling cipher data: writing name: ... so you know what went wrong.

But I'm curious to know what you would have written.

I look where the error comes from and explain it:

"failed to write Name as bytes: %v", err

nkcr · 2019-10-28T14:28:37Z

ciphersuite/ed25519.go

+	"golang.org/x/xerrors"
+)
+
+const errNotEd25519CipherSuite = "invalid cipher suite"


Shouldn't those be public? like this an external package can assert for this type of error

No it was only to avoid repetitive string values but for the moment I don't think it's necessary. We can always change that later.

btw, if you want to have it public, have it as an error, so that errors.Is can be used directly.

nkcr · 2019-10-28T14:46:03Z

ciphersuite/ed25519.go

+}
+
+func (s *Ed25519CipherSuite) unpackSecretKey(sk SecretKey) (*Ed25519SecretKey, error) {
+	if data, ok := sk.(*RawSecretKey); ok {


a personal opinion that I wanted to express for a moment: I don't like those if with short statement. IMO each line of code should do one thing, otherwise that makes the code harder to read. (same for the named return)

I think this kind of ok conversion is quite frequent in Go

I had to get used to them, too... But I saw even worse (don't remember where):

if ok := some_method; !ok { }

nkcr · 2019-10-28T14:47:05Z

ciphersuite/ed25519.go

+		return signature, nil
+	}
+
+	return nil, xerrors.New("wrong type of signature")


that breaks our philosophy of "fail first" that we tend to use

nkcr · 2019-10-28T15:01:21Z

ciphersuite/registry.go

+		return nil, xerrors.Errorf("cipher suite: %v", err)
+	}
+
+	pk, err := c.PublicKey(raw)


Can't we pass only raw.Data? At this stage we are sure to have the correct cipher suite, so the check raw.Name() != s.Name() inside the PublicKey() function looks superfluous. Or I am missing a case?

nkcr · 2019-10-28T15:08:01Z

ciphersuite/registry.go

+// Verify takes a public key, a signature and a message and performs a verification
+// that will return an error if the signature does not match the message. It
+// will also return an error if the cipher suite is unknown.
+func (cr *Registry) Verify(pk PublicKey, sig Signature, msg []byte) error {


just a thought, shouldn't we differentiate the case where the signature is incorrect from the one where an error occurred in the program? It feels a bit strange that the verification outputs an error while it only says that a signature is invalid.
Maybe it's better to rename the function IsValid(...) and return a (bool, error). IDK

Gilthoniel · 2019-10-29T11:56:22Z

On hold during the TLS related investigation.

cgrigis · 2019-10-30T11:22:33Z

ciphersuite/registry.go

+	}
+}
+
+// RegisterCipherSuite stores the cipher if it does not exist. It returns the


Suggested change

// RegisterCipherSuite stores the cipher if it does not exist. It returns the

// RegisterCipherSuite stores the cipher if it does not exist. It returns

cgrigis · 2019-10-30T11:26:58Z

ciphersuite/registry.go

+func (cr *Registry) RegisterCipherSuite(suite CipherSuite) CipherSuite {
+	name := suite.Name()
+	if suite := cr.ciphers[name]; suite != nil {
+		// Cipher suite already registered so we return it so it can be reused.


Isn't it surprising for the caller to receive a suite different that the provided one, with no error?

nkcr · 2020-10-27T09:03:55Z

This PR is celebrating it's first year tomorrow 🎂 😰
Should we consider merging it ? It would require a bit of effort to address some of the comments, anyone's up to do that ?

ineiti · 2020-10-27T09:14:13Z

I propose to close this issue without merging for the following reasons:

I think it solves the wrong problem - see my comment in the related issue.
there already is a way to have independent keypairs for each service. So if anything, this interfaces should be extended, and not a new added.
it was originally thought to help implement Drynx / Spindle from the LDS, but finally they used their own keypair implementation

Cipher suite interface to replace Kyber

cb98361

This adds a cipher suite interface that will be used instead of Kyber abstractions to allow any kind of asymmetric cryptography. It also implements the interface by using the Ed25519 scheme provided by the language. Related to #579

Gilthoniel self-assigned this Oct 28, 2019

Wrap missing errors using xerrors

ffd8249

dedis deleted a comment Oct 28, 2019

tharvik reviewed Oct 28, 2019

View reviewed changes

tharvik's comments

7b7d7d3

Gilthoniel force-pushed the ciphersuite_579 branch from 81867ec to 7b7d7d3 Compare October 28, 2019 13:00

dedis deleted a comment Oct 28, 2019

Fix a comment

b41d7fe

nkcr suggested changes Oct 28, 2019

View reviewed changes

cgrigis reviewed Oct 30, 2019

View reviewed changes

	// interfaces cannot be used as is. That is why every the different elements
	// interfaces cannot be used as is. That is why every different elements

	// RegisterCipherSuite stores the cipher if it does not exist. It returns the
	// RegisterCipherSuite stores the cipher if it does not exist. It returns

Conversation

Gilthoniel commented Oct 28, 2019

Uh oh!

Gilthoniel commented Oct 28, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkcr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gilthoniel commented Oct 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkcr commented Oct 27, 2020

Uh oh!

ineiti commented Oct 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants