Implementation of the cipher suite registry #587

Gilthoniel · 2019-10-15T12:01:59Z

Add a ciphersuite package that contains the abstraction of the cryptographic primitives.
Add a builder.go file that contains the implementation of a server builder.
Remove the public API to create a server.
Remove the service factory to have a non-static service registration through the builder.

Closes #579

The registry will be used to replace the Kyber primitives so that Onet can have access to a wider range of cipher suite at runtime.

This changes the service factory to remove the static registry and have instead a registry per server.

ineiti · 2019-10-21T07:18:05Z

Is there a document describing what you're doing here?

Gilthoniel · 2019-10-22T10:44:53Z

Question:

Rename ciphersuite to something shorter ? csuite ?

Gilthoniel · 2019-10-22T10:45:28Z

Follow-ups (marked as TODO in the code):

server.go : it's using an online service to verify the connection or find the host
builder.go : better check of the parameters
service.go : database name mixed between the builder and old logic
simulation.go : better use of the builder if possible

ineiti

If you want to have my opinion on this PR, then you will need to split it into different PRs. As far as I can see (I might be completely wrong), you're doing quite a lot in this PR (perhaps I should not throw this stone - but I think I got better on this front). Too much for me, too many parts of onet touched/changed. The last 20-something files are not even shown in the git-overview...

There is quite a lot of changes in the services and messages that I don't understand and where I don't find anything in the README. I did make some comments in there, but I have no idea what is going on.

With regard to the cipher suite registry, I don't see how you want to make this work with either the kyber or the lattigo implementation. I would like to have an example of both of these in the ciphersuite_test.go, I think that would also help you to improve the interface. I'm sure it's possible, but I don't see how. Also, please make sure that your new interfaces make sense and discuss with Bryan, Philipp, and Christian.

Also, the UnsecureCipherSuite sounds like a really bad idea to me. People might use it. It doesn't show how to use it in a given setting. I would prefer to have something like Ed25519CipherSuite or so, which is kind of a simple suite that should give you all you need. But having a Signature method that puts the message in the signature is really bad precedence for people trying to learn what you're doing... Again, they might just copy it and use it for something critical.

My proposition to go on with this PR is to focus on the cipher suite registry and undo all other changes to the service and message-passing that are not directly linked to the cipher suite registry.

Anyway - my 2 cents.

ineiti · 2019-10-24T09:14:27Z

README.md

+creating a service to extend the base interface and force the registration of the
+service with this extended cipher suite:
+
+```go


I would prefer seeing the actual cipher suite definition with some explanation here, and then how to extend it...

ineiti · 2019-10-24T09:16:07Z

ciphersuite/ciphersuite.go

+
+// CipherData is a self-contained message type that can be used
+// over the network in the contrary of the interfaces.
+type CipherData struct {


Why don't you use the same system as with the kyber suite binaryMarshaller? It reminds me of https://xkcd.com/927/

ineiti · 2019-10-24T09:16:58Z

ciphersuite/ciphersuite.go

+}
+
+func (d *CipherData) String() string {
+	buf := append([]byte(d.Name), d.Data...)


For a string I would do something like

fmt.Sprintf("%s: %x", d.Name, d.Data)

makes it a little bit nicer...

ineiti · 2019-10-24T09:19:26Z

ciphersuite/ciphersuite.go

+// MarshalText implements the encoding interface TextMarshaler so that
+// it can be serialized in format such as TOML.
+func (d *CipherData) MarshalText() ([]byte, error) {
+	name := []byte(d.Name)


If it ends up in a toml file it will be read by persons, so I'd prefer to have a clear-text representation of the Name. So I would rather use String() here.
Also, I don't see why you want to add the size here?

ineiti · 2019-10-24T09:20:54Z

ciphersuite/ciphersuite.go

+
+// Packable provides the primitives necessary to make network
+// messages out of interfaces.
+type Packable interface {


How does this work together with the current way of registering the interfaces to the protobuf package?

ineiti · 2019-10-24T09:25:49Z

ciphersuite/ciphersuite.go

+
+	// Sign must produce a signature that can be validated by the
+	// associated public key of the secret key.
+	Sign(sk SecretKey, msg []byte) (Signature, error)


Not sure: is this enough for lattice-based crypto? Should there be a more generic interface?

ineiti · 2019-10-24T09:26:09Z

ciphersuite/ciphersuite.go

+	// Verify must return nil when the signature is valid for the
+	// message and the public key. Otherwise it should return the
+	// reason of the invalidity.
+	Verify(pk PublicKey, signature Signature, msg []byte) error


Same thing here: are there more parameters needed in the case of lattigo?

ineiti · 2019-10-24T09:27:32Z

ciphersuite/unsecure_csuite.go

+
+const nonceLength = 16
+
+// UnsecureCipherSuiteName is the reference name for the cipher suite


If this is only used for testing, then why isn't it in _test.go?

Why is it unsecure? Why do you need an unsecure suite?

ineiti · 2019-10-24T09:31:56Z

ciphersuite/unsecure_csuite.go

+// Sign takes a secret key and a message and returns the signature of
+// the message that can be verified by the associated public key.
+func (c *UnsecureCipherSuite) Sign(sk SecretKey, msg []byte) (Signature, error) {
+	if len(msg) == 0 {


Most often you don't sign the message, but you sign the hash of the message. As sha256([]byte{}) is defined, I think it's wrong to return an error here.

I would at least use sha256 here in the 'signature'. Then you have a constant length signature, which is much more accurate, even for a UnsecureCipherSuite.

ineiti · 2019-10-24T09:37:52Z

overlay.go

 	// messages going to protocol instances
 	c.RegisterProcessor(o,
 		ProtocolMsgID,     // protocol instance's messages
 		RequestTreeMsgID,  // request a tree
 		ResponseTreeMsgID, // send a tree back to a request
-		RequestRosterMsgID,


Where did these messages go to? Are they not needed anymore?

tharvik · 2019-10-24T09:45:13Z

app/server.go

+	if host != "" {
+		builder.SetHost(host)
+	} else {
+		builder.SetHost("0.0.0.0")


DefaultAddress?

Also, it gets overwriten by next if host == "".

tharvik · 2019-10-24T09:53:00Z

ciphersuite/ciphersuite.go

+	"golang.org/x/xerrors"
+)
+
+var sizeLength = 32 / 8


const, not very descriptive name.

tharvik · 2019-10-24T09:57:15Z

ciphersuite/ciphersuite.go

+	Signature() Signature
+
+	// KeyPair must return a random secret key and its associated public key.
+	KeyPair() (PublicKey, SecretKey)


It's an action, not a value, GenRandomKeyPair?

tharvik · 2019-10-24T10:08:45Z

ciphersuite/registry.go

+func (cr *Registry) KeyPair(name Name) (PublicKey, SecretKey) {
+	c, err := cr.get(name)
+	if err != nil {
+		panic(err)


Why panic? The others func of this API are returning error when something is not found.

tharvik · 2019-10-24T10:12:24Z

ciphersuite/registry.go

+// will also return an error if the cipher suite is unknown.
+func (cr *Registry) Verify(pk PublicKey, sig Signature, msg []byte) error {
+	if pk.Name() != sig.Name() {
+		return xerrors.New("mismatching cipher names")


Hum, you can use import "errors" directly now (Errorf is now part of "fmt").

I think cothority still wants to keep backward-compatibility with 1.12, which has no xerrors in the stdlib yet.

tharvik · 2019-10-24T11:10:32Z

tree.go

 	// List is the list of actual entities.
-	List      []*network.ServerIdentity
-	Aggregate kyber.Point
+	List []*network.ServerIdentity


type Roster []*network.ServerIdentity

tharvik · 2019-10-24T11:12:22Z

tree.go

-	return r
+// Len returns the length of the roster.
+func (ro *Roster) Len() int {
+	return len(ro.List)


Why? It's shorter to write len() and avoid the call.

tharvik · 2019-10-24T11:17:22Z

tree.go

 		if err != nil {
-			return RosterID{}, xerrors.Errorf("marshaling: %v", err)
+			// An error at that point would mean something worst is
+			// happening, so we keep the function simple and panic.


Or, we can have a RosterBuilder which populate the ID field when building :P

tharvik · 2019-10-24T11:19:42Z

treenode.go

+	data := n.Host().ServerIdentity.ServicePublic(name)
+	pk, err := n.overlay.cr.UnpackPublicKey(data)
+	if err != nil {
+		log.Error(err)


log.Panicf

tharvik · 2019-10-24T11:20:58Z

treenode.go

+	if err != nil {
+		log.Error(err)
+		panic("Couldn't unpack the public key of the distant node. Please check " +
+			"the of the server.")


"Sure, I'll check the of the server."

This improves the code to prevent mixing cipher data between different element types.

Gilthoniel · 2019-10-28T10:02:38Z

I heard your thoughts! Thanks, I'm going to create multiple small PRs with independent modules and a last one to make them work together.

Related to dedis/onet#587

Implementation of the cipher suite registry

4343398

The registry will be used to replace the Kyber primitives so that Onet can have access to a wider range of cipher suite at runtime.

Gilthoniel self-assigned this Oct 15, 2019

Gaylor Bosson added 6 commits October 16, 2019 16:43

Adapt the packages to use ciphersuite instead of kyber

1d788e4

Refactor service factory to be instantiated by server.

34a3cf9

This changes the service factory to remove the static registry and have instead a registry per server.

Use the new builder to make servers and remove other public APIs

c6f5b28

Add comments to the builders

4cb149c

Unit tests for the ciphersuite package

c8c2664

Unit tests for the builders

8713c63

Gilthoniel force-pushed the crypto_interfaces_579 branch from 83c1d77 to 8713c63 Compare October 18, 2019 12:37

Extend ciphersuite and builder documentation

3f5bff0

Gaylor Bosson added 4 commits October 21, 2019 16:29

Improve support to adapt blscosi from cothority

4a3abad

Registered cipher suite in the service constructor

f632216

Extend the readme to add ciphersuite and builder explanations

f230e79

Get public keys from a roster

da062ce

ineiti requested changes Oct 24, 2019

View reviewed changes

tharvik reviewed Oct 24, 2019

View reviewed changes

Gaylor Bosson added 2 commits October 24, 2019 16:19

Use specific raw types per element

35239d6

This improves the code to prevent mixing cipher data between different element types.

Implement an Ed25519 cipher suite to replace the unsecure one

9ec1d63

dedis deleted a comment Oct 28, 2019

Gilthoniel mentioned this pull request Oct 28, 2019

Cipher suite interface to replace Kyber #592

Open

Gilthoniel pushed a commit to dedis/cothority that referenced this pull request Oct 28, 2019

Adaptation of the byzcoinx package for Onet

8122a47

Related to dedis/onet#587

Gilthoniel force-pushed the crypto_interfaces_579 branch from 069d10e to 9ec1d63 Compare December 4, 2019 08:12

Gilthoniel closed this Mar 24, 2020

ineiti deleted the crypto_interfaces_579 branch March 24, 2020 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of the cipher suite registry #587

Implementation of the cipher suite registry #587

Gilthoniel commented Oct 15, 2019 •

edited

Loading

ineiti commented Oct 21, 2019

Gilthoniel commented Oct 22, 2019

Gilthoniel commented Oct 22, 2019 •

edited

Loading

ineiti left a comment •

edited

Loading

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

ineiti Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

ineiti Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

tharvik Oct 24, 2019

Gilthoniel commented Oct 28, 2019


		const nonceLength = 16

		// UnsecureCipherSuiteName is the reference name for the cipher suite

Implementation of the cipher suite registry #587

Implementation of the cipher suite registry #587

Conversation

Gilthoniel commented Oct 15, 2019 • edited Loading

ineiti commented Oct 21, 2019

Gilthoniel commented Oct 22, 2019

Gilthoniel commented Oct 22, 2019 • edited Loading

ineiti left a comment • 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

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

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

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

Gilthoniel commented Oct 28, 2019

Gilthoniel commented Oct 15, 2019 •

edited

Loading

Gilthoniel commented Oct 22, 2019 •

edited

Loading

ineiti left a comment •

edited

Loading