RFC: Standardize API of all crypto/hashes packages

[mratsim]

As we're working with crypto at Status, we regularly get bitten by the different types in Nim libraries to represent binary data.

Context:

Crypto and hashing libraries in the wild uses arrays or seq of uint8, char, byte, object, uint32, cuchar and also strings to represent the binary hashes. I have noted the following issues:

A. Output hash type is inconsistent:

• array[N, char]:
  • nimSHA2
• array[N, uint8]:
  • md5
• object-wrapped array[bits div 8, uint8] (number of bits is part of the type):
  • keccak_tiny (Status)
  • nimcrypto (Status)
• array[N, byte]:
  • Eth-keys (Status)
• distinct array[N, uint32]:
  • SHA1
• string
  • libsodium
• ptr cuchar + len
  • nimssl (bindings autogenerated on install so not visible on Github)

B. 0.18 introduction of $ for arrays broke libraries:

• Inconsistent result between 0.17.2 and 0.18.0 jangko/nimSHA2#2
• Araq on IRC

C. Input type is inconsistent:

• a string (binary buffer)
  • nimSHA2
  • libsodium
• a hex string or a binary buffer (any kind: string, arrays, sequences)
  • keccak_tiny (Status) and
    its dependency memRange package
• a hex string
  • eth_keys (Status)
• ptr cuchar + len
  • nimssl (bindings autogenerated on install so not visible on Github)
• ptr uint8 + len
  • nimcrypto. Further overloads are coming.

D. $ Conversion of the hash type is inconsistent between string, hex or binary representation:

• nimSHA2 $ on array[N, char] (N = 28, 32, 48, 64 only) returns a binary string
  • every other size returns the string representation of the numbers
• libsodium also returns a binary string and requires bin2hex for the hex representation.
• keccak_tiny (Status) returns the hex representation
• md5 returns the hex representation
• nim-eth-keys (Status) uses the default $ for array of bytes (returns a string of uint8) and require toHex for the hex representation
• nimssl doesn't have $(it's a pointer). It requires toHex for the hex representation.

Note that we are currently having this discussion internally, but since Araq mentionned his pain working with hashes following 0.18, I believe the whole Nim ecosystem would benefit.

Stakes

Nimbus (A client for Ethereum 2.0) will bring a lot of attention to Nim crypto ecosystem and it's in our best interest to adopt healthy common standards.
Beyond the internal representations, I think a common set of APIs is what matters

Questions and considerations for API:

1. "String" input

proc hash1234(s: string): Hash = ..., what should s be, a hex string or binary data?

2. Variable size input

Which one(s) among

• proc hash1234(s: seq[uint8]): Hash = ...
• proc hash1234(s: seq[byte]): Hash = ...
• proc hash1234(s: seq[char]): Hash = ...
• proc hash1234(s: string): Hash = ...

3. $ behaviour

Which one among:

• An error
• The hex representation
• The uint8/byte numbers
• Convert to binary blob

4. Chaining

hash1234(hash1234(foo)) should be allowed without casting/conversion

5. Hex output

Which one among:

• $
• toHex

Opinions

For consistency one type between byte/uint8/char should be used.
At the same time one type between seq[byte]/seq[uint8]/seq[char] and string should be used.

Note: Those are only my opinions and does not represent the opinion of all Status devs

Opinion 1: $ is very error prone right now. From an ergonomic point of view, it makes sense to convert binary blob/hashes to their hex representation.

Opinion 2: $ converting to the string repr of seq[uint8] can be avoided by using distinct types or wrapping Hash = array[N, byte|uint8] in an object.

Opinion 3: The internal representation should be byte.

Rationale:

• byte is for data or binary blob that are read/written
• uint8 is for numbers (add, multiply, divide, modulo ...)
• char is for text that can be printed.

A counterpoint can be made that byte (or char) hides the actual size of the type.
However Windows and POSIX require 1 byte = 8-bit. The only systems with 1 byte != uint8 are DSP chips (they use 1 byte = 12, 16, 24 bit), see here.

Opinion 4: Let's nail it! 🎆

Inviting: @jangko, @cheatfate, @Araq, @zah, @yglukhov to the discussion.

[mratsim]

Mentionned by @arnetheduck , C++17 introduced a distinct type called byte

enum class byte : unsigned char {} ; (since C++17)

std::byte is a distinct type that implements the concept of byte as specified in the C++ language definition.

Like char and unsigned char, it can be used to access raw memory occupied by other objects (object representation), but unlike those types, it is not a character type and is not an arithmetic type. A byte is only a collection of bits, and only bitwise logic operators are defined for it.

[PMunch]

Yes! This has been annoying me for quite some while now. Another thing that should be taken into consideration is reading of data from a stream and writing to one. Whatever solution this comes to, the corresponding base type should be added to the streams module as well so as to facilitate easy input -> en-/decrypt -> output.

Personally I agree with the rationale offered, and while the DSP argument is valid Nim already has things like int which can take on multiple sizes. Doing the same for byte makes sense. I could write a crypto procedure that takes byte8 and the compiler will check that code using byte actually has the right sized byte.

[arnetheduck]

Another consideration for the API is streaming - for this it's customary to have a state that goes through init, update (many times) and finalization..

there can then be convenience helpers that call these three functions for non-streaming cases

[zielmicha]

I would vote for: proc hash1234(s: string): Hash = .... string in Nim (e.g. in stdlib) represents not only text, but also binary data.

Hash should be distinct array[byte, N] - byte is already defined as an alias to uint8. We should not worry about DSP architectures, it is very unlikely Nim will be used on one of these.

[dom96]

What's the rationale for having a distinct Hash type? Couldn't we simply get away with proc hash(s: string): string?

As far as representing binary data in Nim is concerned, I agree with @zielmicha, string should be used.

[jangko]

this byte vs uint8 vs char or string/seq problem is not exclusively belongs to crypto/hashes library. Graphics library also suffer from the same problem. How to consistently represent pixels data for graphics API.

let's say we already reach consensus here and adopt a common standard. but the problem is still there, the output from other library, e.g. output from graphics library we feed into crypto/hashes library still need go through some sort of conversion. this additional conversion step might not be acceptable under some circumstances.

therefore I suggest we could use Nim provided features such as generics or template to create a flexible API that capable of accepting various inputs and produce various outputs. I know this involved quite a lot of work and required more elaborate test, but at least it can prevent future war on types.

[PMunch]

@dom96 and @zielmicha, why reuse the string type for something that's not intuitively a string? Nim has a nice powerful type system, so no need to represent two different things with the same type. Differentiating them means that we could even do nice things like having a $ for byte data that outputs hexadecimal, and we can avoid passing a string to something that takes a byte array and vice-versa. I would say we should go the opposite direction and specify a bytes type alias in the standard library that is actually a seq[byte] or an openarray[byte] or even a concept of indexable container of bytes to really make it clear that it is the preferred way of representing byte data.

[zielmicha]

@PMunch Then you can't do hash123("hello world").

The underlying problem is that there is no definite distinction what is a binary string and what is a text string.

Python 3 has the same problem. Note how it needs separate stream types for text and binary IO.

[mratsim]

I agree with PMunch, it's not C, we shouldn't use char * (string) for data blob. I also like the $ outputting hex data by defaut for openarray[byte]

@jangko not ideal but casting works until everyone migrates to the same standard

Note that @zah started a library just to deal with this issue when receiving data from external sources: https://github.com/status-im/nim-ranges.

@zielmicha

proc hash123(s: openarray[byte]): array[N, byte] =
  ...

proc hash123(s: string): array[N, byte] =
  ...

proc hash123_fromHex(s: string): array[N, byte] =
  ...

Also maybe it would be worth it to introduce a hex package in the stdlib, with:
type HexString = distinct string

[PMunch]

@zielmicha basically what @mratsim said, but you could also have a converter from string to bytes to make strings work everywhere that bytes work, but still keep bytes a separate type. mratsims example of what multiple hash functions would look like looks shockingly similar to how Ada does it. The Bytes type there is declared in another part of the library, but that library implements all kinds of different crypto algorithms, something that Nim has in multiple different modules. Ada is known for it's type safety and I think taking a page from their book on this is a good way to go.

The underlying problem is that there is no definite distinction what is a binary string and what is a text string.

Couldn't agree more, and this is what we now have the possibility to add to Nim. By having a bytes type we can easily make the distinction clear, both to the programmer, and to the reader.

[mratsim]

@Araq feedback on IRC

input type: openArray[byte]
output type: array[X, byte] where X depends on the concrete hashing algorithm

[genotrance]

Just last night, I was thinking of writing a Nim API to abstract the C interop in nimssl which is a wrapper for Openssl. Right now, only the SHA and AES portions are wrapped.

Based on what is agreed here, accordingly the API can be adjusted. The underlying implementation returns an array of chars which I accordingly cast and then convert to a hex string.

[FedericoCeratto]

Mind adding https://nimble.directory/pkg/libsodium https://github.com/federicoceratto/nim-libsodium to the list?

[Araq]

Here is my proposal. A_ here stands for the concrete algorithm (SHA264, etc.).

type
  A_Digest* = distinct array[X, byte]
  A_Context* = object ## accumulator state
    # fields here

proc init*(a: var A_Context) = ...
proc update*(a: var A_Context; bytes: openArray[byte]) = ...
proc finish*(a: var A_Context): A_Digest = ...
proc toHex*(a: A_Digest; upperCase: bool = false): string = ...

This is the minimal protocol a hashing algorithm should support. The API introduces no overhead as far as I can see and it supports "streaming". On top of that either via a concept or via dynamic binding a dispatcher can be implemented, but first we also need a convenience template that is algorithm agnostic:

template hashBytes*(algorithm: typedesc; bytes: openArray[byte]; upperCase = false): string =
  var context: algorithm
  init(context)
  update(context, bytes)
  toHex(finish(context), upperCase)

And let's also have a toBytes view for string:

template toBytes*(x: string): seq[byte] = 
  cast[seq[byte]](x)

Thankfully seq[byte] can be passed to openArray[byte] without a copy.