From 82fe4b9262760a27040adaaab0d0417ccc86d47e Mon Sep 17 00:00:00 2001 From: kwasniow Date: Mon, 29 Jun 2026 13:06:29 +0200 Subject: [PATCH 1/3] Sframe return invalid key id error --- include/sframe/result.h | 13 ++++++++ include/sframe/sframe.h | 6 ++++ src/result.cpp | 2 ++ src/sframe.cpp | 4 +-- test/sframe.cpp | 71 +++++++++++++++++++++++++++++++++++++++-- 5 files changed, 92 insertions(+), 4 deletions(-) diff --git a/include/sframe/result.h b/include/sframe/result.h index 0a74c20..18e5edc 100644 --- a/include/sframe/result.h +++ b/include/sframe/result.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -18,6 +19,7 @@ enum class SFrameErrorType unsupported_ciphersuite_error, authentication_error, invalid_key_usage_error, + unknown_key_id_error, }; class SFrameError @@ -35,6 +37,13 @@ class SFrameError { } + SFrameError(SFrameErrorType type, const char* message, uint64_t key_id) + : type_(type) + , message_(message) + , key_id_(key_id) + { + } + SFrameError(const SFrameError& other) = default; SFrameError(SFrameError&& other) noexcept = default; SFrameError& operator=(SFrameError&& other) noexcept = default; @@ -43,11 +52,15 @@ class SFrameError const char* message() const { return message_; } + // Populated only when type() == SFrameErrorType::unknown_key_id_error. + std::optional key_id() const { return key_id_; } + private: SFrameErrorType type_; // Message storage is borrowed; callers must pass a string with static or // otherwise stable lifetime. const char* message_ = nullptr; + std::optional key_id_ = std::nullopt; }; #ifdef __cpp_exceptions diff --git a/include/sframe/sframe.h b/include/sframe/sframe.h index 397d03e..13ccb2d 100644 --- a/include/sframe/sframe.h +++ b/include/sframe/sframe.h @@ -65,6 +65,12 @@ struct invalid_key_usage_error : std::runtime_error using parent = std::runtime_error; using parent::parent; }; + +struct unknown_key_id_error : std::runtime_error +{ + using parent = std::runtime_error; + using parent::parent; +}; #endif enum class CipherSuite : uint16_t diff --git a/src/result.cpp b/src/result.cpp index 6fae585..3092125 100644 --- a/src/result.cpp +++ b/src/result.cpp @@ -32,6 +32,8 @@ throw_sframe_error(const SFrameError& error) throw authentication_error(); case SFrameErrorType::invalid_key_usage_error: throw invalid_key_usage_error(error.message()); + case SFrameErrorType::unknown_key_id_error: + throw unknown_key_id_error(error.message()); } } #endif diff --git a/src/sframe.cpp b/src/sframe.cpp index 60c28c1..a1dca17 100644 --- a/src/sframe.cpp +++ b/src/sframe.cpp @@ -130,8 +130,8 @@ Result Context::require_key(KeyID key_id) const { if (!keys.contains(key_id)) { - return SFrameError(SFrameErrorType::invalid_parameter_error, - "Unknown key ID"); + return SFrameError( + SFrameErrorType::unknown_key_id_error, "Unknown key ID", key_id); } return Result::ok(); } diff --git a/test/sframe.cpp b/test/sframe.cpp index 9f8adf8..a3afc2a 100644 --- a/test/sframe.cpp +++ b/test/sframe.cpp @@ -258,12 +258,12 @@ TEST_CASE("SFrame Context Remove Key") // Remove sender key and verify protect fails sender.remove_key(kid); CHECK(sender.protect(kid, ct_out, plaintext, metadata).error().type() == - SFrameErrorType::invalid_parameter_error); + SFrameErrorType::unknown_key_id_error); // Remove receiver key and verify unprotect fails receiver.remove_key(kid); CHECK(receiver.unprotect(pt_out, encrypted, metadata).error().type() == - SFrameErrorType::invalid_parameter_error); + SFrameErrorType::unknown_key_id_error); // Re-add keys and verify round-trip works again sender.add_key(kid, KeyUsage::protect, key).unwrap(); @@ -286,6 +286,73 @@ TEST_CASE("SFrame Context Remove Key - Nonexistent Key") CHECK_NOTHROW(ctx.remove_key(KeyID(0x99))); } +TEST_CASE("SFrame Unknown Key") +{ + const auto suite = CipherSuite::AES_GCM_128_SHA256; + const auto kid = KeyID(0x42); + const auto unknown_kid = KeyID(0x43); + const auto key = from_hex("000102030405060708090a0b0c0d0e0f"); + const auto plaintext = from_hex("00010203"); + const auto metadata = bytes{}; + + auto pt_out = bytes(plaintext.size()); + auto ct_out = bytes(plaintext.size() + Context::max_overhead); + + auto sender = Context(suite); + sender.add_key(kid, KeyUsage::protect, key).unwrap(); + + // Protecting with a key ID that was never added fails with + // unknown_key_id_error + CHECK( + sender.protect(unknown_kid, ct_out, plaintext, metadata).error().type() == + SFrameErrorType::unknown_key_id_error); + + // Produce a valid ciphertext whose header references `kid` + auto encrypted = + to_bytes(sender.protect(kid, ct_out, plaintext, metadata).unwrap()); + + // A receiver that doesn't know `kid` fails to unprotect with + // unknown_key_id_error + auto receiver = Context(suite); + CHECK(receiver.unprotect(pt_out, encrypted, metadata).error().type() == + SFrameErrorType::unknown_key_id_error); +} + +TEST_CASE("SFrame Unknown Key Error Reports Key ID") +{ + const auto suite = CipherSuite::AES_GCM_128_SHA256; + const auto kid = KeyID(0x42); + const auto unknown_kid = KeyID(0x43); + const auto key = from_hex("000102030405060708090a0b0c0d0e0f"); + const auto plaintext = from_hex("00010203"); + const auto metadata = bytes{}; + + auto pt_out = bytes(plaintext.size()); + auto ct_out = bytes(plaintext.size() + Context::max_overhead); + + auto sender = Context(suite); + sender.add_key(kid, KeyUsage::protect, key).unwrap(); + + // Protecting with a key ID never added: error must name that key ID + auto protect_err = + sender.protect(unknown_kid, ct_out, plaintext, metadata).error(); + CHECK(protect_err.type() == SFrameErrorType::unknown_key_id_error); + CHECK(protect_err.key_id().has_value()); + CHECK(protect_err.key_id().value() == unknown_kid); + + // Produce a ciphertext whose header embeds kid + auto encrypted = + to_bytes(sender.protect(kid, ct_out, plaintext, metadata).unwrap()); + + // Receiver with no keys: error must name the key ID parsed from the + // ciphertext + auto receiver = Context(suite); + auto unprotect_err = receiver.unprotect(pt_out, encrypted, metadata).error(); + CHECK(unprotect_err.type() == SFrameErrorType::unknown_key_id_error); + CHECK(unprotect_err.key_id().has_value()); + CHECK(unprotect_err.key_id().value() == kid); +} + TEST_CASE("MLS Remove Epoch") { const auto suite = CipherSuite::AES_GCM_128_SHA256; From d0819ddd5bbe1c50a21c930357c6851170dac0c8 Mon Sep 17 00:00:00 2001 From: kwasniow Date: Thu, 2 Jul 2026 15:56:13 +0200 Subject: [PATCH 2/3] mls also should return error key id --- include/sframe/result.h | 24 ++++++++---------------- include/sframe/vector.h | 12 ++++-------- src/crypto_boringssl.cpp | 3 +-- src/crypto_openssl11.cpp | 6 ++---- src/crypto_openssl3.cpp | 3 +-- src/header.cpp | 12 ++++-------- src/result.cpp | 6 ++---- src/sframe.cpp | 7 +++---- test/sframe.cpp | 4 ++-- 9 files changed, 27 insertions(+), 50 deletions(-) diff --git a/include/sframe/result.h b/include/sframe/result.h index 18e5edc..b6d9d15 100644 --- a/include/sframe/result.h +++ b/include/sframe/result.h @@ -28,21 +28,18 @@ class SFrameError SFrameError(SFrameErrorType type) : type_(type) , message_(nullptr) - { - } + {} SFrameError(SFrameErrorType type, const char* message) : type_(type) , message_(message) - { - } + {} SFrameError(SFrameErrorType type, const char* message, uint64_t key_id) : type_(type) , message_(message) , key_id_(key_id) - { - } + {} SFrameError(const SFrameError& other) = default; SFrameError(SFrameError&& other) noexcept = default; @@ -79,18 +76,15 @@ class Result Result(SFrameError error) : data_(std::move(error)) - { - } + {} Result(SFrameErrorType error) : data_(SFrameError(error)) - { - } + {} Result(T value) : data_(std::move(value)) - { - } + {} Result(const Result& other) = delete; Result& operator=(const Result& other) = delete; @@ -138,13 +132,11 @@ class Result Result(SFrameError error) : error_(std::move(error)) - { - } + {} Result(SFrameErrorType error) : error_(SFrameError(error)) - { - } + {} Result() = default; diff --git a/include/sframe/vector.h b/include/sframe/vector.h index d86065a..d247801 100644 --- a/include/sframe/vector.h +++ b/include/sframe/vector.h @@ -107,24 +107,20 @@ class vector : private std::vector public: constexpr vector() : parent(N) - { - } + {} constexpr vector(size_t size) : parent(size) - { - } + {} constexpr vector(gsl::span content) : parent(content.begin(), content.end()) - { - } + {} template constexpr vector(const vector& content) : parent(content.begin(), content.end()) - { - } + {} T* data() { return parent::data(); } const T* data() const { return parent::data(); } diff --git a/src/crypto_boringssl.cpp b/src/crypto_boringssl.cpp index 0f1957b..dc19df9 100644 --- a/src/crypto_boringssl.cpp +++ b/src/crypto_boringssl.cpp @@ -20,8 +20,7 @@ namespace SFRAME_NAMESPACE { #ifdef __cpp_exceptions crypto_error::crypto_error() : std::runtime_error(ERR_error_string(ERR_get_error(), nullptr)) -{ -} +{} #endif static Result diff --git a/src/crypto_openssl11.cpp b/src/crypto_openssl11.cpp index 82c0b43..c12a9d5 100644 --- a/src/crypto_openssl11.cpp +++ b/src/crypto_openssl11.cpp @@ -26,8 +26,7 @@ using scoped_hmac_ctx = std::unique_ptr; #ifdef __cpp_exceptions crypto_error::crypto_error() : std::runtime_error(ERR_error_string(ERR_get_error(), nullptr)) -{ -} +{} #endif static Result @@ -79,8 +78,7 @@ struct HMAC explicit HMAC(scoped_hmac_ctx ctx_) : ctx(std::move(ctx_)) - { - } + {} public: HMAC(HMAC&&) noexcept = default; diff --git a/src/crypto_openssl3.cpp b/src/crypto_openssl3.cpp index b19b58d..349b19b 100644 --- a/src/crypto_openssl3.cpp +++ b/src/crypto_openssl3.cpp @@ -20,8 +20,7 @@ namespace SFRAME_NAMESPACE { #ifdef __cpp_exceptions crypto_error::crypto_error() : std::runtime_error(ERR_error_string(ERR_get_error(), nullptr)) -{ -} +{} #endif static Result diff --git a/src/header.cpp b/src/header.cpp index b1bc9c1..bdd5c5f 100644 --- a/src/header.cpp +++ b/src/header.cpp @@ -103,8 +103,7 @@ struct ValueOrLength ValueOrLength(bool is_length_in, uint8_t value_or_length_in) : is_length(is_length_in) , value_or_length(value_or_length_in) - { - } + {} }; struct ConfigByte @@ -115,14 +114,12 @@ struct ConfigByte ConfigByte(uint64_t kid_in, uint64_t ctr_in) : kid(ValueOrLength::for_u64(kid_in)) , ctr(ValueOrLength::for_u64(ctr_in)) - { - } + {} explicit ConfigByte(uint8_t encoded) : kid(ValueOrLength::decode(encoded >> 4)) , ctr(ValueOrLength::decode(encoded & 0x0f)) - { - } + {} size_t encoded_size() const { @@ -178,8 +175,7 @@ Header::Header(KeyID key_id_in, Counter counter_in, input_bytes encoded_in) : key_id(key_id_in) , counter(counter_in) , _encoded(encoded_in) -{ -} +{} #if 0 std::tuple diff --git a/src/result.cpp b/src/result.cpp index 3092125..ca81315 100644 --- a/src/result.cpp +++ b/src/result.cpp @@ -5,13 +5,11 @@ namespace SFRAME_NAMESPACE { #ifdef __cpp_exceptions unsupported_ciphersuite_error::unsupported_ciphersuite_error() : std::runtime_error("Unsupported ciphersuite") -{ -} +{} authentication_error::authentication_error() : std::runtime_error("AEAD authentication failure") -{ -} +{} void throw_sframe_error(const SFrameError& error) diff --git a/src/sframe.cpp b/src/sframe.cpp index a1dca17..349b295 100644 --- a/src/sframe.cpp +++ b/src/sframe.cpp @@ -78,8 +78,7 @@ KeyRecord::from_base_key(CipherSuite suite, Context::Context(CipherSuite suite_in) : suite(suite_in) -{ -} +{} Context::~Context() = default; @@ -419,8 +418,8 @@ MLSContext::ensure_key(KeyID key_id, KeyUsage usage) const auto epoch_index = key_id & epoch_mask; auto& epoch = epoch_cache[epoch_index]; if (!epoch) { - return SFrameError(SFrameErrorType::invalid_parameter_error, - "Unknown epoch"); + return SFrameError( + SFrameErrorType::unknown_key_id_error, "Unknown key ID", key_id); } if (keys.contains(key_id)) { diff --git a/test/sframe.cpp b/test/sframe.cpp index a3afc2a..146bf59 100644 --- a/test/sframe.cpp +++ b/test/sframe.cpp @@ -223,7 +223,7 @@ TEST_CASE("MLS Failure after Purge") .error() .type() == SFrameErrorType::invalid_parameter_error); CHECK(member_b.unprotect(pt_out, enc_ab_1_data, metadata).error().type() == - SFrameErrorType::invalid_parameter_error); + SFrameErrorType::unknown_key_id_error); const auto enc_ab_2 = member_a.protect(epoch_id_2, sender_id_a, ct_out, plaintext, metadata) @@ -394,7 +394,7 @@ TEST_CASE("MLS Remove Epoch") .error() .type() == SFrameErrorType::invalid_parameter_error); CHECK(member_b.unprotect(pt_out, enc_data, metadata).error().type() == - SFrameErrorType::invalid_parameter_error); + SFrameErrorType::unknown_key_id_error); // Epoch 2 should still work enc = member_a.protect(epoch_id_2, sender_id, ct_out, plaintext, metadata) From 0861d4ff3067f7fa411ccaacd7a21eccf7cc9e80 Mon Sep 17 00:00:00 2001 From: kwasniow Date: Wed, 8 Jul 2026 15:58:36 +0200 Subject: [PATCH 3/3] Formatting update --- include/sframe/result.h | 24 ++++++++++++++++-------- include/sframe/vector.h | 12 ++++++++---- src/crypto_boringssl.cpp | 3 ++- src/crypto_openssl11.cpp | 6 ++++-- src/crypto_openssl3.cpp | 3 ++- src/header.cpp | 12 ++++++++---- src/result.cpp | 6 ++++-- src/sframe.cpp | 3 ++- 8 files changed, 46 insertions(+), 23 deletions(-) diff --git a/include/sframe/result.h b/include/sframe/result.h index b6d9d15..18e5edc 100644 --- a/include/sframe/result.h +++ b/include/sframe/result.h @@ -28,18 +28,21 @@ class SFrameError SFrameError(SFrameErrorType type) : type_(type) , message_(nullptr) - {} + { + } SFrameError(SFrameErrorType type, const char* message) : type_(type) , message_(message) - {} + { + } SFrameError(SFrameErrorType type, const char* message, uint64_t key_id) : type_(type) , message_(message) , key_id_(key_id) - {} + { + } SFrameError(const SFrameError& other) = default; SFrameError(SFrameError&& other) noexcept = default; @@ -76,15 +79,18 @@ class Result Result(SFrameError error) : data_(std::move(error)) - {} + { + } Result(SFrameErrorType error) : data_(SFrameError(error)) - {} + { + } Result(T value) : data_(std::move(value)) - {} + { + } Result(const Result& other) = delete; Result& operator=(const Result& other) = delete; @@ -132,11 +138,13 @@ class Result Result(SFrameError error) : error_(std::move(error)) - {} + { + } Result(SFrameErrorType error) : error_(SFrameError(error)) - {} + { + } Result() = default; diff --git a/include/sframe/vector.h b/include/sframe/vector.h index d247801..d86065a 100644 --- a/include/sframe/vector.h +++ b/include/sframe/vector.h @@ -107,20 +107,24 @@ class vector : private std::vector public: constexpr vector() : parent(N) - {} + { + } constexpr vector(size_t size) : parent(size) - {} + { + } constexpr vector(gsl::span content) : parent(content.begin(), content.end()) - {} + { + } template constexpr vector(const vector& content) : parent(content.begin(), content.end()) - {} + { + } T* data() { return parent::data(); } const T* data() const { return parent::data(); } diff --git a/src/crypto_boringssl.cpp b/src/crypto_boringssl.cpp index dc19df9..0f1957b 100644 --- a/src/crypto_boringssl.cpp +++ b/src/crypto_boringssl.cpp @@ -20,7 +20,8 @@ namespace SFRAME_NAMESPACE { #ifdef __cpp_exceptions crypto_error::crypto_error() : std::runtime_error(ERR_error_string(ERR_get_error(), nullptr)) -{} +{ +} #endif static Result diff --git a/src/crypto_openssl11.cpp b/src/crypto_openssl11.cpp index c12a9d5..82c0b43 100644 --- a/src/crypto_openssl11.cpp +++ b/src/crypto_openssl11.cpp @@ -26,7 +26,8 @@ using scoped_hmac_ctx = std::unique_ptr; #ifdef __cpp_exceptions crypto_error::crypto_error() : std::runtime_error(ERR_error_string(ERR_get_error(), nullptr)) -{} +{ +} #endif static Result @@ -78,7 +79,8 @@ struct HMAC explicit HMAC(scoped_hmac_ctx ctx_) : ctx(std::move(ctx_)) - {} + { + } public: HMAC(HMAC&&) noexcept = default; diff --git a/src/crypto_openssl3.cpp b/src/crypto_openssl3.cpp index 349b19b..b19b58d 100644 --- a/src/crypto_openssl3.cpp +++ b/src/crypto_openssl3.cpp @@ -20,7 +20,8 @@ namespace SFRAME_NAMESPACE { #ifdef __cpp_exceptions crypto_error::crypto_error() : std::runtime_error(ERR_error_string(ERR_get_error(), nullptr)) -{} +{ +} #endif static Result diff --git a/src/header.cpp b/src/header.cpp index bdd5c5f..b1bc9c1 100644 --- a/src/header.cpp +++ b/src/header.cpp @@ -103,7 +103,8 @@ struct ValueOrLength ValueOrLength(bool is_length_in, uint8_t value_or_length_in) : is_length(is_length_in) , value_or_length(value_or_length_in) - {} + { + } }; struct ConfigByte @@ -114,12 +115,14 @@ struct ConfigByte ConfigByte(uint64_t kid_in, uint64_t ctr_in) : kid(ValueOrLength::for_u64(kid_in)) , ctr(ValueOrLength::for_u64(ctr_in)) - {} + { + } explicit ConfigByte(uint8_t encoded) : kid(ValueOrLength::decode(encoded >> 4)) , ctr(ValueOrLength::decode(encoded & 0x0f)) - {} + { + } size_t encoded_size() const { @@ -175,7 +178,8 @@ Header::Header(KeyID key_id_in, Counter counter_in, input_bytes encoded_in) : key_id(key_id_in) , counter(counter_in) , _encoded(encoded_in) -{} +{ +} #if 0 std::tuple diff --git a/src/result.cpp b/src/result.cpp index ca81315..3092125 100644 --- a/src/result.cpp +++ b/src/result.cpp @@ -5,11 +5,13 @@ namespace SFRAME_NAMESPACE { #ifdef __cpp_exceptions unsupported_ciphersuite_error::unsupported_ciphersuite_error() : std::runtime_error("Unsupported ciphersuite") -{} +{ +} authentication_error::authentication_error() : std::runtime_error("AEAD authentication failure") -{} +{ +} void throw_sframe_error(const SFrameError& error) diff --git a/src/sframe.cpp b/src/sframe.cpp index 349b295..3fbf77f 100644 --- a/src/sframe.cpp +++ b/src/sframe.cpp @@ -78,7 +78,8 @@ KeyRecord::from_base_key(CipherSuite suite, Context::Context(CipherSuite suite_in) : suite(suite_in) -{} +{ +} Context::~Context() = default;