Generic signature API #143

bwesterb · 2020-07-15T15:06:06Z

Used to easily add support for new (PQ) signature schemes to our fork of Go and cfssl.

bwesterb · 2020-07-15T15:39:54Z

Another reason is that I'm quickly prototyping integration into Go stdlib to find possible issues with the API. I found one already: we can't use PubicKeyAlgorithm() x509.PublicKeyAlgorithm in our interface for that would cause an import cycle.

bwesterb · 2020-07-22T09:55:22Z

@armfazh @claucece Have another look.

claucece · 2020-07-23T06:54:23Z

I'll check this tomorrow ;)

sign/api.go

sign/eddilithium3.go

claucece · 2020-07-24T06:02:18Z

I think it is looking good. Maybe we should be more clear on some names and all, though

sign/api.go

claucece · 2020-07-24T05:48:27Z

sign/api.go

+	// XXX add a (compile time?) lookup table
+	name = strings.ToLower(name)
+	for _, scheme := range schemes {
+		if strings.ToLower(scheme.Name()) == name {


Maybe we don't need to repeat here the 'ToLower'.. but maybe it will be a safe security measure...

If we would remove this ToLower() then we will never match any scheme that has an upper case character in the return of its scheme.Name().

Yes, but perhaps the scheme.Name() should always be enforced to be lower case... what do you think?

@armfazh Removed case insensitivity again. Did you do that on purpose @armfazh ?

sign/api.go

claucece · 2020-07-24T05:59:33Z

sign/eddilithium3.go

+func (s *edDilithium3Scheme) Sign(sk PrivateKey, message []byte,
+	opts *SignatureOpts) []byte {
+	sig := make([]byte, eddilithium3.SignatureSize)
+	if opts != nil && opts.Context != "" {


What if only opts.Context != ""?

I don't understand. For opts.Context to make sense, we must have opts != nil.

if any of the options is not used, then is ignored.

I don't understand — could you rephrase perhaps?

bwesterb · 2020-07-24T07:46:29Z

Maybe we should be more clear on some names and all, though

Sure, which?

armfazh · 2020-07-25T09:31:15Z

I just added a generic interface for four signatures, there are some methods marked with TODO.
Also I moved some methods into the circl/pki package.

claucece · 2020-07-27T07:16:04Z

Sure, which?

Let me thinking of a nice naming convention and propose something over here tomorrow.

sign/ed25519/signapi.go

sign/api/api.go

bwesterb · 2020-08-05T10:54:11Z

@armfazh

I dislike that SchemeByName is now under the github.com/cloudflare/circl/sign/api package.

I don't see a way to solve this without making something else ugly.

Providing a context to a scheme that does not support it is completely ignored. It should be an error.

I've changed that behaviour: provided a context where none is supported causes an error.

bwesterb · 2020-08-05T10:54:31Z

I've also removed the Hash from the SignatureOpts as it's not used — we can add it back in if we have a scheme that actually uses it.

bwesterb · 2020-08-05T11:16:18Z

@armfazh @claucece I've addressed all comments. Before starting with final polish (i.e. better documentation & examples), I think we still need to discuss some API issues:

Are we really happy with having sign package which defines Scheme and sign/api package which actually lists them? Isn't schemes perhaps a better name than api as the sign package itself defines the API. Then schemes.All() and scheme.ByName() would be nice to write.
Do we want Scheme names to be case insensitive? This was the case but @armfazh reversed it. This is required for easy integration with cfssl.
Doesn't ed25519 have a variant that support contexts? Should we add this to the Ed25519 scheme or have a separate Ed25519Ctx scheme? I guess it's best to have separate schemes for these variants — @claucece what do you think?

armfazh · 2020-08-05T17:45:14Z

Are we really happy with having sign package which defines Scheme and sign/api package which actually lists them? Isn't schemes perhaps a better name than api as the sign package itself defines the API. Then schemes.All() and scheme.ByName() would be nice to write.

Agree on s/api/schemes/

Do we want Scheme names to be case insensitive? This was the case but @armfazh reversed it. This is required for easy integration with cfssl.

I don't have a strong opinion on this. Feel free to bring back that code.

Doesn't ed25519 have a variant that support contexts? Should we add this to the Ed25519 scheme or have a separate Ed25519Ctx scheme? I guess it's best to have separate schemes for these variants — @claucece what do you think?

That variant would be another named scheme Ed25519Ctx, which is separated from the current Ed25519 scheme.

I've also removed the Hash from the SignatureOpts as it's not used — we can add it back in if we have a scheme that actually uses it.

I added that the Hash field to make the structure compatible with the crypto.SignerOpts. So, we can receive a crypto.SignerOpts, and internally convert it to a different type.

I dislike that SchemeByName is now under the github.com/cloudflare/circl/sign/api package.

I also didn't like it at all. But, that's the way I found for avoiding circle dependencies. Renaming sign/api to sign/schemes can alleviate a bit its usage.

armfazh · 2020-08-05T17:48:22Z

sign/ed25519/signapi.go

+	"github.com/cloudflare/circl/sign"
+)
+
+var Scheme sign.Scheme = &scheme{}


I don't understand why to take the address of a zero-sized value. You can implement the interface with scheme and not with *scheme.

Under the hood an instance of an interface is always a pair of a pointer and a concrete type, so instantiating it with a pointer is closer to what really happens. The assembly generated btw is exactly the same.

The difference is on the assembly calls.
call AX vs call "".(*Impl2).A(SB)
one call is with a register and the other is with a pointer.

I am not convinced why to use pointer when they are not needed.

Oops, I simplified my example too much.

Take a look at this one

Creating an instance of an interface with either a empty struct or a pointer to an empty struct results in the pointer runtime.zerobase. (The code of alloc1 and alloc2 is the same. This time I checked properly.)

The generic way to run a function on an interface involves a virtual call (call ax). So interestingly the Go compiler optimised the call with the pointer receiver, but not the call with the value receiver. I don't see a reason why it couldn't optimise the one with the value receiver as well and perhaps it will in the future.

So using pointers or not does not actually change anything when using empty structs (and the calling code doesn't know the types.) And if it does it optimizes the pointer case better. That's not the reason I use pointers though — as I said I prefer putting them here as it's closer to what Go actually does under the hood: an instance of an interface is always a pointer.

So interestingly the Go compiler optimised the call with the pointer receiver, but not the call with the value receiver.

I want to learn more about this.

So using pointers or not does not actually change anything when using empty structs.

Agree.

sign/sign.go

bwesterb · 2020-08-06T10:29:15Z

Agree on s/api/schemes/

Done. Updated this branch, Go fork and cfssl fork.

I added that the Hash field to make the structure compatible with the crypto.SignerOpts. So, we can receive a crypto.SignerOpts, and internally convert it to a different type.

The type of Scheme.Sign is Sign(sk PrivateKey, message []byte, opts *SignatureOpts) []byte so that wouldn't accept a crypto.SignerOpts anyway now. (Note that there are two sign functions: one on the scheme and one on the PrivateKey — the one on the private key does accept a crypto.SignerOpts but does not allow for the advanced options like Context.)

armfazh · 2020-08-06T17:21:07Z

the one on the private key does accept a crypto.SignerOpts but does not allow for the advanced options like Context.)

I had the same intuition, but here is the trick. Even if we announce to receive a crypto.SignerOpts, we could pass a Sign.SignatureOpts (which must have field Hash) and internally convert the parameter into a Sign.SignatureOpts struct. (I learn this trick from @claucece)

claucece · 2020-08-11T07:22:41Z

Are we really happy with having sign package which defines Scheme and sign/api package which actually lists them? Isn't schemes perhaps a better name than api as the sign package itself defines the API. Then schemes.All() and scheme.ByName() would be nice to write.

Yes!

Do we want Scheme names to be case insensitive? This was the case but @armfazh reversed it. This is required for easy integration with cfssl.

Mmm... It will be better case insensitive.

Doesn't ed25519 have a variant that support contexts? Should we add this to the Ed25519 scheme or have a separate Ed25519Ctx scheme? I guess it's best to have separate schemes for these variants — @claucece what do you think?

There are two: the ctx and the ph. The schemes should be there on the ed25519 API.

claucece · 2020-08-11T07:23:30Z

I had the same intuition, but here is the trick. Even if we announce to receive a crypto.SignerOpts, we could pass a Sign.SignatureOpts (which must have field Hash) and internally convert the parameter into a Sign.SignatureOpts struct. (I learn this trick from @claucece)

Exactly! Then, the "Golang-way" API does not change ;)

claucece · 2020-08-11T07:25:17Z

@bwesterb check what I did with the ed448 and ed25519 API, specially the usage of this: https://github.com/cloudflare/circl/blob/master/sign/ed25519/ed25519.go#L69

bwesterb · 2020-08-12T17:31:15Z

I had the same intuition, but here is the trick. Even if we announce to receive a crypto.SignerOpts, we could pass a Sign.SignatureOpts (which must have field Hash) and internally convert the parameter into a Sign.SignatureOpts struct. (I learn this trick from @claucece)

Yes, that's for a method on the private key, but here we're dealing with a top level function.

(For the Sign method on the private key it's actually a bit tricky. If we use the same type for an ed25519 and ed25519ctx private key, then we can't know whether it was used for the scheme ed25519 or ed25519ctx. I'd like to propose to fix this in a coming pull request where we add schemes for the variants of ed*.)

bwesterb · 2020-08-12T17:34:00Z

@armfazh @claucece I'm happy with the PR as it is now (assuming we'll be able to make changes to the API before we tag it.) Do you agree?

claucece · 2020-08-17T17:20:38Z

(For the Sign method on the private key it's actually a bit tricky. If we use the same type for an ed25519 and ed25519ctx private key, then we can't know whether it was used for the scheme ed25519 or ed25519ctx. I'd like to propose to fix this in a coming pull request where we add schemes for the variants of ed*.)

Mmm.. ok.

Do you agree?

Sure!

armfazh

It seems ready to go.

pki/pki.go

Co-authored-by: Armando Faz <armfazh@users.noreply.github.com>

bwesterb · 2020-08-19T09:29:58Z

Thank you, I merged it. I created an issue ( #147 ) to track desired improvements to this API.

bwesterb force-pushed the signapi branch 4 times, most recently from 42fc6ae to 1e2cb7f Compare July 22, 2020 09:54

bwesterb force-pushed the signapi branch 9 times, most recently from 29ed92c to 0ca32e0 Compare July 22, 2020 22:14

armfazh reviewed Jul 24, 2020

View reviewed changes

sign/api.go Outdated Show resolved Hide resolved

armfazh reviewed Jul 24, 2020

View reviewed changes

sign/api.go Outdated Show resolved Hide resolved

sign/api.go Outdated Show resolved Hide resolved

sign/eddilithium3.go Outdated Show resolved Hide resolved

sign/eddilithium3.go Outdated Show resolved Hide resolved

sign/eddilithium3.go Outdated Show resolved Hide resolved

claucece reviewed Jul 24, 2020

View reviewed changes

bwesterb and others added 4 commits July 25, 2020 02:24

WIP signature API

750f8cd

API example for eddilithium3.

50efdbf

Adding the remainder signatures.

309e00a

Adding the pki package.

354d6d5

armfazh force-pushed the signapi branch from c2c76d2 to 354d6d5 Compare July 25, 2020 09:26

bwesterb commented Jul 29, 2020

View reviewed changes

sign/ed25519/signapi.go Outdated Show resolved Hide resolved

bwesterb commented Jul 29, 2020

View reviewed changes

sign/api/api.go Outdated Show resolved Hide resolved

Use context properly; remove unused hash

2a6949c

bwesterb added 3 commits August 5, 2020 12:59

Check for PEM block types and don't include scheme name in public key

46dbc61

deduplicate structure

ef376fe

satisfy linter

6564028

armfazh reviewed Aug 5, 2020

View reviewed changes

sign/sign.go Show resolved Hide resolved

rename sign/api to sign/schemes

3f119a5

bwesterb force-pushed the signapi branch from 67a9e26 to 3f119a5 Compare August 6, 2020 10:21

bwesterb mentioned this pull request Aug 11, 2020

Add (more generic) support for Circl signature schemes cloudflare/go#25

Merged

bwesterb added 2 commits August 12, 2020 19:20

schemes.ByName: make case insensitive

e93ed24

Use Fail instead of FailNow

f3c2794

bwesterb changed the title ~~[WIP] signature API~~ Generic signature API Aug 12, 2020

armfazh approved these changes Aug 18, 2020

View reviewed changes

pki/pki.go Outdated Show resolved Hide resolved

pki/pki.go Show resolved Hide resolved

pki/pki.go Outdated Show resolved Hide resolved

bwesterb and others added 2 commits August 19, 2020 11:03

Update pki/pki.go

96ed392

Co-authored-by: Armando Faz <armfazh@users.noreply.github.com>

comments armfazh

f629445

bwesterb merged commit 0c3b4ec into master Aug 19, 2020

bwesterb deleted the signapi branch August 19, 2020 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic signature API #143

Generic signature API #143

bwesterb commented Jul 15, 2020 •

edited

bwesterb commented Jul 15, 2020

bwesterb commented Jul 22, 2020

claucece commented Jul 23, 2020

claucece commented Jul 24, 2020

claucece Jul 24, 2020

bwesterb Jul 24, 2020

claucece Jul 27, 2020

bwesterb Aug 5, 2020

claucece Jul 24, 2020

bwesterb Jul 24, 2020

armfazh Jul 25, 2020

bwesterb Aug 5, 2020

bwesterb commented Jul 24, 2020

armfazh commented Jul 25, 2020

claucece commented Jul 27, 2020

bwesterb commented Aug 5, 2020

bwesterb commented Aug 5, 2020 •

edited

bwesterb commented Aug 5, 2020 •

edited

armfazh commented Aug 5, 2020

armfazh Aug 5, 2020

bwesterb Aug 5, 2020

armfazh Aug 5, 2020

bwesterb Aug 6, 2020

armfazh Aug 6, 2020

bwesterb commented Aug 6, 2020

armfazh commented Aug 6, 2020

claucece commented Aug 11, 2020

claucece commented Aug 11, 2020

claucece commented Aug 11, 2020

bwesterb commented Aug 12, 2020

bwesterb commented Aug 12, 2020

claucece commented Aug 17, 2020

armfazh left a comment

bwesterb commented Aug 19, 2020

Generic signature API #143

Generic signature API #143

Conversation

bwesterb commented Jul 15, 2020 • edited

bwesterb commented Jul 15, 2020

bwesterb commented Jul 22, 2020

claucece commented Jul 23, 2020

claucece commented Jul 24, 2020

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

bwesterb commented Jul 24, 2020

armfazh commented Jul 25, 2020

claucece commented Jul 27, 2020

bwesterb commented Aug 5, 2020

bwesterb commented Aug 5, 2020 • edited

bwesterb commented Aug 5, 2020 • edited

armfazh commented Aug 5, 2020

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

bwesterb commented Aug 6, 2020

armfazh commented Aug 6, 2020

claucece commented Aug 11, 2020

claucece commented Aug 11, 2020

claucece commented Aug 11, 2020

bwesterb commented Aug 12, 2020

bwesterb commented Aug 12, 2020

claucece commented Aug 17, 2020

armfazh left a comment

Choose a reason for hiding this comment

bwesterb commented Aug 19, 2020

bwesterb commented Jul 15, 2020 •

edited

bwesterb commented Aug 5, 2020 •

edited

bwesterb commented Aug 5, 2020 •

edited