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

S3 requests is not correctly signed #238

Closed
littlekid440 opened this issue Aug 24, 2020 · 20 comments · Fixed by #382
Closed

S3 requests is not correctly signed #238

littlekid440 opened this issue Aug 24, 2020 · 20 comments · Fixed by #382
Labels

Comments

@littlekid440
Copy link

My request are failing with the following

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>There were headers present in the request which were not signed</Message><HeadersNotSigned>x-amz-security-token</HeadersNotSigned><RequestId>redacted</RequestId><HostId>redacted</HostId></Error>
@isoos
Copy link
Contributor

isoos commented Aug 25, 2020

@littlekid440 what package and method were you using?

@littlekid440
Copy link
Author

littlekid440 commented Aug 25, 2020

@isoos this is for aws_s3_api. I have tried a few method and i'm facing the same issue with all of them. eg. service.putObject

@Schwusch
Copy link
Collaborator

Schwusch commented Sep 6, 2020

I wonder if this is a consequence of this commit.

@Schwusch
Copy link
Collaborator

Schwusch commented Sep 13, 2020

I've digged further into this, and I found when looking at the signing documentation

When you add the X-Amz-Security-Token parameter to the query string, some services require that you include this parameter in the canonical (signed) request. For other services, you add this parameter at the end, after you calculate the signature. For details, see the API reference documentation for that service.

Somewhere in the specs there should be an indication whether the header should be signed or not. I haven't found it yet though.
Have you seen it @isoos ?

@isoos
Copy link
Contributor

isoos commented Sep 13, 2020

Yeah, it seems to be a non-documented/implicit knowledge.

https://github.com/aws/aws-sdk-js seems to use the presence of credentials.sessionToken as when to make it part of the signed request. Maybe we could use the same, and just add it to the singed part when it is present.

@Schwusch
Copy link
Collaborator

Schwusch commented Sep 13, 2020

Just checked S3 spec:

"metadata": {
        "signatureVersion": "s3"
}

In the docs, most things go into query params https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html .
I don't really get where the session token goes though.

@jcblancomartinez
Copy link

jcblancomartinez commented Dec 24, 2020

Hi @Schwusch,

I'm also having the same issue as @littlekid440

When signing S3's PutObject operation via aws_s3_api, I'm getting:

AccessDeniedThere were headers present in the request which were not signed
x-amz-security-token

It seems that S3 expects 'X-Amz-Security-Token' header to be signed in and here you are not signing it.

Do you have any ETA on when this will be addressed?

As a workaround, I'm doing the following and it is working fine:

final _s.RestXmlProtocol protocol = _s.RestXmlProtocol(
  client: _client,
  service: 's3',
  region: _region,
  credentials: AwsClientCredentials(
  accessKey: credentials.accessKey,
  secretKey: credentials.secretKey,
  sessionToken: credentials.sessionToken),
  endpointUrl: _domain,
);

final $result = await protocol.send(
  method: 'PUT',
  requestUri:  '/${Uri.encodeComponent(_bucketName)}/${Uri.encodeComponent(fileName)}',
  headers: {'x-amz-security-token': credentials.sessionToken},
  payload: body,
  exceptionFnMap: _exceptionFns,
);
PutObjectOutput output = PutObjectOutput.fromXml($result.body, headers: $result.headers);

Thanks.

@Schwusch
Copy link
Collaborator

Hi @jcblancomartinez,
we don't have any ETA on anything, generally.
We're doing this library in our spare time, and we happily accept pull requests!

I am not sure how to approach this problem, S3 seems to be a different beast with features e.g. presigned urls.
In aws-sdk-js, it has a separate signer, although it may also be signed with V2 or V4 or even a region defined signing version.

@xvrh Do you have any thoughts on how to approach a problem like this?

@Schwusch Schwusch changed the title Failed to S3 requests are not correctly signed Dec 25, 2020
@Schwusch Schwusch changed the title S3 requests are not correctly signed S3 requests is not correctly signed Dec 25, 2020
@Schwusch
Copy link
Collaborator

I think I have a way forward with this.
The trick is to expose signAws4HmacSha256 and other signing functions in the shared_aws_api, and let the protocol class send method take an optional signing function as an argument. The function signature would be something like:

typedef RequestSigner = void Function({
  required Request rq,
  required ServiceMetadata service,
  required String region,
  required AwsClientCredentials credentials,
});

E.g. RestXmlProtocol.send() method signature would then be:

Future<RestXmlResponse> send({
    required String method,
    required String requestUri,
    required Map<String, AwsExceptionFn> exceptionFnMap,
    bool signed = true,
    Map<String, List<String>>? queryParams,
    Map<String, String>? headers,
    dynamic payload,
    String? resultWrapper,
    RequestSigner? requestSigner, // take signer as an argument
  }) async {}

Later, the signing could simply be:

if (requestSigner != null) {
  requestSigner(
    rq: rq,
    service: _endpoint.service,
    region: _endpoint.signingRegion,
    credentials: credentials,
  );
} else {
  signAws4HmacSha256(
    rq: rq,
    service: _endpoint.service,
    region: _endpoint.signingRegion,
    credentials: credentials,
);
}

That way we only have to supply the send() method a signing function when it's not supposed to be V4 signing.
During service code generation, it's possible to then decide what signing function (if any!) the operation needs, and provide it.

@Schwusch Schwusch added the bug label Dec 28, 2021
@Schwusch
Copy link
Collaborator

Now when #334 is merged it is possible to pass a requestSigner function to RestXmlProtocol constructor, which makes it possible to implement S3 signing separately.

@Schwusch
Copy link
Collaborator

@isoos , @xvrh would any of you like to take on developing the S3 RequestSigner?
I think I have tried 20 times without figuring it out. It feels like it shouldn't be too different from the regular signAws4HmacSha256 method, but the specifics confuse me.

Perhaps it would solve #356 as well 🤷‍♂️

@isoos
Copy link
Contributor

isoos commented Mar 20, 2022

Hm.. Looking at the code, it seems that I've forgotten much more about this than I first thought... Do you know any library in other language that implements it and we could use as reference?

@Schwusch
Copy link
Collaborator

Schwusch commented May 3, 2022

I'm shopping around for implementations that are tested and correct, but they seem scarce. I found a JS implementation, although it has some problems as well.
Not sure if it's worth investing more time, if AWS themselves are close to releasing their own SDK:
TylerSustare/aws_sdk#1 (comment)

@chirag729
Copy link

@Schwusch Is this of any use? https://github.com/xtyxtyx/minio-dart/blob/master/lib/src/minio_sign.dart

I've used that library with s3 successfully before

@Schwusch
Copy link
Collaborator

Schwusch commented Jun 9, 2022

I found this as well ☺️
https://pub.dev/packages/aws_signature_v4

@dsyrstad
Copy link

@Schwusch @isoos
I ran into the same issue using aws_s3_api when running on an EC2 instance with IMDS credentials. I saw this PR on a forked repo: https://github.com/intaekim-gea/aws_client/pull/1/files. I found that it works for me if I change my pubspec to reference the forked version:

...
dependency_overrides:
  # Needed to fix AWS S3 request signing because aws_s3_api is broken.
  # See: https://github.com/agilord/aws_client/issues/238
  #  and https://github.com/intaekim-gea/aws_client/pull/1/files
  shared_aws_api:
    git:
      url: https://github.com/intaekim-gea/aws_client.git
      path: shared_aws_api

FWIW, I use both SSM and S3 on my service. SSM worked with shared_aws_api: ^2.0.0, but S3 did not. After this change, S3 now works in addition to SSM.

@Schwusch
Copy link
Collaborator

Thanks @dsyrstad for testing the patch. It looks like it's the same code change as in #364, so I say we bring it in and make a new release.

@dsyrstad
Copy link

dsyrstad commented Jan 20, 2023

@Schwusch Thanks for releasing this patch! I've tested it with S3 and SSM on EC2 and can verify that it works.

However, I'm currently having to use a dependency override to get 2.0.1 because aws_s3_api and aws_ssm_api are on 2.0.0 and still reference aws_shared_api 2.0.0:

dependency_overrides:
  # Needed to fix AWS S3 request signing because aws_s3_api 2.0.0 is broken.
  # See: https://github.com/agilord/aws_client/issues/238
  shared_aws_api: ^2.0.1

So it would be nice if the generated apis were updated when shared_aws_api is updated.

Also, I contributed the IMDS provider for aws_credential_providers (#351) about a year ago, but I still have to use my own version because it relies on shared_aws_api 1.2.0. It would also be nice if this was upgraded to shared_aws_api 2.0.1, and kept in sync.

@Schwusch
Copy link
Collaborator

I think a regular dart pub update would suffice, since the packages are depending on shared_aws_api 2.0.0 or higher.

We can bump the dependencies in aws_credential_providers as well, good catch.

@dsyrstad
Copy link

@Schwusch Good catch - I missed that. My pubspec.lock was pinning it to 2.0.0. It's all good now and does not require the dependency override.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants