Skip to content

AWS_CSM_ENABLED with DefaultMonitoring Does not work because Header Look Up is case-sensitive and fails to pick up a needed key #1536

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

Closed
2 tasks done
vinayan3 opened this issue Dec 10, 2020 · 7 comments
Labels
bug This issue is a bug. closed-for-staleness

Comments

@vinayan3
Copy link

vinayan3 commented Dec 10, 2020

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
A clear and concise description of what the bug is.
export AWS_CSM_ENABLED=true causes the SDK to assert.

/aws-cpp-sdk/aws-core/source/http/standard/StandardHttpRequest.cpp:73: virtual const String& Aws::Http::Standard::StandardHttpRequest::GetHeaderValue(const char*) const: Assertion `iter != headerMap.end()' failed.

SDK version number
1.7.211
Platform/OS/Hardware/Device
What are you running the sdk on? Linux/Ubuntu 18.04 Desktop/Dell XPS 15 9500

To Reproduce (observed behavior)
Steps to reproduce the behavior (please share code)

  1. Create a small client example which puts an object to S3.
  2. Before running binary set export AWS_CSM_ENABLED=true.
  3. See assert

Expected behavior
A clear and concise description of what you expected to happen.
No Assert

Logs/output
Stack Trace

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007f3990252921 in __GI_abort () at abort.c:79
#2  0x00007f399024248a in __assert_fail_base (fmt=0x7f39903c9750 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x55e1a1ba87de "iter != headerMap.end()", file=file@entry=0x55e1a1ba8790 "third_party/aws-cpp-sdk/aws-core/source/http/standard/StandardHttpRequest.cpp", line=line@entry=73, function=function@entry=0x55e1a1ba8860 <Aws::Http::Standard::StandardHttpRequest::GetHeaderValue[abi:cxx11](char const*) const::__PRETTY_FUNCTION__> "virtual const String& Aws::Http::Standard::StandardHttpRequest::GetHeaderValue(const char*) const") at assert.c:92
#3  0x00007f3990242502 in __GI___assert_fail (assertion=0x55e1a1ba87de "iter != headerMap.end()", file=0x55e1a1ba8790 "third_party/aws-cpp-sdk/aws-core/source/http/standard/StandardHttpRequest.cpp", line=73, function=0x55e1a1ba8860 <Aws::Http::Standard::StandardHttpRequest::GetHeaderValue[abi:cxx11](char const*) const::__PRETTY_FUNCTION__> "virtual const String& Aws::Http::Standard::StandardHttpRequest::GetHeaderValue(const char*) const") at assert.c:101
#4  0x000055e1a1582923 in Aws::Http::Standard::StandardHttpRequest::GetHeaderValue[abi:cxx11](char const*) const (this=0x7f398fe3d410, headerName=0x55e1a1ba4b6e "X-Amz-Security-Token") at third_party/aws-cpp-sdk/aws-core/source/http/standard/StandardHttpRequest.cpp:73
#5  0x000055e1a157c746 in Aws::Http::HttpRequest::GetAwsSessionToken[abi:cxx11]() const (this=0x7f398fe3d410) at third_party/aws-cpp-sdk/aws-core/include/aws/core/http/HttpRequest.h:309
#6  0x000055e1a1579ea1 in Aws::Monitoring::FillOptionalApiAttemptFieldsToJson (json=..., request=0x7f398fe3d410, outcome=..., metricsFromCore=...) at third_party/aws-cpp-sdk/aws-core/source/monitoring/DefaultMonitoring.cpp:154
#7  0x000055e1a157b46e in Aws::Monitoring::DefaultMonitoring::CollectAndSendAttemptData (this=0x7f398fe633c0, serviceName="S3", requestName="PutObject", request=std::shared_ptr<const Aws::Http::HttpRequest> (use count 3, weak count 0) = {...}, outcome=..., metricsFromCore=..., context=0x7f398fe3fe10) at third_party/aws-cpp-sdk/aws-core/source/monitoring/DefaultMonitoring.cpp:273
#8  0x000055e1a157a924 in Aws::Monitoring::DefaultMonitoring::OnRequestSucceeded (this=0x7f398fe633c0, serviceName="S3", requestName="PutObject", request=std::shared_ptr<const Aws::Http::HttpRequest> (use count 3, weak count 0) = {...}, outcome=..., metricsFromCore=..., context=0x7f398fe3fe10) at third_party/aws-cpp-sdk/aws-core/source/monitoring/DefaultMonitoring.cpp:224
#9  0x000055e1a15704c0 in Aws::Monitoring::OnRequestSucceeded (serviceName="S3", requestName="PutObject", request=std::shared_ptr<const Aws::Http::HttpRequest> (use count 3, weak count 0) = {...}, outcome=..., metricsFromCore=..., contexts=std::vector of length 1, capacity 1 = {...}) at third_party/aws-cpp-sdk/aws-core/source/monitoring/MonitoringManager.cpp:60
#10 0x000055e1a15139b3 in Aws::Client::AWSClient::AttemptExhaustively (this=0x7f398fe5d2d0, uri=..., request=..., method=Aws::Http::HttpMethod::HTTP_PUT, signerName=0x55e1a1b9e980 <Aws::Auth::SIGV4_SIGNER> "SignatureV4") at third_party/aws-cpp-sdk/aws-core/source/client/AWSClient.cpp:187
#11 0x000055e1a1519df2 in Aws::Client::AWSXMLClient::MakeRequest (this=0x7f398fe5d2d0, uri=..., request=..., method=Aws::Http::HttpMethod::HTTP_PUT, signerName=0x55e1a1b9e980 <Aws::Auth::SIGV4_SIGNER> "SignatureV4") at third_party/aws-cpp-sdk/aws-core/source/client/AWSClient.cpp:797
#12 0x000055e1a12d5954 in Aws::S3::S3Client::PutObject (this=0x7f398fe5d2d0, request=...) at third_party/aws-cpp-sdk/aws-s3/source/S3Client.cpp:3290

Additional context
The reason this is happening is because the header's key is in lower case

headersMap = {
 ... 
"x-amz-security-token": "some value"
}

The value being looked up is

AWS_SECURITY_TOKEN = "X-Amz-Security-Token";

A case insensitive look up is needed

@vinayan3 vinayan3 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 10, 2020
@KaibaLopez
Copy link
Contributor

Hi @vinayan3 ,
I am not able to reproduce this, but to be honest I may be missing something, I simply set the environment variable AWS_CSM_ENABLED to true and made an upload object request... Is there anything else?
Can you walk me through how you get to see this bug? sample code and cmake parameters would help a lot.
Also, is there a reason why you are using an older version of the SDK or can you update to the newest?

@KaibaLopez KaibaLopez added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. and removed needs-triage This issue or PR still needs to be triaged. labels Dec 10, 2020
@vinayan3
Copy link
Author

Sorry I wasn't more clear @KaibaLopez.

I'm using a slightly older version because the library hasn't been updated yet in the source tree.

In terms of reproducing. I think reading the code could help you understand how this could happen.

  1. Header values are set by lowering them.
    https://github.com/aws/aws-sdk-cpp/blob/master/aws-cpp-sdk-core/source/http/standard/StandardHttpRequest.cpp#L67
void StandardHttpRequest::SetHeaderValue(const char* headerName, const Aws::String& headerValue)
{
    headerMap[StringUtils::ToLower(headerName)] = StringUtils::Trim(headerValue.c_str());
}
  1. Header values accecssor doesn't change the case of the headerName the caller is requesting.
    https://github.com/aws/aws-sdk-cpp/blob/master/aws-cpp-sdk-core/source/http/standard/StandardHttpRequest.cpp#L60
const Aws::String& StandardHttpRequest::GetHeaderValue(const char* headerName) const
{
    auto iter = headerMap.find(headerName);
    assert (iter != headerMap.end());
    return iter->second;
}
  1. The header that the DefaultMonitoring is getting is Aws::Http::AWS_SECURITY_TOKEN. It's case has not be lowered like most of the other values in the file https://github.com/aws/aws-sdk-cpp/blob/master/aws-cpp-sdk-core/source/http/HttpRequest.cpp#L15
const char AWS_SECURITY_TOKEN[] = "X-Amz-Security-Token";
const char ACCEPT_HEADER[] = "accept";
const char ACCEPT_CHAR_SET_HEADER[] = "accept-charset";
const char ACCEPT_ENCODING_HEADER[] = "accept-encoding";

So what's happening is the security token is being set by using the string AWS_SECURITY_TOKEN and when it's later retrieved by the same constant it cannot be found.

The reason maybe my application is currently crashing is I'm developing locally and running it via GDB with break on exceptions, asserts, etc... on.

If you are still having trouble with this I can write a unit test. I'm quite confident that a unit test would demonstrate the behavior that I'm observing.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Dec 11, 2020
@KaibaLopez
Copy link
Contributor

Unit test would help a lot yea.
I do see the problem you are referring to btw, I just need to be able to reproduce to understand the whole story and setting AWS_CSM_ENABLED=true doesn't seem to do it on my side.
Also, thanks for bringing this up to us and i don't know if you just prefer not to, but we do take/encourage contributions... would you want to make a PR on this?

@KaibaLopez KaibaLopez added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. label Dec 14, 2020
@github-actions
Copy link

Greetings! It looks like this issue hasn’t been active in longer than a week. We encourage you to check if this is still an issue in the latest release. Because it has been longer than a week since the last update on this, and in the absence of more information, we will be closing this issue soon. If you find that this is still a problem, please feel free to provide a comment or add an upvote to prevent automatic closure, or if the issue is already closed, please feel free to open a new one.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Dec 19, 2020
@vinayan3
Copy link
Author

Please don't close this. I'll have to do some work with the debugger next week to figure out the exact code path which set the header. I'll also try with a prod build to see if the issue replicates.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. labels Dec 20, 2020
@vinayan3
Copy link
Author

vinayan3 commented Dec 29, 2020

I still haven't gotten down to why you can't reproduce this.

I put this in GDB and figured out that this is the line using the env-var to set the value:

inline void SetAwsSessionToken(const Aws::String& value)
{
SetHeaderValue(AWS_SECURITY_TOKEN, value);
}

This is the line which is accessing the value by the header:

inline const Aws::String& GetAwsSessionToken() const
{
return GetHeaderValue(AWS_SECURITY_TOKEN);
}

As for putting up a PR I will try to go through the steps to get the repo setup and use the CMake build system. I currently use a different one for the project I am on.

@github-actions
Copy link

Greetings! Sorry to say but this is a very old issue that is probably not getting as much attention as it deservers. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Dec 30, 2021
@github-actions github-actions bot closed this as completed Jan 4, 2022
amit-schreiber-firebolt pushed a commit to firebolt-analytics/aws-sdk-cpp that referenced this issue May 8, 2025
amit-schreiber-firebolt pushed a commit to firebolt-analytics/aws-sdk-cpp that referenced this issue May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. closed-for-staleness
Projects
None yet
Development

No branches or pull requests

2 participants