Simplified parameter verification for PrivateKey constructor

[adamantike]

It also fixes cases where supplied parameter is an empty string ("").

[sybrenstuvel]

I have a few issues with this pull request. First, I designed the code to accept any number you pass for exp1, exp2 and coef, or compute the correct values when you pass None. Your pull request changes this, and will also compute the values when you pass the number zero. I think this is okay, since actually storing zero will result in invalid operations later on anyway. However, such a change in semantics should be covered by one or more unit tests that check the new behaviour.

It also fixes cases where supplied parameter is an empty string ("").

An empty string is simply invalid for those parameters. The fix for this thus should be at the caller's side, which should be changed to pass a number instead. Please don't start accepting non-numeric types as valid values for numeric parameters.

[adamantike]

An empty string is simply invalid for those parameters. The fix for this thus should be at the caller's side, which should be changed to pass a number instead. Please don't start accepting non-numeric types as valid values for numeric parameters.

Existing code just silently accept non-numeric values for parameters:

>>> from rsa import key
>>> pk = key.PrivateKey(3727264081, 65537, 3349121513, 65063, 57287, exp1="", exp2=None, coef="asdf")
>>> pk.exp1
''
>>> pk.exp2
10095
>>> pk.coef
'asdf'

I agree that these fixes should be made at caller's side, but we can assert params and not accept invalid values.

Your pull request changes this, and will also compute the values when you pass the number zero. I think this is okay, since actually storing zero will result in invalid operations later on anyway.

It could be fixed by doing something like:

self.exp1 = exp1 if is_integer(exp1) else int(d % (p - 1))

So I think either asserting params or checking for integer_types would improve this PR.

[sybrenstuvel]

I agree that these fixes should be made at caller's side, but we can assert params and not accept invalid values.

I think that the code shouldn't be overcomplicated, and asserting for parameter type isn't really Pythonic. Also, I've never had a complaint that the range of accepted parameter types was too broad, so I don't see the concrete problem that you're trying to solve.

self.exp1 = exp1 if is_integer(exp1) else int(d % (p - 1))

This would silently ignore the value someone passes, which conflicts with the Zen of Python: "errors should never be silently ignored", as well as "explicit is better than implicit".

[adamantike]

What is the purpose of allowing custom exponents/coefficient, instead of using calculated values?

I am asking this because I am currently implementing primitives with CRT usage, and this feature makes we can't trust on exps/coeff, so these need to be recalculated when needed.

[sybrenstuvel]

My idea was to support key file formats that contain these values. Why bother recomputing them, when the values can be taken from the file?

[adamantike]

Maybe it's just me being paranoid, but it makes possible to create PrivateKey objects with incorrect exps/coeff values. I know I know, it is caller's responsability, but I would like to trust on those attributes for CRT-based decryption (and any other future implementations).

By recomputing them, we just add one calculation per key (when created by key file), but we save one calculation per decryption (CRT). Currently we save the initial calculation, but we should do one per decryption (because of exps/coeff validation).

I can think of the following alternatives:

1. Calculated values: Disallow custom exps/coeff values, and compute them. For key file import, we can use these values to check those provided by key file (and maybe warn user about inconsistency?). We can trust on exps/coeff for CRT-based primitives.
2. Trusting on caller's side: Keep PrivateKey creation the way it is. Caller's responsability, so we completely trust on key's exps/coeff. (Maybe warning the caller when it gives custom values?)
3. Exhaustic values recalculation: Keep PrivateKey creation the way it is. Recalculate exps/coeff values every time we need them.

I am between 1 because of mathematical correctness, and 2 because of trusted flexibility. 3 is a no-go for me. Also, I don't think we should rely just on one saved recalculation, when creating a new key through key file.

[sybrenstuvel]

You make some great points. I think that option 1 is a very good one.

[adamantike]

I'll create a new PR addressing this conversation. Closing this one.