Support additional SSH key algorithms

[gabi-250]

This introduces an additional "catch-all" key type to ssh-key to support additional SSH key algorithms, as described in #135.

I recommend reviewing this PR commit by commit (I've split the change into 3 logical chunks to make it easier to review):

* ssh-key: create `Keypair` and `KeyData` variants for custom algorithms.

   Adds the `Keypair::Other` and `KeyData::Other` variants for
   storing the key material of keys that use a custom algorithm.

   Adds the `OpaqueKeypair` and `OpaqueKeyData` types for representing keys
   meant to be used with an algorithm unknown to this crate (e.g. custom
   algorithm). They are said to be opaque, because the meaning of their
   underlying byte representation is not specified.

* ssh-key: add a catch-all variant to Algorithm.

  Adds a new `Algorithm::Other` variant for representing additional
  algorithms.

  Breaking changes: `Algorithm::as_str`, `Algorithm::as_certificate_str`
  now return `&str` instead of `&static str`.

 * ssh-key: define a type for custom algorithm names.

  Adds an `AlgorithmName` type for additional algorithm names. The syntax
  for additional algorithm names is described in [section 6 of RFC4251].

  Introduces a dependency on `tinystr`. Using `tinystr::TinyAsciiStr` for
  the representing algorithm names enables `AlgorithmName` to be `Copy`.

Let me know if you disagree with my approach, I'm happy to rework this if needed.

Closes #135

[tarcieri]

I'd prefer not to add additional dependencies unless absolutely necessary.

[gabi-250]

That is understandable. I jotted down some alternatives in my other comment: #136 (comment)

[tarcieri]

Since the Other variants are gated on alloc anyway, another approach would be to store the algorithm name along with the opaque data, rather than in the AlgorithmName::Other variant. That way you could just use a String.

[gabi-250]

Thank you for the quick review!

Since the Other variants are gated on alloc anyway, another approach would be to store the algorithm name along with the opaque data, rather than in the AlgorithmName::Other variant. That way you could just use a String.

I had considered this possibility, but there were a few snags:

• we still need an Algorithm::Other variant for the new KeypairData::Other and KeyData::Other variants (both KeyData and KeypairData have an algorithm(&self) -> Algorithm method)
• Algorithm::Other needs to know its string representation (because of its as_str and as_certificate_str implementations). We could:
  1. make Algorithm::Other wrap the algorithm name and certificate string identifier (Algorithm::Other { name: String, cert_str: String }). However, this would make Algorithm be non-Copy (which I was trying to avoid), or
  2. make Algorithm::Other a unit variant (this way Algorithm can stay Copy), and
     • remove the Algorithm::as_str method
     • give KeyData an algorithm_str method: for the old variants algorithm_str would return the existing algorithm name constants, whereas for the Other variant it would return the String stored in KeyData
     • replace all .algorithm().as_str() calls with .algorithm_str()

The first option made most sense to me, and since I wanted to keep Algorithm Copyable I decided to use tinystr instead of String. That being said, I understand your concern about introducing additional dependencies: if you're alright with Algorithm not being Copy anymore, I can push another commit that removes the tinystr dependency. Alternatively, I can implement the second option (or if you have a different idea, I would be more than happy to implement that instead).

[tarcieri]

Okay, yes, that's a problem.

Perhaps you can make AlgorithmName an array + length internally, rather than using tinystr.

Alternatively I guess it's fine to drop the Copy bound on Algorithm and use a String.

[gabi-250]

Okay, yes, that's a problem.

Perhaps you can make AlgorithmName an array + length internally, rather than using tinystr.

Sounds good! 8d73988 removes the tinystr dependency.

I also moved AlgorithmName to its own module (f676aa5) and fixed the failing clippy lint (98de060).

Let me know if you'd like me to squash any of these changes before you re-review.

[tarcieri]

My gut feeling looking at this, and AlgorithmName in particular, is that it would be significantly simpler to replace that with something like Box<str> or String, e.g. Algorithm::Other(String) or perhaps pub type AlgorithmName = Box<str>. Since all of this functionality is bounded on alloc for other reasons, we can take advantage of that dependency here.

The Copy bound for Algorithm isn't particularly important. It was just convenient to support at the time. Now it's introducing some incidental complexity.

The drawback of using a stack-allocated string type is it makes Algorithm significantly larger just to support the Other variant, whereas with Box<str> or String it would store pointer+len(+capacity).

[gabi-250]

My gut feeling looking at this, and AlgorithmName in particular, is that it would be significantly simpler to replace that with something like Box<str> or String, e.g. Algorithm::Other(String) or perhaps pub type AlgorithmName = Box<str>. Since all of this functionality is bounded on alloc for other reasons, we can take advantage of that dependency here.

The Copy bound for Algorithm isn't particularly important. It was just convenient to support at the time. Now it's introducing some incidental complexity.

The drawback of using a stack-allocated string type is it makes Algorithm significantly larger just to support the Other variant, whereas with Box<str> or String it would store pointer+len(+capacity).

Good point! Thank you for bearing with me.

I made Algorithm non-Copy (df169eb), and turned the two inner components of AlgorithmName (the certificate_str and id, the algorithm name identifier) into Strings (38b8189).

I also reverted 2bd496b, which is no longer necessary.

[tarcieri]

Thanks!