Add SPAKE2 password authenticated key exchange (RFC 9382)

[randombit]

No description provided.

[coveralls]

coverage: 90.662% (+0.002%) from 90.66%
when pulling cf80393 on jack/spake2
into ee093a6 on master.

[reneme]

I have a few suggestions re the API and some nits. Most of them, I prototyped in another branch, including an example.cpp.

[reneme]

concat() could make this easier:

Suggested change

	std::vector<uint8_t> ad(a_identity.size() + b_identity.size() + context.size() + 3 * 8);
	BufferStuffer stuffer(ad);
	auto append_with_le64 = [&](std::span<const uint8_t> data) {
	stuffer.append(store_le(static_cast<uint64_t>(data.size())));
	stuffer.append(data);
	};
	append_with_le64(a_identity);
	append_with_le64(b_identity);
	append_with_le64(context);
	return ad;
	auto store_le64 = [](uint64_t s) { return store_le(s); };
	// clang-format off
	return concat<std::vector<uint8_t>>(store_le64(a_identity.size()), a_identity,
	store_le64(b_identity.size()), b_identity,
	store_le64(context.size()), context);
	// clang-format on

[reneme]

Perhaps pull the store_le64 lambda as a free-standing function into the anonymous namespace and reuse it for the hash calculation in process_message().

auto store_le64(uint64_t n) -> std::array<uint8_t, 8> {
   return store_le(n);
}

[reneme]

Suggested change

	EC_AffinePoint peer_pt(m_params.group(), peer_message);
	const EC_AffinePoint peer_pt(m_params.group(), peer_message);

[reneme]

Suggested change

	append_to_hash_with_le64(m_params.a_identity());
	// Calculate TT
	append_to_hash_with_le64(m_params.a_identity());

[reneme]

Along the lines of m_params.spake2_their_pt(), we could add this to the params class:

      auto spake2_sort_messages(SPAKE2_PeerId whoami,
                                std::span<const uint8_t> ours,
                                std::span<const uint8_t> theirs) const
         -> std::pair<std::span<const uint8_t>, std::span<const uint8_t>> {
         if(whoami == SPAKE2_PeerId::PeerB) {
            std::swap(ours, theirs);
         }
         return {ours, theirs};
      }

... and then reduce this code to:

Suggested change

	// Always pA followed by pB:
	if(m_whoami == SPAKE2_PeerId::PeerA) {
	append_to_hash_with_le64(our_pt);
	append_to_hash_with_le64(peer_message);
	} else {
	append_to_hash_with_le64(peer_message);
	append_to_hash_with_le64(our_pt);
	}
	auto [pA, pB] = m_params.spake2_sort_messages(m_whoami, our_pt, peer_message);
	append_to_hash_with_le64(pA);
	append_to_hash_with_le64(pB);

[reneme]

Instead of keeping the state visible to the user in a context object, we could make generate_message() return an opaque (non-copyable) state object that process_message() requires to be moved into as a parameter. This introduces a bit of boiler plate, but effectively avoids misuse.

Eg.

      struct Internal;

      struct State {
            State(std::unique_ptr<Internal> i);

            ~State();
            State(const State&) = delete;
            State& operator=(const State&) = delete;
            State(State&&) noexcept;
            State& operator=(State&&) noexcept;

            std::unique_ptr<Internal> internal;  // NOLINT(misc-non-private-member-*)
      };

Internal would be implemented in the *.cpp and essentially contain the content of m_our_message.
In full consequence, this could reduce the API to two free-standing functions and remove the need for the context class entirely. I prototyped this in the branch I mentioned in the PR-submission. The example-code in this branch also uses the free-standing functions for illustration.

[reneme]

Does it, though? One might argue, that this constructor provides the flexibility to use a different memory-hard hash function than the default (argon2id). Perhaps we should instead add another constructor that takes a std::span<> w_bytes as an already expanded shared secret, that is then just turned into a scalar of the group.

This would be a major footgun, though, because people might just accidentally pass the password (meant for the std::string_view overload) into this proposed constructor and get no pwhash at all.

[randombit]

It does allow this (or Argon2id with different parameters, etc), so there is utility outside of the testing scenario. However it seems like an obvious footgun already, and a span argument instead seems even more prone to misuse.

I suppose I can amend the comment to make it clear it is usable as long as the scalar is derived from the password using a MHF and with care to avoid bias, referring to the relevant RFC sections.

[reneme]

Perhaps a few negative tests? E.g. when the context string is passed inconsistently.

[randombit]

@reneme Finally coming back to this. I do like your approach in terms of footgun resistance but I think it does not work for FFI where we must put the state into a unique_ptr so we can return it out to the FFI caller.

[randombit]

Still todo

• Implement optional key confirmation rather than making the application do it
• More tests especially negative tests
• Benchmarks
• FFI + FFI tests + Python

[reneme]

I do like your approach in terms of footgun resistance but I think it does not work for FFI where we must put the state into a unique_ptr so we can return it out to the FFI caller.

The State object is stack-based and can't be copied. Therefore it is a bit awkward to use with std::unique_ptr, for sure. But I wouldn't see that as a show stopper. A small wrapper with a member of State would be enough to mitigate this. If you really want, you could even just use std::optional<> as the wrapper type...

std::unique_ptr<std::optional<State>> wrapper;
State s1{}; // from the PAKE
wrapper.reset(new std::optional<State>(std::move(s1)));
// ---- FFI API boundary
State s2 = std::move(wrapper->value());

Godbolt.org

[randombit]

I think we're going to go for SPAKE2+ instead

[randombit]

SPAKE2+ #5711