Improve handling of native dependencies

[joshuagl]

Please fill in the fields below to submit a pull request. The more information
that is provided, the better.

Fixes issue: #179 (partial fix, securesystemslib.gpg still needs to be addressed)

Description of the changes being introduced by the pull request:

• Makes colorama optional, add a warning to the log when it's not available but otherwise ssl functions as normal
• rename pyca_crypto_keys -> rsa_keys
• By default fallback to pure Python ed25519 certificate verification when the nacl module isn't available, rather than making the use of nacl an option at function call time.
• Ensures the public facing modules interface and keys are importable even when the native dependencies are not available
• Ensure that public functions using features of the native dependencies provide an actionable error message when they can't be used (due to the absence of native dependencies)

Please verify and check that the pull request fulfils the following
requirements:

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[joshuagl]

I need to figure out how to cleanly test interface and keys without native dependencies installed.

Does it make sense to define a different test suite with a test for the public interfaces that verifies that all public functions either return successfully or return the expected UnsupportedLibraryError, we could then have a separate Tox environment without the native dependencies installed that runs this suite.

Is there a cleaner/simpler way to do it?

[joshuagl]

I've got a partial implementation of a mechanism to test the public interfaces in 0ca75ef, does that look reasonable to you @lukpueh? If so I'll finish off that patch (it's currently incomplete and has some hard-coded paths).

[lukpueh]

Thanks, @joshuagl! This looks very reasonable to me.

[lukpueh]

So I just looked closer at your changes and they still seem very sensible. Thanks a lot, @joshuagl!

I realized that the name of pyca_crypto_keys.py is quite misleading. It suggests that it is securesystemslib's interface to pyca/cryptography, although other modules (ecdsa_keys.py, hash.py) also interface that library. Maybe it would be clearer and more accurate to call it rsa_keys.py, this would also make it consistent with the other low-level modules in securesystemslib, which both encapsulate key algorithms and not crypto libraries.

Another thing that I noticed is that pynacl import error handling is repeated on two levels of abstractions, i.e. in keys.py and in ed25519_keys.py. Do you think we could do it only once? Maybe we can draw the line that I described in #179 (comment) one level of abstraction further down. That is e.g.:

# in ed25519_keys.py

NACL = True
try:
  import nacl

except ImportError:
  NACL = False

# ed25519_keys.sign() will raise an error if nacl does not exist
def sign():
  if not NACL:
    raise UnsupportedLibraryError()
  else: 
    nacl.ed25519_sign()

# ed25519_keys.verify() has a pure-python fallback. 
def verify():
  if not NACL:
    pure_python_ed25519_verify()
  else: 
    nacl.ed25519_sign()

This would make keys.py completely oblivious to the whole import story. And we could do the same thing for ecdsa_keys.py and rsa_keys.py (sic!), albeit without the pure-python fallback.

In your approach, OTOH, I see the advantage of failing early, i.e. raising an UnsupportedLibraryError before doing unnecessary work in any of the functions in keys.py.

I hope you don't mind my brainstorming here. I really appreciate your efforts and am curious about your thoughts. (cc @SantiagoTorres).

[lukpueh]

Btw. since securesystemslib.gpg requires gpg and pyca/cryptography I also mentioned it in #179. But I suggest that we deal with it in a separate PR.

[joshuagl]

Thanks for the constructive feedback @lukpueh

I realized that the name of pyca_crypto_keys.py is quite misleading. It suggests that it is securesystemslib's interface to pyca/cryptography, although other modules (ecdsa_keys.py, hash.py) also interface that library.

Would you like to see that rename here, or in a separate PR?

Another thing that I noticed is that pynacl import error handling is repeated on two levels of abstractions, i.e. in keys.py and in ed25519_keys.py. Do you think we could do it only once? Maybe we can draw the line that I described in #179 (comment) one level of abstraction further down. That is e.g.:

# in ed25519_keys.py

NACL = True
try:
  import nacl

except ImportError:
  NACL = False

# ed25519_keys.sign() will raise an error if nacl does not exist
def sign():
  if not NACL:
    raise UnsupportedLibraryError()
  else: 
    nacl.ed25519_sign()

# ed25519_keys.verify() has a pure-python fallback. 
def verify():
  if not NACL:
    pure_python_ed25519_verify()
  else: 
    nacl.ed25519_sign()

This would make keys.py completely oblivious to the whole import story. And we could do the same thing for ecdsa_keys.py and rsa_keys.py (sic!), albeit without the pure-python fallback.

In your approach, OTOH, I see the advantage of failing early, i.e. raising an UnsupportedLibraryError before doing unnecessary work in any of the functions in keys.py.

I was torn on this. Doing the import handling a level lower has the benefits of:

• not handling the import at two levels
• keeping the code in the public interfaces simpler and moving the complexity (or at least ugly code) to the lower level modules

One downside to the approach is that the unit tests become more complex, as in many places we'll need to provide valid (enough) inputs to the higher-level functions so that the "unnecessary work" can be completed before the UnsupportedLibraryError is thrown by the lower level wrappers.

I hope you don't mind my brainstorming here. I really appreciate your efforts and am curious about your thoughts. (cc @SantiagoTorres).

I don't mind at all. I very much appreciate the constructive discussion around these changes.
I think the more effort/cleaner high-level interfaces approach is the better one.

Assuming you agree, I'll revise this PR to:

• move the pynacl import error handling only to the lower-level modules
• rename pyca_crypto_keys.py -> rsa_keys.py

I'll also take a look at adding similar logic to securesystemslib.gpg, depending on how much work it looks to be I may handle it here or in a separate PR.

Does that seem like a reasonable approach?

[joshuagl]

I didn't get around to addressing securesystemslib.gpg yet, but I think the rest of this PR is in reasonable shape for review.

[lukpueh]

Excellent work, @joshuagl. Thanks a lot for taking a stab at this task. I have left a few very minor comments. Let me know when you want me to take another look.

[lukpueh]

It would be cool to configure coverage to exclude these kinds of branches from py{27, 35, 36, 37, 38} only, and do the inverse for purepy{27, 36}. Do you want to look into it? We can also ticketize it.

[joshuagl]

This is technically easily handled thanks to the versatility of coverage[1], but I held off on doing it because I wasn't sure how valuable coverage results would be for the purepy environments - at least in the current setup with only check_public_interfaces.py run in the purepy Tox environments we won't cover much of the code at all.

[lukpueh]

I was thinking of a setup with mutual exclusive coverage exemption. Something like a # pragma: purepy, to mark lines/branches that are excluded from py{27, 35, 36, 37, 38}, but are also the only lines/branches considered for purepy{27, 38}, so that all lines must be tested by either one or the other. But let's do this in a separate PR (I'll add a note on #179).

[joshuagl]

Thanks for the review @lukpueh. I think I've addressed all of your review comments except configuring coverage differently in the purepy case. I'd appreciate your thoughts on that one, I commented in-line.

[lukpueh]

Thanks for addressing all the comments! LGTM.