Method SetIntString in the Scalar interface to create arbitrary large scalars from strings

[pmcampones]

Hello all,

I added the method SetIntString(v string) (Scalar, error) in the Scalar interface and implemented it on the structs that satisfy Scalar.

My reasoning was that I needed to use a Secret Sharing protocol and was having a fair bit of trouble creating a scalar with the value of the secret. In the vss_test.go tests, the secret is a random value, and there is a method just to create a random Scalar; however, to create a deterministic value, I either had to use SetInt64 or SetBytes.
With SetInt64, the number I set could have at most 64 bits.
With SetBytes, to generate a number I would have to pass its representation as a byte array. I found this hard to accomplish.

To make SetIntString I simply used big.Int's method to convert the string into a big.Int and then used what was already available to convert the big.Int into a Scalar.

I tested the changes in vss_test.go, function TestDeterministicStringSecret. It repeats tests that were already done but with an hardcoded secret instead of a random value.
I had to refactor the code a little to avoid repeating code, but nothing major. All tests are still running fine.

Thanks

[CLAassistant]

All committers have signed the CLA.

[github-actions]

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

[AnomalRoil]

Thanks for the extra tests! Overall no specific code comment.

I'm not 100% sure I see the value of having the extra function in the Scalar interface, since it'll force everyone to implement it.

Did you have a specific usecase where you needed it to be part of the interface?

[AnomalRoil]

I guess my main worry with making the Scalar interface any bigger is that it'll break all implementations using custom scalars.

That being said we're en route to V4 so we could do it... Opinions @pierluca ?

[pierluca]

I share your concern, @AnomalRoil . Could you elaborate a little bit more about the use case, @pmcampones ?

[ineiti]

It says "string encoded integer value", which definitely looks like it should be done using SetInt64. Other possibilities I see:

• SetBigInt which could use more than 64 bits
• SetBytes which already exists :)

So I suppose this should be something like SetInt64(strconv.Atoi()) or use the SetBytes for bigger integers.

[pmcampones]

Maybe SetBigInt would be a better name for the method.
In the comment thread bellow we discussed using only SetBytes instead of the SetIntString.
I find using SetIntString a bit more convenient and less error prone than using SetBytes.

[AnomalRoil]

Looking at this implementation I don't see why the code couldn't live in your own codebase as a helper function: you don't seem to need any internal or non-exported function to achieve this, no?

Like you could do something like:

func NewIntString(v string) (kyber.Scalar, error) {
	bigV := new(big.Int)
	bigV, ok := bigV.SetString(v, 0)
	if !ok {
		return nil, fmt.Errorf("unable to set string number: %v", v)
	}
	i := new(mod.Int)
	return i.Init(bigV, yourModulus), nil
}

assuming you work with a fixed modulus, no?

[pmcampones]

Thanks for the reply!

I agree that touching the interface is contentious. And I didn't even consider the fact that other people using Kyber could have their own Scalar implementations, which now would have to be changed.

The mod.Int struct is public and has the methods to create a scalar from any pair of big ints, as seen in Init.
However this is not possible for the remaining implementations of Scalar.
For instance, I needed to use the scalar struct in the edwards25519.scalar file. The functionalities of the edwards curve require that specific implementation of the scalar.
The methods associated with this curve only require the Scalar interface in their signature. However, in the implementation of the methods a downcast is made to force a specific Scalar implementation.

I suppose the bigger problem lies in having methods requiring specific Scalar implementations, contrary to what the signature indicates. But just adding the SetIntString method was my quick and easy solution for the problem.

We could have the SetIntString method just in the edwards25519.scalar, instead of all scalars. It wouldn't force a change in the interface on one hand, on the other it would reveal more of the internals of edwards25519, because now the user that wants to create a scalar must know that specific implementation is used.
I'm not sure which solution is better. Placing the method just in edwards25519 could be less of an hassle for people that have their own Scalar implementations, but changing the interface would be cleaner for people (like me) who are just using the Scalar types provided.

[pierluca]

I understand your argument and I'm open to discussing it, albeit I would need a very convincing use case. At the moment, I don't see why the SetBytes function wouldn't be sufficient to develop the kind of auxiliary functions that you're referring to.

[pmcampones]

I agree that there is nothing you can't do with SetBytes. I just find the SetIntString more user friendly.
Someone that doesn't know that big.Int has a method called Bytes would be stumped trying to compute the byte array representation of a large number.

On that note, the Bytes array on big.Int is big endian, while the default byte order of the kyber scalars are little endian.
I think this is bound to cause some bugs.
This is compounded by the fact that different Scalar implementations handle endianess differently. For instance, the mod.Int SetBytes implementation inverts the byte array if the default endian order is Little Endian; while the edwards25519 SetBytes implementation just assumes the byte array received comes in the correct order.
A user that makes auxiliary functions using SetBytes must read the different implementations and decide whether or not to invert the array depending on the specific Scalar implementation to be used.

Finally, while its not a problem in my use case, I think that using the Bytes method in big.Int and SetIntBytes of Scalar implementations might cause some performance hiccups.
I did no benchmark so this is just speculation.
With SetIntBytes we have a slowish operation for converting a string into a big.Int, and fast operations to use the contents of the big.Int in the Scalar.
With an auxiliary function using SetBytes, we would first have to make a big.Int from a string, then convert it to a byte array representation, only to then call SetBytes which would do the opposite, converting the byte array representation into a large number.

[ineiti]

As others have said, I also think this is a very specific use-case and should be handled locally in your code. I think a SetBigInt could be useful, if others would agree. But doing the decimal-to-BigInt conversion in all curves seems wrong: why not hexadecimal-to-BigInt? Or octal-to-BigInt?

[pmcampones]

As the commit stands currently, the SetIntString method does not need the argument to be decimal. It can be decimal, hexadecimal, octal or even binary, as long as it has the appropriate prefix.

[ineiti]

OK, I didn't see it uses big.Int.SetString. There's still two things that bother me:

• Scalar should be more low level than handling strings
• if we add a SetBigInt, how should a Scalar which is not built on big.Int handle this? I think currently all scalars are based on big.Int, but I'm not sure.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[sonarqubecloud]

Please retry analysis of this Pull-Request directly on SonarCloud