[Core] - Support azure-core version without OpenSSL dependency #2839

vhvb1989 · 2021-09-07T19:08:52Z

An azure-core lib version without OpenSSL is required for the speech service which will consume only UUID (and HTTP in some future).

This PR makes some changes to the current azure core components consuming OpenSSL :

Uuid on Linux : It is used to generate a random buffer. Switching to use the win-implementation which uses the std lib lib.
base64 : Removing the current implementation calling openSSL for base64 for the azure-core for C99 implementation created in the past by @antkmsft
Crypto / Hashing : MD5 and Sha hashing is currently exposed on the public surface and implemented by calling OpenSSL lib. This PR is making all the Cryptographic namespace an optional feature that can be opt-out when building the azure-core lib

fixes: #2790

…-uuid-without-openssl

RickWinter

This change will result in non-unique UUIDs on linux platforms. std::random is not cryptographically secure on linux

sdk/core/azure-core/src/uuid.cpp

…-uuid-without-openssl

…re-sdk-for-cpp into update-uuid-without-openssl

vhvb1989 · 2021-09-09T00:22:07Z

Linux perf results:

64-bit Mersenne Twister by Matsumoto
=== Results ===
Completed 536,943 operations in a weighted-average of 10s (53,693.482313 ops/s, 1.86242e-05 s/op)

OpenSSL
=== Results ===
Completed 44,534 operations in a weighted-average of 10s (4,453.341869 ops/s, 0.00022455 s/op)

sdk/core/azure-core/src/base64.cpp

reviewing

ahsonkhan

I am not sure we should expose the build crypto option, if we can remove the openssl dependency all together.

Will review base64 impl in a bit, do we have sufficient test coverage for it?

sdk/core/azure-core/CHANGELOG.md

sdk/core/azure-core/CMakeLists.txt

sdk/core/azure-core/src/uuid.cpp

ahsonkhan

I think we need more testing of the base64 implementation, especially around edge cases (for integer overflow) and invalid input.

sdk/core/azure-core/src/base64.cpp

ahsonkhan · 2021-09-24T22:38:30Z

sdk/core/azure-core/src/base64.cpp

+
+  while (sourceIndex < static_cast<int64_t>(inputSize) - 4)
+  {
+    int64_t result = base64Decode(inputPtr + sourceIndex);


If result is negative, it means invalid input and we should throw.

We need test cases like the following, which are invalid input:
"A" - too small
"AB" - too small
"ABC" - too small
"ABCD==" - not a multiple of 4
"ABCD====" - invalid padding count
"A%%%" - non-base64 chars

The input has to be of characters from "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"

ahsonkhan · 2021-09-24T22:40:29Z

sdk/core/azure-core/src/base64.cpp

+    i0 |= i3;
+    i0 |= i2;
+
+    base64WriteThreeLowOrderBytes(destinationPtr, i0);


Same here and elsewhere - add input validation checks (if i0 < 0) and throw for invalid input

https://github.com/Azure/azure-sdk-for-c/blob/d7b388c68b07ee769dd9b061114b93336fd53385/sdk/src/azure/core/az_base64.c#L487-L490

ahsonkhan · 2021-09-24T22:42:24Z

sdk/core/azure-core/src/base64.cpp

+    destination.resize(resultSize);
+    destination.shrink_to_fit();


Resizing strings and vectors after the fact seems to be a relatively expensive operation.
@CaseyCarter how do we avoid this, and maybe just create the vector of the right size at the end without having to do linear data copy or re-allocation? In .NET/C#, this was relatively easy with span/slicing for buffers.

sdk/core/azure-core/src/uuid.cpp

sdk/core/azure-core/test/perf/inc/azure/core/test/uuid_test.hpp

CaseyCarter · 2021-09-25T00:56:33Z

sdk/core/azure-core/src/base64.cpp

+  int64_t sourceIndex = 0;
+  int64_t destinationIndex = 0;


Ditto "indices into memory buffers, these should be size_t"

sdk/core/azure-core/src/base64.cpp

CaseyCarter · 2021-09-25T01:18:10Z

sdk/core/azure-core/src/base64.cpp

+    int64_t result = base64Decode(inputPtr + sourceIndex);
+    base64WriteThreeLowOrderBytes(destinationPtr, result);
+    destinationPtr += 3;
+    destinationIndex += 3;


destinationIndex is unused.

sdk/core/azure-core/src/base64.cpp

Co-authored-by: Ahson Khan <[email protected]> Co-authored-by: Casey Carter <[email protected]>

Co-authored-by: Ahson Khan <[email protected]>

antkmsft

Casey has some good feedback, can we at least open work items? Re: random seems pretty important. Prefix increment, not so important, but I'd take it as well, but I am biased.

…-uuid-without-openssl

…re-sdk-for-cpp into update-uuid-without-openssl

…-uuid-without-openssl

vhvb1989 added 6 commits September 3, 2021 17:43

perf test uuid

7fe324a

use win impl for uuid on linux

7b19eb9

use c99 base64 encode-decode

91a40e5

make crypto optional

91f05e4

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into update…

3b2d411

…-uuid-without-openssl

return empty vector for base64

5f303ed

vhvb1989 requested review from RickWinter, ahsonkhan, antkmsft and gearama as code owners September 7, 2021 19:08

vhvb1989 self-assigned this Sep 7, 2021

vhvb1989 added Azure.Core Client This issue points to a problem in the data-plane of the library. Cognitive - Speech labels Sep 7, 2021

vhvb1989 added this to the [2021] October milestone Sep 7, 2021

vhvb1989 added 3 commits September 7, 2021 19:13

cl

683bcdf

cl

30d11df

win - compile - fixes

5a5856d

RickWinter requested changes Sep 8, 2021

View reviewed changes

RickWinter reviewed Sep 8, 2021

View reviewed changes

sdk/core/azure-core/src/uuid.cpp Show resolved Hide resolved

vhvb1989 added 4 commits September 8, 2021 21:48

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into update…

7a99912

…-uuid-without-openssl

using 64-bit Mersenne Twister for rand gen

a80be57

comments

35b79e9

Merge branch 'update-uuid-without-openssl' of github.com:vhvb1989/azu…

f80b320

…re-sdk-for-cpp into update-uuid-without-openssl

vhvb1989 added 2 commits September 9, 2021 11:18

msvc compile fixes

2d25f30

update

3291ce3

vhvb1989 requested a review from RickWinter September 9, 2021 18:34

ahsonkhan reviewed Sep 9, 2021

View reviewed changes

sdk/core/azure-core/src/base64.cpp Show resolved Hide resolved

fix on cast

fb2ba79

RickWinter previously approved these changes Sep 13, 2021

View reviewed changes

RickWinter self-requested a review September 13, 2021 18:49

ahsonkhan reviewed Sep 24, 2021

View reviewed changes

sdk/core/azure-core/CHANGELOG.md Outdated Show resolved Hide resolved

sdk/core/azure-core/CHANGELOG.md Outdated Show resolved Hide resolved

sdk/core/azure-core/CMakeLists.txt Outdated Show resolved Hide resolved

sdk/core/azure-core/src/uuid.cpp Outdated Show resolved Hide resolved

ahsonkhan reviewed Sep 24, 2021

View reviewed changes

sdk/core/azure-core/src/uuid.cpp Show resolved Hide resolved

ahsonkhan reviewed Sep 24, 2021

View reviewed changes

CaseyCarter reviewed Sep 25, 2021

View reviewed changes

vhvb1989 and others added 2 commits September 27, 2021 10:16

Apply suggestions from code review

a8a0703

Co-authored-by: Ahson Khan <[email protected]> Co-authored-by: Casey Carter <[email protected]>

Apply suggestions from code review

53de278

Co-authored-by: Ahson Khan <[email protected]>

RickWinter modified the milestones: [2021] October, [2021] November Oct 7, 2021

antkmsft approved these changes Oct 18, 2021

View reviewed changes

vhvb1989 added 11 commits October 25, 2021 23:08

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into update…

70b5153

…-uuid-without-openssl

Merge branch 'update-uuid-without-openssl' of github.com:vhvb1989/azu…

d6642da

…re-sdk-for-cpp into update-uuid-without-openssl

rever

ffc484d

rever

c03066e

update

3f8d987

update

326b465

update

71c2a30

update

75e7020

updates

9864fd4

avoid shrink

39b6342

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into update…

77459a0

…-uuid-without-openssl

vhvb1989 enabled auto-merge (squash) October 27, 2021 20:59

vhvb1989 merged commit 3be793d into Azure:main Oct 27, 2021

[Core] - Support azure-core version without OpenSSL dependency #2839

[Core] - Support azure-core version without OpenSSL dependency #2839

Uh oh!

Conversation

vhvb1989 commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RickWinter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vhvb1989 commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CaseyCarter Sep 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CaseyCarter Sep 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

antkmsft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vhvb1989 commented Sep 7, 2021 •

edited

Loading

vhvb1989 commented Sep 9, 2021 •

edited

Loading