Add (Half)SipHash implementation

[Entreco]

2nd attempt, to provide a (Half)SipHash implementation.
SipHash implements the Mac interface now.

I'm not really happy that the size of the key determines which algorithm is used.
Maybe we can make the distinction more explicit...

Looking forward to feedback/concerns/suggestions

[05nelsonm]

Did a very quick review. A couple of things that stick out:

1. Would suggest taking a look at other modules and their implementations. Trying to follow (roughly) how a project has things structured elsewhere is a courtesy which saves people (reviewers) time.
2. This implementation does not follow other Mac implementations whereby a separate class is utilized for each algorithm.
3. Tests do not follow structure of other modules which are all verified against Bouncycastle and are designed to properly exercise the implementation's internals.

public sealed class SipHash: Mac {
    // ...
}

public class SIPHASH: SipHash {
    // ...
}

public class HALF_SIPHASH: SipHash {
    // ...
}

EDIT: A good reference to utilize is the kmac module and how it is structured.

[05nelsonm]

must be copied, otherwise updating old SipHash will also affect new

[05nelsonm]

Use val and an appropriate sized ByteArray. removal and replacement is an incredible amount of unnecessary overhead.

[Entreco]

I don't fully understand what appropriately sized ByteArray means in this context. Any ByteArray could be used as an for the algorithm. One of the advantages of SipHash is that it's simpler and faster on short messages, so I expect that we should go for a relatively small ByteArray here?

[05nelsonm]

So I'm looking into things further regarding SipHash, and it seems like an array or a list is not even necessary to use.

As per the reference implementation, SipHash works with 64 bit words (a kotlin.Long) and HalfSiphHash algorithms work with 32 bit words (a kotlin.Int). The rounds then shift bits and update the state locally.

Can see this in BouncyCastle's implementation of SipHash and SipHash128

[05nelsonm]

All tests must implement the repository test abstraction. Also, the Jvm implementations are verified against BouncyCastle.

See the other modules' test implementations.

[Entreco]

Jep, I have one failure on the jvm, which indicates that there is something wrong with my update implementation.
Unfortunately, there is no HalfSipHash implementation on BouncyCastle (that I am aware)

[Entreco]

Thanks! And sorry for not following the existing structure, It was not trivial for me to determine how to structure it. I don't have any background in cryptography. Your comments steered me in the right direction.

I still have 1 failing jvm unit test tough, givenMac_whenUpdatedMedium_thenDoFinalReturnsExpected()

Expected :e635b1b821db25e1
Actual :fccb4afe00669767

My assumption is that something goes wrong when I update the Engine:

        override fun update(input: ByteArray, offset: Int, len: Int) {
            if (offset < 0 || len < 0 || offset > input.size - len) throw IllegalArgumentException("Bad arguments")
            inputs.addAll(offset, input.take(len))
        }

Because these update calls are not used in the provided data set. E.g. the other unit tests, only use mac.doFinal() and not mac().update().doFinal()

I realize that my approach with a MutableList<Byte> is not in line with the other implementations, which use a protected override val source: Digest. I tried to use one of the existing Digest implementations, but did not manage to get that working for now. Would be great if you can give me some pointers on that topic. I think I may have to rewrite the engines, to follow the update/dofinal/digest methods.

[05nelsonm]

I still have 1 failing jvm unit test tough, givenMac_whenUpdatedMedium_thenDoFinalReturnsExpected()

Expected :e635b1b821db25e1 Actual :fccb4afe00669767

My assumption is that something goes wrong when I update the Engine:

        override fun update(input: ByteArray, offset: Int, len: Int) {
            if (offset < 0 || len < 0 || offset > input.size - len) throw IllegalArgumentException("Bad arguments")
            inputs.addAll(offset, input.take(len))
        }

No checks are necessary with regard to the input. The Mac abstraction verifies all input for correctness before passing it off to Mac.Engine. It seems as though the issue lies in what is being utilized to hold state, a MutableList.

Because these update calls are not used in the provided data set. E.g. the other unit tests, only use mac.doFinal() and not mac().update().doFinal()

I realize that my approach with a MutableList<Byte> is not in line with the other implementations, which use a protected override val source: Digest. I tried to use one of the existing Digest implementations, but did not manage to get that working for now. Would be great if you can give me some pointers on that topic. I think I may have to rewrite the engines, to follow the update/dofinal/digest methods.

Correct, Digest is utilized for Hmac and Kmac algorithms, as per the spec. Following the reference implementation, there is not Digest utilized for this implementation.

[05nelsonm]

Another notable issue here are the naming conventions utilized; unsure how to square this circle.

Reference implementation states the following

SipHash-2-4-64
SipHash-2-4-128
HalfSipHash-2-4-32
HalfSipHash-2-4-64

which would suggest the following class names

public sealed class SipHash: Mac {
    // ...
}

public class SipHash64: SipHash {
    // ...
}

public class SipHash128: SipHash {
    // ...
}

public class HalfSipHash64: SipHash {
    // ...
}

public class HalfSipHash32: SipHash {
    // ...
}

Algorithm names would the follow that of the reference implementation based off of c and d arguments (default of 2, 4 respectively).

My only concern for the algorithm() function return values is that there are differences with what BouncyCastle is using.

• Provider
• SipHash
• SipHash128

I am more inclined to go with algorithm names based off of the actual reference implementation (I think BC has gotten it incorrect). As there is not actual FIPS spec for this, it's sort of up in the air.