Smartcard support #38

ale-linux · 2023-11-08T07:54:33Z

This PR implements the protocol from https://eprint.iacr.org/2023/853. The protocol is between 3 parties, a user, a smartcard and a verifier, where user and smartcard can jointly convince a verifier they posses a tuple of attributes, one of which is only known by the smartcard. In the idemix implementation, the smartcard is a NymSigner and the user just produces an idemix signature against the nym generated by the smartcard. The only difference is that - contrary to the traditional idemix signature - the user does not know the usk (used by the smartcard/NymSigner). The protocol adapts the ZKP to cater for this lack of knowledge.

This PR implements the protocol from https://eprint.iacr.org/2023/853. The protocol is between 3 parties, a user, a smartcard and a verifier, where user and smartcard can jointly convince a verifier they posses a tuple of attributes, one of which is only known by the smartcard. In the idemix implementation, the smartcard is a NymSigner and the user just produces an idemix signature against the nym generated by the smartcard. The only difference is that - contrary to the traditional idemix signature - the user does not know the usk (used by the smartcard/NymSigner). The protocol adapts the ZKP to cater for this lack of knowledge. Signed-off-by: Alessandro Sorniotti <[email protected]>

adecaro · 2024-06-11T11:23:06Z

bccsp/handlers/nym.go

@@ -160,7 +160,10 @@ func (i *NymPublicKeyImporter) KeyImport(raw interface{}, opts bccsp.KeyImportOp

 	pk, err := i.User.NewPublicNymFromBytes(bytes)
 	if err != nil {
-		return nil, err
+		pk, err = i.User.NewPublicNymFromBytes(append([]byte{04}, bytes...))


can you add a comment to clarify why appending that magic number?
Can you put it in a const?

Good point. So, there are several issues here at play. I'll try to summarise them here and maybe you can give me your take on how to best document it? So:

the NymPublicKeyImporter does not have a translator to handle the various byte representations of group elements. Instead, it directly passes the bytes to the underlying implementation without any translation. This works well most of the times since actually for most curves, the serialisation of a group element is a fixed-size array of the two coordinates. However some curves use an encoding where the constant byte value 0x04 is prefixed to the two coordinates to signal that the point is uncompressed.

it is meaningful to add the translator, but I would do it in a separate change

prefixing 0x04 cannot turn a bad point marshalling into a good one anyway, unless it's the encoding issue that we are handling.

Wdyt?

I would add in the comments points 1 and 2, and then open a github issue for the translator 👍

adecaro · 2024-06-11T11:24:02Z

bccsp/handlers/nymsigner.go

+	// handle the smartcard case
+	if signerOpts.IsSmartcard {
+		if s.SmartcardNymSignatureScheme == nil {
+			return nil, fmt.Errorf("smartcard mode is unsupported")


let's always use "github.com/pkg/errors", please.

Actually that package has been sunset. Afaik the new go error design prescribes the use of fmt.Errorf and the %w modifier. See this thread hyperledger/fabric#4461

I mentioned this because in some other places "github.com/pkg/errors" is used.
Then, a possible solution would be to create a local errors package with aliases to the functions you want to use. Then, the code can be refactored to use those aliases.
Up to you :)

I think the "right" choice would be to just follow the new go error proposal and start using just fmt. There are other active forks of "github.com/pkg/errors", that's not even the issue. The issue is rather that if the proper way of handling errors is another one, we shouldn't use them. Let me investigate a bit more, but I think for now we can leave it as is?

adecaro · 2024-06-11T11:25:38Z

bccsp/schemes/aries/signer.go

@@ -50,7 +54,15 @@ func (s *Signer) getPoKOfSignature(

 	messagesFr := credential.toSignatureMessage(sk, s.Curve)

-	pokOS, err := bbs12381g2pub.NewBBSLib(s.Curve).NewPoKOfSignature(signature, messagesFr, revealedAttributesIndex(attributes), ipk)
+	var pokOS *bbs12381g2pub.PoKOfSignature
+	if sigtype == types.Smartcard {


could you add a comment to explain the difference between the two cases?

adecaro · 2024-06-11T11:54:48Z

bccsp/schemes/aries/smartcard.go

+
+func (sc *Smartcard) NymEid() (*math.Zr, *math.G1) {
+	// increment counter
+	sc.ctr++


Just to make sure: Smartcard should not be assumed to be thread-safe. Is this written anywhere?

ah so this code is just a smartcard emulator to test the e2e without any separate hardware. How do you think we should capture that?

actually - a clarification: the signing part of that function is just a mock - because signing actually needs to be performed by the smartcard. The verification part instead is meaningful and used - see also next comment.

Then, please could you a comment to the Smartcard struct to mention that it is used as an emulator?

adecaro · 2024-06-11T11:59:21Z

bccsp/handlers/nymsigner.go

@@ -35,6 +32,32 @@ func (s *NymSigner) Sign(k bccsp.Key, digest []byte, opts bccsp.SignerOpts) ([]b
 		return nil, errors.New("invalid issuer public key, expected *issuerPublicKey")
 	}

+	// handle the smartcard case
+	if signerOpts.IsSmartcard {


this part of the code is not tested, can it be without an actual smartcard?

yeah exactly, that's tricky. This particular code is only a wrapper to call

https://github.com/IBM/idemix/blob/smartcard-squash/bccsp/schemes/aries/smartcard.go#L116

It's difficult to test that from the bccsp layer since it would require the creation of a mock bccsp smartcard signer. Otoh we do test it from here using the emulated signing.

actually I'm sorry - I wrote this code a while back so I forgot: I double-checked and we DO test this - just not from this package: In particular, this line

https://github.com/IBM/idemix/blob/smartcard-squash/bccsp/smartcard_test.go#L264

ends up testing exactly this code.

perfect 👍

adecaro · 2024-06-11T12:04:28Z

go.mod

@@ -3,9 +3,9 @@ module github.com/IBM/idemix
 go 1.19


what about moving to 1.21. Go 1.19 is deprecated

good point - yes. Given that I have one more functional PR on top of this one, would it be okay to merge this, the next, and then upgrade to go 1.21?

adecaro · 2024-06-11T12:06:27Z

bccsp/types/idemixopts.go

+	IsSmartcard bool
+
+	// Smartcard is a software smartcard used to support signing in s/w
+	Smartcard interface{}


is interface{} the best we can have? It might cause issues with serialisation

So I double-checked (to find out the correct answer to #38 (comment)) and in doing so I established that we use this only in testing to simulate a smartcard nym signer backed by the CSP in order to then be able to test the nym verifier of smartcard-created nym signatures without the actual smartcard. So this will never be set in production.

perfect 👍

I added a comment though

Signed-off-by: Alessandro Sorniotti <[email protected]>

adecaro

LGTM

Signed-off-by: Alessandro Sorniotti <[email protected]>

adecaro

LGTM

ale-linux force-pushed the smartcard-squash branch from 45af305 to 4016d78 Compare January 24, 2024 16:54

ale-linux force-pushed the smartcard-squash branch from 4016d78 to ceb4d6c Compare May 28, 2024 11:11

ale-linux assigned adecaro May 28, 2024

ale-linux changed the title ~~WIP - Smartcard support~~ Smartcard support May 28, 2024

adecaro reviewed Jun 11, 2024

View reviewed changes

Add comment

e1384b2

Signed-off-by: Alessandro Sorniotti <[email protected]>

adecaro self-requested a review June 12, 2024 05:49

adecaro previously approved these changes Jun 12, 2024

View reviewed changes

This was referenced Jun 12, 2024

Add translator object to NymPublicKeyImporter #42

Open

Bump go version #43

Open

More comments

b5320f0

Signed-off-by: Alessandro Sorniotti <[email protected]>

ale-linux dismissed adecaro’s stale review via b5320f0 June 12, 2024 07:07

adecaro self-requested a review June 12, 2024 07:23

adecaro approved these changes Jun 12, 2024

View reviewed changes

adecaro merged commit 114d281 into main Jun 12, 2024
2 checks passed

adecaro deleted the smartcard-squash branch June 12, 2024 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smartcard support #38

Smartcard support #38

ale-linux commented Nov 8, 2023 •

edited

Loading

adecaro Jun 11, 2024

ale-linux Jun 11, 2024

adecaro Jun 12, 2024

ale-linux Jun 12, 2024

adecaro Jun 11, 2024

ale-linux Jun 11, 2024

adecaro Jun 12, 2024

ale-linux Jun 12, 2024

adecaro Jun 11, 2024

ale-linux Jun 11, 2024

adecaro Jun 11, 2024

ale-linux Jun 11, 2024

ale-linux Jun 11, 2024

adecaro Jun 12, 2024

ale-linux Jun 12, 2024

adecaro Jun 11, 2024

ale-linux Jun 11, 2024

ale-linux Jun 11, 2024

adecaro Jun 12, 2024

adecaro Jun 11, 2024

ale-linux Jun 11, 2024

adecaro Jun 12, 2024

adecaro Jun 11, 2024

ale-linux Jun 11, 2024

adecaro Jun 12, 2024

ale-linux Jun 12, 2024

adecaro left a comment

adecaro left a comment

Smartcard support #38

Smartcard support #38

Conversation

ale-linux commented Nov 8, 2023 • 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

Choose a reason for hiding this comment

adecaro left a comment

Choose a reason for hiding this comment

adecaro left a comment

Choose a reason for hiding this comment

ale-linux commented Nov 8, 2023 •

edited

Loading