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

provider/aws: Fixed the SNS password-protected HTTPS endpoints autoconfirm #9696

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@Ninir
Contributor

Ninir commented Oct 28, 2016

Hi everyone,

Just suggesting a fix for the SNS autoconfirm for HTTPs endpoints having a Basic authentication.
The suggested change makes the user-defined endpoint obfuscating the password.

Related issue:

@radeksimko

This comment has been minimized.

Show comment
Hide comment
@radeksimko

radeksimko Apr 23, 2017

Member

Thanks for raising this PR @Ninir

I think you uncovered a few more issues which we'll need to address, namely

  1. If we can't find the subscription we don't error out -> we should.
  2. Even when we manage to find the right subscription via ARN I think we'll have another issue with spurious diff because Read() will be setting endpoint to "obfuscated" version of the URL which won't match with the config/state. I think this is a candidate for DiffSuppressFunc. I don't have a valid URL on hand to test and verify this, but I'm fairly confident it's a problem.

For the detection/parsing I think we should use net/url instead of regular expressions. It should provide much cleaner API to work with. The url.Userinfo is immutable, but I think it should be easy to reconstruct it with obfuscated password, modify the url.URL and turn it back into string for comparison purposes.

It would be awesome if we had acceptance tests for a confirming http(s) endpoint - I'll see if I can put something together with API Gateway & Lambda.

Feel free to ask if there's anything unclear in the meantime.

Member

radeksimko commented Apr 23, 2017

Thanks for raising this PR @Ninir

I think you uncovered a few more issues which we'll need to address, namely

  1. If we can't find the subscription we don't error out -> we should.
  2. Even when we manage to find the right subscription via ARN I think we'll have another issue with spurious diff because Read() will be setting endpoint to "obfuscated" version of the URL which won't match with the config/state. I think this is a candidate for DiffSuppressFunc. I don't have a valid URL on hand to test and verify this, but I'm fairly confident it's a problem.

For the detection/parsing I think we should use net/url instead of regular expressions. It should provide much cleaner API to work with. The url.Userinfo is immutable, but I think it should be easy to reconstruct it with obfuscated password, modify the url.URL and turn it back into string for comparison purposes.

It would be awesome if we had acceptance tests for a confirming http(s) endpoint - I'll see if I can put something together with API Gateway & Lambda.

Feel free to ask if there's anything unclear in the meantime.

@Ninir

This comment has been minimized.

Show comment
Hide comment
@Ninir

Ninir May 4, 2017

Contributor

Hi @radeksimko

This was probably my very first contribution... a lot to improve :)

Will work on this... thanks for your inputs :) 👍

Contributor

Ninir commented May 4, 2017

Hi @radeksimko

This was probably my very first contribution... a lot to improve :)

Will work on this... thanks for your inputs :) 👍

@catsby catsby removed the waiting-response label May 8, 2017

@hashibot hashibot referenced this pull request in terraform-providers/terraform-provider-aws Jun 13, 2017

Closed

Terraform does not handle SNS subscription for HTTPS w/ Basic Authentication #463

@Ninir Ninir referenced this pull request in terraform-providers/terraform-provider-aws Jun 14, 2017

Merged

SNS: Fixed the password-protected HTTPS endpoints autoconfirm #861

2 of 2 tasks complete

@Ninir Ninir closed this Jun 14, 2017

@Ninir Ninir deleted the Ninir:sns_fix branch Jun 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment