dedis / kyber

Advanced crypto library for the Go language
Other
636 stars 168 forks source link

NewKey #205

Closed ineiti closed 6 years ago

ineiti commented 7 years ago

From @nikkolasg I recall now that NewKey was previously in Suite and so I naturally shifted it into Group since Suite does not exist anymore (sigh). if we really want it out of the Group interface, we thought with @ineiti of different solutions:

  1. move the bit twiddling thing in the NewKeyPair method, where a manual switch to check if the Suite is ed25519 and in that case, apply the bit twiddling which would be implemented in the key package (where the KeyPair is defined).
  2. move out the functionality of creating a "key" into a kyber.KeyFactory functionality. That way, NewKeyPair can check if the group given implements KeyFactory, if yes, then call group.NewKey otherwise simply create a regular Scalar. The actual bit twiddling would stay in the ed25519 package.
ineiti commented 7 years ago
  1. doesn't make sense to me, as the user would have to define which KeyFactory to use, with combinations that would not make sense. I think 1. is the most appropriate action to take here. Even though I'd like to keep the information on how to generate secure keys in the group itself.
nikkolasg commented 7 years ago

as the user would have to define which KeyFactory to use

No the user don't choose anything, it can just checks if the "suite" it has chosen implements this interface or not. It's like CipherFactory: the user does not choose it, it just checks if it is implemented.

ineiti commented 7 years ago

Now I'm confused - I thought that if a user includes CipherFactory in his suite-definition, he has to implement the method defined in CipherFactory. So in the same way, if he defines KeyFactory, he has to define how to implement the method NewKeyPair. What am I missing?

nikkolasg commented 7 years ago

It's not what I understood and the way I did at least. If group/edwards25519.Suite implements CipherFactory, the user can use this because it implements his suite-definition. If edwards25519 does not implement CipherFactory, then the user won't be able to use it, in fact it'll probably won't even compile!

ineiti commented 7 years ago

OK, I see - that means that the key-generation is still in group/edwards, but will not be be picked up if the user only uses the group...

If we want to separate it, I'd prefer to implement it in NewKeyPair.

But for the moment only Bryan wants to separate it ;) But as he has +1000 voting-power, I vote for NewKeyPair, else it's not a clean separation.

nikkolasg commented 7 years ago

but will not be be picked up if the user only uses the group...

What do you mean by that ? On a higher level, this question only arises because of the weirdness of ed25519. Every other groups from what I know don't have this co-factor / sub group problem and treat keys and scalar equally. It's a pity.. :/

ineiti commented 7 years ago

picked up -> exported as methods

nikkolasg commented 7 years ago

Ok so let me propose something in the middle:

Basically combining both approaches so it's transparent to the user (the user won't even realize that) and at the same time keep the specific code in the correct package, while still allowing maybe new weird elliptic curves/group to define their own way of creating a "key". What do you think ?

ineiti commented 7 years ago

I'll leave this one to Bryan. Personally I'll leave NewKey in Group. But if Bryan wants to move it out, I'd put it into NewKeyPair. Else it's only removed from the interface, but not from the ed25519 itself. And I'd keep the magic method-implementation to onet/cothority ;)

nikkolasg commented 7 years ago

I don't understand what your comments says about the approach I just proposed, which uses NewKeyPair as the "deciding" party to see if there is a special way of generating a key, while still keeping the special way in the correct belonging package. You don't want that ? You're okay-ish ? Or you're OK ? :D

jeffallen commented 6 years ago

My plan is to have this in kyber/util/key.go:

type Generator interface {
  NewKey(cipher.Stream)
}

and then in NewKeyPair, check if suite implements Generator and if true, then use it, otherwise do the default thing.

nikkolasg commented 6 years ago

Just an additional remark / historical note on this topic:

Ideally, this kind of structure, and even the code NewKeyPair should be at the top root level. When I tried to do this, I ran into the cyclic import problem for testing the functionality. Basically kyber.ǸewKey(edwards25519) imports edwards25519 which imports kyber.