Remove custom PrivateKey exponents/coefficient

[adamantike]

As discussed in #50, it removes option for custom exps/coef on PrivateKey constructor. This way, we can trust on these values for CRT-based primitives and any other future implementation.

RFC state because I would like to discuss what's the best solution for PKCS#1 key file imports, and not-so-small repos depend on this (boto/boto, aws/aws-cli, google/oauth2client, ... ;) ).

Alternatives I can think of:

1. Overwrite file exps/coef (currently implemented): It just drops key file exps/coef, and recompute them for the new PrivateKey instance. Mathematically correct, but it could theoretically change behaviour for malformed key files (actually, we currently don't use these values, so I think it's safe).
2. Overwrite file exps/coef, and warn user: Same as 1, and we also warn user about malformed key file (incorrect exps or coeff). Question is, how we warn them? (print(), log... ?)
3. Use file exps/coef: Change calculated values to those provided by key file. It uses incorrect values if present, so I discard this option.

[coveralls]

Coverage decreased (-0.06%) to 91.677% when pulling 9e936e8 on adamantike:no-custom-privatekey-exps into cf124d3 on sybrenstuvel:master.

[coveralls]

Coverage decreased (-0.06%) to 91.677% when pulling 8608c3b on adamantike:no-custom-privatekey-exps into cf124d3 on sybrenstuvel:master.

[sybrenstuvel]

Given those choices, I'd say choice 2, with a log.warning() call to warn about mismatches.

Alternatively we could use the warning module... The downside is that it states: "Repetitions of a particular warning for the same source location are typically suppressed." -- this is not something that we want. We want to warn about every malformed key. On the other hand, it does allow programmatic testing of whether a warning occurred, which makes it possible to let the application itself (rather than the user) know about this malformed key. With logging this is also possible, but I think using the logger to communicate this type of information is Quite Bad Design.

What do you think @adamantike ?

[adamantike]

I completely agree, we have to warn the user about every malformed key, not just the first time. And I think stderr is appropiate for this instead of hiding these warnings in a logfile. I can update this to use warnings with the always filter.

However, I'm in favor of adding the checking in the _load_pkcs1_der() function instead of the constructor, as it's not longer a constructor responsibility.

[coveralls]

Coverage decreased (-0.06%) to 91.856% when pulling ba9db55 on adamantike:no-custom-privatekey-exps into 9f57740 on sybrenstuvel:master.

[coveralls]

Coverage remained the same at 91.914% when pulling 0b9241b on adamantike:no-custom-privatekey-exps into 9f57740 on sybrenstuvel:master.

[coveralls]

Coverage remained the same at 91.914% when pulling 1322cc1 on adamantike:no-custom-privatekey-exps into 9f57740 on sybrenstuvel:master.

[coveralls]

Coverage remained the same at 91.914% when pulling b0bb1c8 on adamantike:no-custom-privatekey-exps into 9f57740 on sybrenstuvel:master.

[sybrenstuvel]

However, I'm in favor of adding the checking in the _load_pkcs1_der() function instead of the constructor, as it's not longer a constructor responsibility.

+1

[adamantike]

@sybrenstuvel this is ready for review! I added mock as a dependecy for tox, as it can be really useful for our test suite (e.g. existing monkey-patching for prime number generation)

[coveralls]

Coverage remained the same at 91.914% when pulling c666c4d on adamantike:no-custom-privatekey-exps into 9f57740 on sybrenstuvel:master.

[sybrenstuvel]

I don't think this is something a library should do, or is it? I don't have much experience with the warnings module, but this looks like changing global configuration. If that's indeed the case, we shouldn't (it should be left to the application using our library).

[adamantike]

Nice catch!! I will change it to set this just in the test

[sybrenstuvel]

Please use unittest.mock. mock is an implementation backported to Python 2.

[adamantike]

We need compatibility with Python 2, and unittest.mock doesn't provide that to us. According to mock documentation:

mock is now part of the Python standard library, available as unittest.mock in Python 3.3 onwards. However, if you are writing code that runs on multiple versions of Python the mock package is better, as you get the newest features from the latest release of Python available for all Pythons.

[sybrenstuvel]

Ok, we leave it in then ;-)

[coveralls]

Coverage decreased (-0.01%) to 91.905% when pulling db9bd13 on adamantike:no-custom-privatekey-exps into dc57888 on sybrenstuvel:master.