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

KMS service definition: SignResult wrapper is not used #2576

Closed
zbaylin opened this issue Dec 11, 2021 · 9 comments
Closed

KMS service definition: SignResult wrapper is not used #2576

zbaylin opened this issue Dec 11, 2021 · 9 comments
Assignees
Labels
guidance Question that needs advice or information. service-api This issue is caused by the service API, not the SDK implementation.

Comments

@zbaylin
Copy link

zbaylin commented Dec 11, 2021

Describe the bug
The result of the sign operation is wrapped in a field called SignResult, but this is not specified as a resultWrapper in kms/2014-11-01/service-2.json:

"Sign":{
"name":"Sign",
"http":{
"method":"POST",
"requestUri":"/"
},
"input":{"shape":"SignRequest"},
"output":{"shape":"SignResponse"},

Use elsewhere
Amazon's AWS SDKs have a SignResult model, taking the Android one as an example:
https://github.com/aws-amplify/aws-sdk-android/blob/d4116c1909f1ecb5a31edf8588882a6181b73deb/aws-android-sdk-kms/src/main/java/com/amazonaws/services/kms/AWSKMSClient.java#L5660-L5687

This should be an easy fix, let me know if I can just open a PR for it or if there's more that needs to be done.

Thanks!

@zbaylin zbaylin added the needs-triage This issue or PR still needs to be triaged. label Dec 11, 2021
@stobrien89 stobrien89 added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Dec 13, 2021
@stobrien89 stobrien89 self-assigned this Dec 13, 2021
@stobrien89 stobrien89 added guidance Question that needs advice or information. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Dec 13, 2021
@stobrien89
Copy link

Hi @zbaylin,

Thanks for reaching out! The service models we use are defined at the service level, meaning they are written/maintained by service teams and pushed to the (standard(non-amplify): Python, JavaScript, Go, CLI, etc.) AWS SDKs and tools. We unfortunately cannot accept external contributions to the models since they would be overwritten by the automated process that pushes this code from the service teams.

As for the resultWrapper key itself, it's really up to the service that writes the model as to whether or not they want to include specific elements. My guess would be that there's a particular reason they excluded it from the SDK models (I don't see it anywhere else in their model), but I can try to find out why if you'd like!

@stobrien89 stobrien89 added the response-requested Waiting on additional info and feedback. label Dec 13, 2021
@zbaylin
Copy link
Author

zbaylin commented Dec 13, 2021

Hi @stobrien89,

Thanks for the quick response.

We unfortunately cannot accept external contributions to the models since they would be overwritten by the automated process that pushes this code from the service teams.

Bummer, but that makes sense.

My guess would be that there's a particular reason they excluded it from the SDK models (I don't see it anywhere else in their model), but I can try to find out why if you'd like!

That would be great if possible, but no worries if not/rush! In the meantime I can just override the key myself in my project.

Thanks again!

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. label Dec 13, 2021
@stobrien89
Copy link

@zbaylin,

No problem! I did want to double-check where you're seeing the key and which SDK/tool you're using— I didn't see the key present in a sample call from the CLI or Boto3(both of which use botocore as the low-level library). A sample response (with any sensitive information redacted) would also be great.

@stobrien89 stobrien89 added the response-requested Waiting on additional info and feedback. label Dec 13, 2021
@zbaylin
Copy link
Author

zbaylin commented Dec 14, 2021

@stobrien89 I'm attempting to add KMS support to ocaml-aws, which uses the botocore service definitions to generate code.

Here's an example request/response (with redactions):
Request URI:

https://kms.us-east-2.amazonaws.com?Version=2014-11-01&Action=Sign&SigningAlgorithm=RSASSA_PSS_SHA_512&Message=OCaml%20AWS&KeyId=[redacted]

Response:

<SignResponse xmlns="https://trent.amazonaws.com/doc/2014-11-01/">
  <SignResult>
    <SigningAlgorithm>RSASSA_PSS_SHA_512</SigningAlgorithm>
    <Signature>[redacted]</Signature>
    <KeyId>[redacted]</KeyId>
  </SignResult>
  <ResponseMetadata>
    <RequestId>[redacted]</RequestId>
  </ResponseMetadata>
</SignResponse>

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. label Dec 14, 2021
@stobrien89
Copy link

Hi @zbaylin,

Thanks for the confirmation. I thought you were probably working with the raw response, but just wanted to be sure. The KMS team would have likely requested this information as well.

I've created an internal ticket to inquire about this and will let you know as soon as I have an update!

@stobrien89
Copy link

V487833764

@stobrien89 stobrien89 added the service-api This issue is caused by the service API, not the SDK implementation. label Dec 15, 2021
@zbaylin
Copy link
Author

zbaylin commented Dec 16, 2021

@stobrien89 Thanks!

I actually realized that this is more broad than just Sign. I think every KMS operation is "wrapped" like this:

ListKeys:

<ListKeysResponse xmlns="https://trent.amazonaws.com/doc/2014-11-01/">
  <ListKeysResult>
    <KeyCount>1</KeyCount>
    <Keys>
      <member>
        [redacted]
      </member>
    </Keys>
    <Truncated>false</Truncated>
  </ListKeysResult>
  <ResponseMetadata>
    <RequestId>[redacted]</RequestId>
  </ResponseMetadata>
</ListKeysResponse>

@stobrien89
Copy link

Hi again @zbaylin,

Sorry for the lack of updates!

I was able to get in touch with someone more familiar with the modeling and they mentioned that the lack of the ResultWrapper is due to the fact that KMS uses JSON RPC protocol with version 1.1, which does not use ResultWrapper. More information on AWS protocols can be found here.

They did add that since you don't seem to use similar protocols in the aws-ocaml models, you should be free to do as you wish. Hope this helps!

@github-actions
Copy link

github-actions bot commented Jan 4, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information. service-api This issue is caused by the service API, not the SDK implementation.
Projects
None yet
Development

No branches or pull requests

2 participants