Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IF: change BLS key/signature encoding to base64url for easier copy/paste #2255

Merged
merged 11 commits into from
Feb 27, 2024
Merged

IF: change BLS key/signature encoding to base64url for easier copy/paste #2255

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e4bf3b8
change BLS key/signature encoding to base64url for easier copy/paste;…
linh2931 Feb 21, 2024
d95e7a0
replace base64 library with Kevin Heifner's implementation
linh2931 Feb 22, 2024
2b41bb9
add convenient methods for existing Leap uses
linh2931 Feb 24, 2024
0ad9b7a
make it compile
linh2931 Feb 24, 2024
308e97c
fix a compile warning
linh2931 Feb 24, 2024
891ed4f
use new base64url encoding for two missing files
linh2931 Feb 24, 2024
63b1d56
remove trailing separator == in test data
linh2931 Feb 24, 2024
9343bdd
Update webauthn test suite for new base64 implementation
linh2931 Feb 24, 2024
1e46ff7
Make the new base64url decode throw for invalid base64url characters,…
linh2931 Feb 25, 2024
00f2ce0
revert changes to tests in test_webauthn.cpp and only adapt the base6…
linh2931 Feb 25, 2024
3eceef3
update test_webauthn's base64url related tests to keep the spirits of…
linh2931 Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 36 additions & 19 deletions libraries/libfc/test/crypto/test_webauthn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,15 @@ BOOST_AUTO_TEST_CASE(challenge_non_base64) try {
std::vector<uint8_t> auth_data(37);
memcpy(auth_data.data(), origin_hash.data(), sizeof(origin_hash));

BOOST_CHECK_EXCEPTION(make_webauthn_sig(priv, auth_data, json).recover(d, true), fc::exception, [](const fc::exception& e) {
return e.to_detail_string().find("encountered non-base64 character") != std::string::npos;
BOOST_CHECK_EXCEPTION(make_webauthn_sig(priv, auth_data, json).recover(d, true), std::exception, [](const std::exception& e) {
return std::string{e.what()}.find("Input is not valid base64-encoded data") != std::string::npos;
});
} FC_LOG_AND_RETHROW();

//valid signature but replace url-safe base64 characters with the non-url safe characters
BOOST_AUTO_TEST_CASE(challenge_wrong_base64_chars) try {
// The new base64 implementation's decode treats url-safe characters '_' and '-' the same
// as '/' and '+' the same. As long as they don't mistmatch, decode always works.
// Valid signature but replace url-safe base64 characters with the non-url safe characters
BOOST_AUTO_TEST_CASE(challenge_interchanged_base64_chars) try {
webauthn::public_key wa_pub(pub.serialize(), webauthn::public_key::user_presence_t::USER_PRESENCE_NONE, "fctesting.invalid");
std::string b64 = fc::base64url_encode(d.data(), d.data_size());

Expand All @@ -183,38 +185,55 @@ BOOST_AUTO_TEST_CASE(challenge_wrong_base64_chars) try {
std::vector<uint8_t> auth_data(37);
memcpy(auth_data.data(), origin_hash.data(), sizeof(origin_hash));

BOOST_CHECK_EXCEPTION(make_webauthn_sig(priv, auth_data, json).recover(d, true), fc::exception, [](const fc::exception& e) {
return e.to_detail_string().find("encountered non-base64 character") != std::string::npos;
});
BOOST_CHECK_NO_THROW(make_webauthn_sig(priv, auth_data, json).recover(d, true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change really looks like a consensus change: what threw an exception before now doesn't

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Matt. We can add a check in the base64url_decode wrapper in the new implementation for that but to be safe we should just switch back to the existing implementation. @heifner ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me it is hard to argue for moving to a new implementation that could possibly introduce a consensus change. Ideally I would like to see all consensus code be isolated and not touched; allowing non-consensus code to be updated/bug-fixed as desired.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My latest commit 00f2ce0 tries to behave in the same way as the existing base64url decode; the existing tests are only modified to accommodate the fact that trailing separators are no longer added.

} FC_LOG_AND_RETHROW();

// The new base64 implementation's decode treats url-safe characters '_' and '-' the same
// as '/' and '+' the same. As long as they don't mistmatch, decode always works.
// Valid signature but url-safe base64 characters are mismatched with the non-url safe
// characters
BOOST_AUTO_TEST_CASE(challenge_mismatched_url_safe_with_non_safe_chars) try {
webauthn::public_key wa_pub(pub.serialize(), webauthn::public_key::user_presence_t::USER_PRESENCE_NONE, "fctesting.invalid");
std::string b64 = fc::base64url_encode(d.data(), d.data_size());

BOOST_REQUIRE(b64[1] == '_');
BOOST_REQUIRE(b64[18] == '_');
BOOST_REQUIRE(b64[36] == '-');

b64[1] = b64[18] = '+';
b64[36] = '+';

std::string json = "{\"origin\":\"https://fctesting.invalid\",\"type\":\"webauthn.get\",\"challenge\":\"" + b64 + "\"}";

std::vector<uint8_t> auth_data(37);
memcpy(auth_data.data(), origin_hash.data(), sizeof(origin_hash));

BOOST_CHECK_THROW(make_webauthn_sig(priv, auth_data, json).recover(d, true), fc::exception);
} FC_LOG_AND_RETHROW();

//valid signature but replace the padding '=' with '.'
//valid signature but replace the last char with '.'
BOOST_AUTO_TEST_CASE(challenge_base64_dot_padding) try {
webauthn::public_key wa_pub(pub.serialize(), webauthn::public_key::user_presence_t::USER_PRESENCE_NONE, "fctesting.invalid");
std::string b64 = fc::base64url_encode(d.data(), d.data_size());
char& end = b64.back();

BOOST_REQUIRE(end == '=');
end = '.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was originally changing the padding from a valid character to an invalid character. Now it's changing a non-padding character from a valid character to an invalid character. I think maybe this should just append a . (or really any other invalid character) to keep the spirit of the original test; we already have challenge_non_base64 to cover validity of non-padded characters.


std::string json = "{\"origin\":\"https://fctesting.invalid\",\"type\":\"webauthn.get\",\"challenge\":\"" + b64 + "\"}";

std::vector<uint8_t> auth_data(37);
memcpy(auth_data.data(), origin_hash.data(), sizeof(origin_hash));

BOOST_CHECK_EXCEPTION(make_webauthn_sig(priv, auth_data, json).recover(d, true), fc::exception, [](const fc::exception& e) {
return e.to_detail_string().find("encountered non-base64 character") != std::string::npos;
BOOST_CHECK_EXCEPTION(make_webauthn_sig(priv, auth_data, json).recover(d, true), std::exception, [](const std::exception& e) {
return std::string{e.what()}.find("Input is not valid base64-encoded data") != std::string::npos;
});
} FC_LOG_AND_RETHROW();

//valid signature but remove padding
//valid signature without padding (base64url_encode does not have padding)
BOOST_AUTO_TEST_CASE(challenge_no_padding) try {
webauthn::public_key wa_pub(pub.serialize(), webauthn::public_key::user_presence_t::USER_PRESENCE_NONE, "fctesting.invalid");
std::string b64 = fc::base64url_encode(d.data(), d.data_size());

BOOST_REQUIRE(b64.back() == '=');
b64.resize(b64.size() - 1);

std::string json = "{\"origin\":\"https://fctesting.invalid\",\"type\":\"webauthn.get\",\"challenge\":\"" + b64 + "\"}";

std::vector<uint8_t> auth_data(37);
Expand All @@ -228,8 +247,6 @@ BOOST_AUTO_TEST_CASE(challenge_extra_bytes) try {
webauthn::public_key wa_pub(pub.serialize(), webauthn::public_key::user_presence_t::USER_PRESENCE_NONE, "fctesting.invalid");
std::string b64 = fc::base64url_encode(d.data(), d.data_size());

BOOST_REQUIRE(b64.back() == '=');
b64.resize(b64.size() - 1);
b64.append("abcd");

std::string json = "{\"origin\":\"https://fctesting.invalid\",\"type\":\"webauthn.get\",\"challenge\":\"" + b64 + "\"}";
Expand Down Expand Up @@ -487,8 +504,8 @@ BOOST_AUTO_TEST_CASE(base64_wonky) try {
std::vector<uint8_t> auth_data(37);
memcpy(auth_data.data(), origin_hash.data(), sizeof(origin_hash));

BOOST_CHECK_EXCEPTION(make_webauthn_sig(priv, auth_data, json).recover(d, true), fc::exception, [](const fc::exception& e) {
return e.to_detail_string().find("encountered non-base64 character") != std::string::npos;
BOOST_CHECK_EXCEPTION(make_webauthn_sig(priv, auth_data, json).recover(d, true), std::exception, [](const std::exception& e) {
return std::string{e.what()}.find("Input is not valid base64-encoded data") != std::string::npos;
});
} FC_LOG_AND_RETHROW();

Expand Down
Loading