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.

Sign up for GitHub

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

Jump to bottom

resource/aws_sns_platform_application: Remove hashing StateFunc on platform credential and principal #3894

Merged
merged 2 commits into from
Jul 14, 2020
Merged

resource/aws_sns_platform_application: Remove hashing StateFunc on platform credential and principal #3894

merged 2 commits into from
Jul 14, 2020

Conversation

rhodrid
Copy link
Contributor

@rhodrid rhodrid commented Mar 23, 2018 •
edited
Loading

When a APNS certificate is renewed and the private key remains the same the new certificate is detected and an attempt is made to update the SNS platform application. This fails because the hashed value for private key (platform credential) is sent along with the new certificate (platform principal) causing the AWS API to reject the request.

@ghost ghost added the size/XS Managed by automation to categorize the size of a PR. label Mar 23, 2018
@rhodrid rhodrid changed the title Remove hashing StateFunc on platform credential and principal resource/aws_sns_platform_application: Remove hashing StateFunc on platform credential and principal Mar 23, 2018
@bflad bflad added bug Addresses a defect in current functionality. service/sns Issues and PRs that pertain to the sns service. labels Mar 23, 2018
@bflad
Copy link
Contributor

bflad commented Mar 27, 2018

Hi @rhodrid 👋 Hmmm. We'll likely need a different way to handle this as this PR will currently be a breaking change for anyone who is currently using the resource.

@bflad bflad added the thinking label Mar 27, 2018
@rhodrid
Copy link
Contributor Author

rhodrid commented Mar 28, 2018

Howdy 🖖I couldn't see another way to do it... but then I'm not very familiar with the code base.

@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from fd11954 to 724d23b Compare April 27, 2018 09:17
@ghost ghost added the size/XS Managed by automation to categorize the size of a PR. label Apr 27, 2018
@rhodrid
Copy link
Contributor Author

rhodrid commented Jul 3, 2018

There is a follow-on failure that I didn't notice when I first reported the issue - the API call will fail because only the change gets sent to the SNS API endpoint.

Error updating SNS platform application: InvalidParameter: Invalid parameter: 
Attributes Reason: PlatformPrincipal attribute provided without PlatformCredential

My "solution" to this is to delete the resource and re-create it. It's not a problem for me at the moment as the resource is not customer facing at this stage of its life but it's a less than ideal way of dealing with it.

Is there anyway to force sending both PlatformPrincipal & PlatformCredential?

@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from 724d23b to 9ae05d4 Compare July 3, 2018 10:35
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. size/S Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Jul 3, 2018
@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from a497a5b to 2035e91 Compare July 3, 2018 15:30
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Jul 3, 2018
@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from 2035e91 to 0c01252 Compare July 4, 2018 15:34
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Jul 4, 2018
@rhodrid
Copy link
Contributor Author

rhodrid commented Jul 10, 2018

I've updated the PR so if platform_principal changes then platform_credential will be added to the request payload. As it stands this PR fixes the issues I was experiencing with this resource.

@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from 0c01252 to 96533c9 Compare August 23, 2018 11:56
@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from 96533c9 to b7d3625 Compare October 4, 2018 08:00
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Oct 4, 2018
@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from b7d3625 to fb3faa1 Compare January 9, 2019 10:18
@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch 2 times, most recently from b15b71a to 360e7e1 Compare February 6, 2019 15:35
@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from 360e7e1 to 95a9462 Compare May 7, 2019 14:40
@aeschright aeschright requested a review from a team June 25, 2019 19:23
@bflad bflad mentioned this pull request Nov 12, 2019
Closed
@bflad bflad added this to the v3.0.0 milestone Nov 25, 2019
@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from 95a9462 to 74c8097 Compare May 19, 2020 09:40
rhodrid added 2 commits May 19, 2020 11:01
@rhodrid
Remove hashing StateFunc on platform credential and principal
fad999e 
When a APNS certificate is renewed and the private key remains
the same the new certificate is detected and an attempt is made
to update the SNS platform application. This fails because the
hashed value for private key (platform credential) is sent along
with the new certificate (platform principal) causing the AWS API
to reject the request.
@rhodrid
Send the PlatformCredential if the PlatformPrincipal has changed
3dd4c79
@rhodrid rhodrid force-pushed the rd-fix-new-apns-cert branch from 74c8097 to 3dd4c79 Compare May 19, 2020 10:02
@breathingdust breathingdust mentioned this pull request May 19, 2020
Closed
@bflad bflad self-assigned this Jul 14, 2020
bflad added a commit that referenced this pull request Jul 14, 2020
@bflad
resource/aws_sns_platform_application: Finalize platform_credential a…
a05aa9e 
…nd platform_princial hash removal

Reference: #3894
Reference: #9951
Reference: #12085
Reference: #13406

This also attempts to prevent the SetPlatformApplicationAttributes API call if no API updates need to occur. While we are in the midst of breaking changes and since this resource cannot be acceptance tested by the HashiCorp maintainers, this also fixes some other technical debt issues.
@bflad
Copy link
Contributor

bflad commented Jul 14, 2020

Thanks so much for this, @rhodrid, its now been pulled in as part of our version 3.0.0. 👍

@bflad bflad merged commit 91f448c into hashicorp:master Jul 14, 2020
bflad added a commit that referenced this pull request Jul 14, 2020
@bflad
Update CHANGELOG for #3894 and #9685
09e2f73
@ghost
Copy link

ghost commented Jul 31, 2020

This has been released in version 3.0.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Aug 13, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Aug 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@bflad bflad

Labels
bug Addresses a defect in current functionality. service/sns Issues and PRs that pertain to the sns service. size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@rhodrid @bflad