C++ Crypto: Part 2- HMAC

Question

Looking around for modern Crypto libraries.
Could not find anything good.

I know I probably did this all wrong so work with me here. There will be four different reviews for four structures that build on each other:

1. Hashing
2. Hashed Key
3. Password Key
4. Salted Challenge Response

The data structures and implementation presented in these questions are based on RFC2104 and this post on codeproject.

This review is for an implementation of HMAC. This is a technique for hashing password using a Key.

Usage Exmple:

Digest<HMac<Sha1>>      digest;
HMac<Sha1>              hasher;

hasher.hash("This is the Key", "This is the message", digest);

hmac.h

#ifndef THORS_ANVIL_CRYPTO_HMAC_H
#define THORS_ANVIL_CRYPTO_HMAC_H

#include "hash.h"

// HMAC: Keyed-Hashing for Message Authentication RFC-2104
namespace ThorsAnvil::Crypto
{

// Look in hash.h for good examples of THash
// ThorsAnvil::Crypto::Sha1
template<typename THash>
struct HMac
{
    static constexpr std::size_t digestSize = THash::digestSize;
    using Hash        = THash;
    using DigestStore = typename Hash::DigestStore;

    void hash(std::string_view key, std::string_view message, DigestStore& digest)
    {
        Hash    hasher;

        enum { BLOCK_SIZE     = 64 };

        /* STEP 1 */
        std::array<Byte, BLOCK_SIZE>   SHA1_Key{'\x00'};
        if (key.size() > BLOCK_SIZE)
        {
            hasher.hashUnsafe(key, &SHA1_Key[0]);
        }
        else
        {
            std::copy(std::begin(key), std::end(key), &SHA1_Key[0]);
        }

        /* STEP 2 */
        std::string     ipad;
        std::string     opad;

        ipad.reserve(BLOCK_SIZE + std::size(message));
        opad.reserve(BLOCK_SIZE + digestSize);
        ipad.resize(BLOCK_SIZE, '\x36');
        opad.resize(BLOCK_SIZE, '\x5c');

        for (int i=0; i< BLOCK_SIZE; i++)
        {
            ipad[i] ^= SHA1_Key[i];
            opad[i] ^= SHA1_Key[i];
        }

        /* STEP 3 */
        std::copy(std::begin(message), std::end(message), std::back_inserter(ipad));

        /* STEP 4 */
        opad.resize(BLOCK_SIZE + digestSize);
        hasher.hashUnsafe(ipad, reinterpret_cast<Byte*>(&opad[BLOCK_SIZE]));

        /* STEP 5 */
        // Moved XOR of opad to STEP 2

        /* STEP 6 */
        // Don't need to copy the hash of ipad onto opad as we hashed
        // into the correct destination.

        /*STEP 7 */
        hasher.hash(opad, digest);
    }
};

}

#endif

Answer 1

Avoid using the same type to store the results of plain digests and HMACs

You can't (or at least should never be able to) compare a HMAC to a plain digest. So would be good if the type system could catch that potential mistake. Instead of having a DigestStore<Hash> class that is used both for plain digests and HMACs, I would just have Digest<Hash> and HMAC<Hash> each store their own result directly.

Allow adding data to the HMAC in multiple steps

As mentioned in the review for part 1, it is not uncommon to have to add multiple, discontiguous pieces of data to be added to the HMAC, so have a member function add() that can update the HMAC. This would mean splitting the creation of the HMAC in three parts:

1. The key material is prepared as part of the constructor
2. The message is added to the hash, either in one go or using multiple function calls
3. The final value is calculated

I would structure the class like so:

template<typename Hash>
class HMAC {
    Digest<Hash> outer_digest;
    Digest<Hash> inner_digest;

public:
    HMAC(std::string_view key) {
        // Add key XOR opad to outer_digest
        // Add key XOR ipad to inner_digest
    }

    // Convenience constructor to do a one-shot HMAC creation
    HMAC(std::string_view key, std::string_view message): HMAC(key) {
        add(message);
        finish();
    }

    void add(std::string_view message) {
        // Add message to inner_digest
    }

    void finish() {
        // Finish inner_digest, add it to outer_digest
        // Finish outer_digest
    }

    // Something to get the bits out
    const auto &get() {
        return outer_digest.get();
    }
};

You might also want to add some way to prevent finish() from being called more than once.

Avoid unsafe operations

Your own words:

I just hate reading these badly written C++ (which is why I started this hack) projects that are crappy wrappers around C rather than using good type safety and nice clean C++ interfaces and techniques.

You want good type safety, but I also think you want good safety in general. Creating unsafe functions goes against that goal. If you store the results of a hash in a Digest object, and have a way to get a const reference to the data it stores, then you don't need a hashUnsafe() function.