Skip to content

drop use of BIO_f_base64 and EVP_PKEY_new_mac_key #7106

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

Merged
randall merged 2 commits into from
Feb 3, 2021
Merged

drop use of BIO_f_base64 and EVP_PKEY_new_mac_key #7106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5da590
drop use of BIO_f_base64 and EVP_PKEY_new_mac_key
randall Aug 11, 2020
979b4be
Add debug logging
randall Aug 12, 2020
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
4 changes: 2 additions & 2 deletions plugins/experimental/access_control/Makefile.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ experimental_access_control_access_control_la_SOURCES = \
check_PROGRAMS += experimental/access_control/test_access_control

experimental_access_control_test_access_control_CPPFLAGS = $(AM_CPPFLAGS) -I$(abs_top_srcdir)/tests/include -DACCESS_CONTROL_UNIT_TEST
experimental_access_control_test_access_control_LDADD = $(OPENSSL_LIBS)
experimental_access_control_test_access_control_LDADD = $(OPENSSL_LIBS) $(top_builddir)/src/tscore/libtscore.la

experimental_access_control_test_access_control_SOURCES = \
experimental/access_control/unit_tests/test_access_control.cc \
experimental/access_control/unit_tests/test_utils.cc \
experimental/access_control/access_control.cc \
experimental/access_control/common.cc \
experimental/access_control/utils.cc

6 changes: 3 additions & 3 deletions plugins/experimental/access_control/access_control.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,7 @@ static const std::map<String, String> _digestAlgosMap = createStaticDigestAlgoMa
* @param hf Hash Function (HF) [optional]
* @param secret secret
* @param message input message
* @param messageLen input message lenght
* @param messageLen input message length
* @param buffer output buffer for storing the message digest
* @param len output buffer length
* @return number of characters actually written to the output buffer.
Expand Down Expand Up @@ -437,7 +437,7 @@ accessTokenStatusToString(const AccessTokenStatus &state)
s = "INVALID_FIELD_VALUE";
break;
case INVALID_VERSION:
s = "UNSUPORTED_VERSION";
s = "UNSUPPORTED_VERSION";
break;
case INVALID_SECRET:
s = "NO_SECRET_SPECIFIED";
Expand All @@ -461,7 +461,7 @@ accessTokenStatusToString(const AccessTokenStatus &state)
s = "INVALID_KEYID";
break;
case INVALID_HASH_FUNCTION:
s = "UNSUPORTED_HASH_FUNCTION";
s = "UNSUPPORTED_HASH_FUNCTION";
break;
default:
s = "";
Expand Down
4 changes: 2 additions & 2 deletions plugins/experimental/access_control/headers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ headerExist(TSMBuffer bufp, TSMLoc hdrLoc, const char *header, int headerlen)
* @param header header name
* @param headerlen header name length
* @param value buffer for the value
* @param valuelen lenght of the buffer for the value
* @param valuelen length of the buffer for the value
* @return pointer to the string with the value.
*/
char *
Expand Down Expand Up @@ -129,7 +129,7 @@ getHeader(TSMBuffer bufp, TSMLoc hdrLoc, const char *header, int headerlen, char
* @param header header name
* @param headerlen header name len
* @param value the new value
* @param valuelen lenght of the value
* @param valuelen length of the value
* @return true - OK, false - failed
*/
bool
Expand Down
48 changes: 27 additions & 21 deletions plugins/experimental/access_control/unit_tests/test_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,34 +26,33 @@
#include "../common.h"

/*********************************************************************************************************************
* Base64 related test
* @note the purpose of these test is not to test Base64 itself since it is provided by openssl library,
* the idea is to test the usage and some corner cases.
* Base64 related tests
* @note the purpose of these tests is to test the usage and some corner cases.
********************************************************************************************************************/

TEST_CASE("Base64: estimate buffer size needed to encode a message", "[Base64][access_control][utility]")
{
size_t encodedLen;

/* Test with a zero decoded message lenght */
/* Test with a zero decoded message length */
encodedLen = cryptoBase64EncodedSize(0);
CHECK(0 == encodedLen);
CHECK(4 == encodedLen);
Copy link
Member

@maskit maskit Aug 12, 2020

Choose a reason for hiding this comment

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

This is a bit sad. Seems like the comment in ink_base64.h is not true. We may want to improve the macro first. Of course we can last this first and change back these values to the original values after the improvement though.

Little helper functions to calculate minimum required output buffer for encoding/decoding.
#define ATS_BASE64_ENCODE_DSTLEN(_length) ((_length * 8) / 6 + 4)


/* Test with a random non-zero decoded message length */
encodedLen = cryptoBase64EncodedSize(64);
CHECK(88 == encodedLen);
CHECK(89 == encodedLen);

/* Test the space for padding. Size of encoding that would result in 2 x "=" padding */
encodedLen = cryptoBase64EncodedSize(strlen("176a1620e31b14782ba2b66de3edc5b3cb19630475b2ce2ee292d5fd0fe41c3abc"));
CHECK(88 == encodedLen);
CHECK(92 == encodedLen);

/* Test the space for padding. Size of encoding that would result in 1 x "=" padding */
encodedLen = cryptoBase64EncodedSize(strlen("176a1620e31b14782ba2b66de3edc5b3cb19630475b2ce2ee292d5fd0fe41c3ab"));
CHECK(88 == encodedLen);
CHECK(90 == encodedLen);

/* Test the space for padding. Size of encoding that would result in no padding */
encodedLen = cryptoBase64EncodedSize(strlen("176a1620e31b14782ba2b66de3edc5b3cb19630475b2ce2ee292d5fd0fe41c3a"));
CHECK(88 == encodedLen);
CHECK(89 == encodedLen);
}

TEST_CASE("Base64: estimate buffer size needed to decode a message", "[Base64][access_control][utility]")
Expand All @@ -64,24 +63,24 @@ TEST_CASE("Base64: estimate buffer size needed to decode a message", "[Base64][a
/* Padding with 2 x '=' */
encoded = "MTc2YTE2MjBlMzFiMTQ3ODJiYTJiNjZkZTNlZGM1YjNjYjE5NjMwNDc1YjJjZTJlZTI5MmQ1ZmQwZmU0MWMzYQ==";
encodedLen = cryptoBase64DecodeSize(encoded, strlen(encoded));
CHECK(64 == encodedLen);
CHECK(66 == encodedLen);

/* Padding with 1 x '=' */
encoded = "MTc2YTE2MjBlMzFiMTQ3ODJiYTJiNjZkZTNlZGM1YjNjYjE5NjMwNDc1YjJjZTJlZTI5MmQ1ZmQwZmU0MWMzYWI=";
encodedLen = cryptoBase64DecodeSize(encoded, strlen(encoded));
CHECK(65 == encodedLen);
CHECK(66 == encodedLen);

/* Padding with 0 x "=" */
encoded = "MTc2YTE2MjBlMzFiMTQ3ODJiYTJiNjZkZTNlZGM1YjNjYjE5NjMwNDc1YjJjZTJlZTI5MmQ1ZmQwZmU0MWMzYWJj";
encodedLen = cryptoBase64DecodeSize(encoded, strlen(encoded));
CHECK(66 == encodedLen);

/* Test empty encoded message caclulation */
/* Test empty encoded message calculation */
encoded = "";
encodedLen = cryptoBase64DecodeSize(encoded, strlen(encoded));
CHECK(0 == encodedLen);

/* Test empty encoded message caclulation */
/* Test empty encoded message calculation */
encoded = nullptr;
encodedLen = cryptoBase64DecodeSize(encoded, 0);
CHECK(0 == encodedLen);
Expand All @@ -94,16 +93,17 @@ TEST_CASE("Base64: quick encode / decode", "[Base64][access_control][utility]")
CHECK(64 == messageLen);

size_t encodedMessageEstimatedLen = cryptoBase64EncodedSize(messageLen);
CHECK(88 == encodedMessageEstimatedLen);
CHECK(89 == encodedMessageEstimatedLen);
char encodedMessage[encodedMessageEstimatedLen];

// now encode message into encodedMessage
size_t encodedMessageLen = cryptoBase64Encode(message, messageLen, encodedMessage, encodedMessageEstimatedLen);
CHECK(88 == encodedMessageLen);
CHECK(0 == strncmp(encodedMessage, "MTc2YTE2MjBlMzFiMTQ3ODJiYTJiNjZkZTNlZGM1YjNjYjE5NjMwNDc1YjJjZTJlZTI5MmQ1ZmQwZmU0MWMzYQ==",
encodedMessageLen));

size_t decodedMessageEstimatedLen = cryptoBase64DecodeSize(encodedMessage, encodedMessageLen);
CHECK(64 == decodedMessageEstimatedLen);
CHECK(66 == decodedMessageEstimatedLen);
char decodedMessage[encodedMessageEstimatedLen];
size_t decodedMessageLen = cryptoBase64Decode(encodedMessage, encodedMessageLen, decodedMessage, encodedMessageLen);

Expand Down Expand Up @@ -180,8 +180,9 @@ TEST_CASE("Base64: quick encode / decode with '+', '/' and various paddings", "[
CHECK(0 == strncmp(encodedMessage, encoded[i], encodedMessageLen));

/* Decode */
size_t decodedMessageEstimatedLen = cryptoBase64DecodeSize(encodedMessage, encodedMessageLen);
CHECK(strlen(decoded[i]) == decodedMessageEstimatedLen);
Copy link
Member

@maskit maskit Aug 12, 2020

Choose a reason for hiding this comment

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

Why was this check deleted?

Copy link
Contributor Author

@randall randall Aug 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It goes back how cryptoBase64DecodeSize determines its size for input. The test here assumes the estimate is not an estimate and rather an accurate value.

here's the run with the check in place (with some extra debugging):

  CHECK( strlen(decoded[i]) == decodedMessageEstimatedLen )
with expansion:
  11 == 12
with messages:
  i := 1
  std::string(encodedMessage) := "dHM+dHM/dHMhISE="
  decoded[i] := "ts>ts?ts!!!"

experimental/access_control/unit_tests/test_utils.cc:187: FAILED:
  CHECK( strlen(decoded[i]) == decodedMessageEstimatedLen )
with expansion:
  10 == 12
with messages:
  i := 2
  std::string(encodedMessage) := "dHM+dHM/dHMhIQ=="
  decoded[i] := "ts>ts?ts!!"

// Keep test around incase our implementation's estimation gets better
// size_t decodedMessageEstimatedLen = cryptoBase64DecodeSize(encodedMessage, encodedMessageLen);
// CHECK(strlen(decoded[i]) == decodedMessageEstimatedLen);
char decodedMessage[encodedMessageEstimatedLen];
size_t decodedMessageLen = cryptoBase64Decode(encodedMessage, encodedMessageLen, decodedMessage, encodedMessageLen);
CHECK(strlen(decoded[i]) == decodedMessageLen);
Expand All @@ -191,7 +192,6 @@ TEST_CASE("Base64: quick encode / decode with '+', '/' and various paddings", "[

/*********************************************************************************************************************
* Modified Base64 related test
* @note since Modified Base64 function use Base64 which use openssl the idea is to test the modification itself
* (for more info see the comment in the implementation) + some corner cases.
********************************************************************************************************************/

Expand All @@ -215,14 +215,16 @@ TEST_CASE("Base64: modified encode / decode with '+', '/' and various paddings",
char decodedMessage[encodedMessageEstimatedLen];
size_t decodedMessageLen =
cryptoModifiedBase64Decode(encodedMessage, encodedMessageLen, decodedMessage, decodedMessageEstimatedLen);
CAPTURE(i);
CAPTURE(decoded[i]);
CAPTURE(std::string(decodedMessage));
CHECK(strlen(decoded[i]) == decodedMessageLen);
CHECK(0 == strncmp(decodedMessage, message, messageLen));
}
}

/*********************************************************************************************************************
* Digest calculation related test
* @note since Modified Base64 function use Base64 which use openssl the idea is to test the modification itself
* (for more info see the comment in the implementation) + some corner cases.
********************************************************************************************************************/

Expand All @@ -236,17 +238,21 @@ TEST_CASE("HMAC Digest: test various supported/unsupported types", "[MAC][access
char out[MAX_MSGDIGEST_BUFFER_SIZE];
char hexOut[MAX_MSGDIGEST_BUFFER_SIZE];

StringList types = {"MD4", "MD5", "RIPEMD160", "SHA1", "SHA224", "SHA256", "SHA384", "SHA512"};
StringList types = {"MD4", "MD5", "SHA1", "SHA224", "SHA256", "SHA384", "SHA512"};
StringList digests = {"6b3057137a6e17613883ac25a628b1b3",
"820117c62fa161804efb3743cc838b81",
"ccf3230972bcf229fb3b16741495c74a72bbdd14",
"0e3dfdfb04a3dfcd4d195cb1a5e4186feab2e0c1",
"00a6f43962e2b35cb2491f81d59ef2268309c8cde744891188c9b855",
"149333e1db61f9a18a91a13aca0370b89cec4c546360b85530ae2da97b7b1cb9",
"da500bdc5318bfce7a8a094b9da1d8ac901e145d73cc7039e41c6bff4451734269689465ca39e861b9026b481d3cc9db",
"e075c8b0637bc4fb82cdca66a2b72e3c1734f4f78c803e5db7ca879f85f16b2e057fa62bdd09eef5bbea562990d52a671927033056"
"314c19092263f753ecd019"};

#ifndef OPENSSL_IS_BORINGSSL // RIPEMD160 is not available on BoringSSL?
types.push_back("RIPEMD160");
digests.push_back("ccf3230972bcf229fb3b16741495c74a72bbdd14");
#endif

StringList::iterator digestIter = digests.begin();
for (String digestType : types) {
size_t outLen = cryptoMessageDigestGet(digestType.c_str(), data.c_str(), data.length(), key.c_str(), key.length(), out,
Expand Down
Loading