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.

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

Remove crypto dependency from sdk #2952

Merged
merged 1 commit into from
May 28, 2024
Merged

Remove crypto dependency from sdk #2952

merged 1 commit into from
May 28, 2024

Conversation

sbiscigl
Copy link
Contributor

@sbiscigl sbiscigl commented May 9, 2024

Description of changes:

Removes the direct dependency on libcrypto from the SDK and uses c-cal from crt instead simplifying the dependency set in the SDK.

Mostly borrowed from pull/2460 with bug fixes.

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sbiscigl sbiscigl force-pushed the remove-crypto-dep branch 7 times, most recently from e61128a to 8f4d559 Compare May 13, 2024 20:03
@jmklix jmklix linked an issue May 15, 2024 that may be closed by this pull request
@sbiscigl sbiscigl force-pushed the remove-crypto-dep branch 5 times, most recently from 4e0fa6b to 2fc815e Compare May 20, 2024 14:46
CMakeLists.txt Show resolved Hide resolved
cmake/external_dependencies.cmake Outdated Show resolved Hide resolved
src/aws-cpp-sdk-core/include/aws/core/utils/Array.h Outdated Show resolved Hide resolved
auto hashResult = m_hash.Calculate(canonicalRequestString);
if (!hashResult.IsSuccess())
auto sha256Digest = HashingUtils::CalculateSHA256(canonicalRequestString);
if (sha256Digest.GetLength() == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

its still computing result, no? why does this have to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CalculateSHA256 returns a byte buf, which according to the CRT API is empty if it "failed". since this utility function hides away cipher state we use the empty/non-empty signal to determine if it was successfully calculated.

@sbiscigl sbiscigl force-pushed the remove-crypto-dep branch from 2fc815e to fdcf989 Compare May 20, 2024 17:54
auto hashResult = m_HMAC->Calculate(ByteBuffer((unsigned char*)simpleDate.c_str(), simpleDate.length()),
ByteBuffer((unsigned char*)signingKey.c_str(), signingKey.length()));
auto kDate = HashingUtils::CalculateSHA256HMAC(
ByteBuffer((unsigned char *) simpleDate.c_str(), simpleDate.length()),
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just add bytebuffer from string constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ByteBuffer is an Array, but yeah that would be a good convenience function let me add that

if (str.size() == 0)
{
Sha256 hash;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we even need object for this anymore? can we just make it a static helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so in theory yes, we could make a static helper function but it would require a major re-write of of how hashing works. specifically in Sha256 we separate implementation using pimpl using a unique ptr to a Hash type. because of this pimpl indirection we dont know the underlying type and invoke it using its virtual function. We could of course remove this pimpl indirection but the delta of that change would prove to be much more than and just declaring a temp hash var and using the vtable. IMO it would create a diff that would make this change more risky than it already is.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could just have a static method in Sha256 that creates an instance and calls hash on it, instead of doing it all over the place? or have oneshot variant that resets state to working after hash? but whatever, not something to lose sleep over.

{
return {std::move(resultBuffer)};
}
Crt::ByteBufDelete(resultBuffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

how does error handling work here? we just return empty cryptoBuffer and nothing else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah thats all from the original API which heavily relied on the virtual operator bool() const { return Good(); } operator to check if the cipher was in a erroronus state or not. which is why i change in this PR to catpure the state of being finished or ready. The error handling here would be to check the ciphers boolean operator before using the return value per the original API.

@sbiscigl sbiscigl force-pushed the remove-crypto-dep branch 3 times, most recently from 3e07631 to 243722c Compare May 22, 2024 14:52

CryptoBuffer& operator=(Crt::ByteBuf&& other) noexcept
{
m_capacity = other.len;
Copy link
Contributor

Choose a reason for hiding this comment

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

add a check that other != parent of this
(i.e. static_cast<Crt::ByteBuf*>(this) != &other) or smth like this
otherwise you risk erasing the parent

Copy link
Contributor Author

@sbiscigl sbiscigl May 22, 2024

Choose a reason for hiding this comment

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

CryptoBuffer != Crt::ByteBuf we actually want to delete the parent unconditionally as we are taking ownership of the underlying buffer from the bytebuf. since this is a move operation other is invalid after this operation and CryptoBuffer should always take owneship from parent, and it cannot move from self as they are different types.


HashResult GetHash() override;

// @private but made public for the sake of make_shared.
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tried to fix perfect forwarding in MakeShared / make_shared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed offline, removing comment, its just a move ctor from the underlying type

@@ -109,7 +109,7 @@ namespace
static Aws::String GetTestContainerName()
{
// "-" is not allowed in container name, convert uuid to its hex.
static const std::string suffix = HashingUtils::HexEncode(ByteBuffer(Aws::Utils::UUID::RandomUUID())).c_str();
static const std::string suffix = HashingUtils::HexEncode(ByteBuffer(Aws::Utils::UUID::RandomUUID().operator ByteBuffer())).c_str();
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you have to change this?
it seems to be an API backward incompatible change.

Copy link
Contributor Author

@sbiscigl sbiscigl May 22, 2024

Choose a reason for hiding this comment

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

This is because of @DmitriyMusatkin 's requested change for adding a string to array ctor.. UUID is a special class in that it has a
operator Aws::String() const
AND
operator ByteBuffer() const

so when we convert a UUID to byte buf the ctor is ambigious now because there is a ctor with both string and byte buf. Is this a breaking API change? sort of? if a class has a operator defined for both String and ByteBuf and used in a constructor for Array. I think we can rationalize in making this change as we are the only people likely doing that.

@sbiscigl sbiscigl force-pushed the remove-crypto-dep branch from a92d069 to e5c6fd6 Compare May 22, 2024 20:06
@sbiscigl sbiscigl force-pushed the remove-crypto-dep branch 2 times, most recently from 592ae58 to a623e04 Compare May 23, 2024 15:39
---------

Co-authored-by: Jonathan M. Henson <henso@amazon.com>
@sbiscigl sbiscigl force-pushed the remove-crypto-dep branch from a623e04 to 45a5dc1 Compare May 28, 2024 15:08
@sbiscigl sbiscigl marked this pull request as ready for review May 28, 2024 16:58
@sbiscigl sbiscigl merged commit b652aae into main May 28, 2024
4 checks passed
@sbiscigl sbiscigl deleted the remove-crypto-dep branch May 28, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libcrypto linking issue with AWS SDK 1.11.285
3 participants