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

Ninir · 2017-06-14T16:48:55Z

Description

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.
Migrated from hashicorp/terraform#9696

Related issues:

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

TODOs

Errors If we can't find the subscription
Prefer the use of package net/url instead of regular expressions

catsby · 2017-07-07T16:43:35Z

Hey @Ninir how do we stand here, is this something you think you'll be able to wrap up?

Ninir · 2017-07-10T08:49:13Z

Hey @catsby
I'll have more time starting from tomorrow, so will get this into shape.
Already have the local work, just need to check a secure endpoint, using a personal VM at the moment.

Ninir · 2017-07-11T17:57:41Z

Here it is! I was able to work with a custom VM using custom code with htpasswd, to replicate this change.

In order to test basic auth, AWS forces you to use https. Thus, I think we could probably set up a testacc using CloudFront pointing to an ELB with a VM...
Thus, we could use Radek's code, and check the auth + subscription (a lot of stuff there).

@radeksimko To better explain why I didn't implement DiffSuppressFunc:
when the check to find a valid subscription is made, we are calling snsconn.ListSubscriptionsByTopic, which returns this kind of response:

Response sns/ListSubscriptionsByTopic Details:
---[ RESPONSE ]--------------------------------------
HTTP/1.1 200 OK

-----------------------------------------------------
<ListSubscriptionsByTopicResponse xmlns="http://sns.amazonaws.com/doc/2010-03-31/">
  <ListSubscriptionsByTopicResult>
    <Subscriptions>
      <member>
        <Owner>123456789643</Owner>
        <Endpoint>https://myuser:****@mydomain.com</Endpoint>
        <Protocol>https</Protocol>
        <SubscriptionArn>arn:aws:sns:eu-west-1:123456789643:terraform-test-topic:ed16f123-61e3-4cbe-a9er-hzv9fz8vh</SubscriptionArn>
        <TopicArn>arn:aws:sns:eu-west-1:123456789643:terraform-test-topic</TopicArn>
      </member>
    </Subscriptions>
  </ListSubscriptionsByTopicResult>
</ListSubscriptionsByTopicResponse>

When the Read function is then called, snsconn.GetSubscriptionAttributes is called, and returns such stuff:

Response sns/GetSubscriptionAttributes Details:
---[ RESPONSE ]--------------------------------------
HTTP/1.1 200 OK

-----------------------------------------------------
<GetSubscriptionAttributesResponse xmlns="http://sns.amazonaws.com/doc/2010-03-31/">
  <GetSubscriptionAttributesResult>
    <Attributes>
      <entry>
        <key>Owner</key>
        <value>123456789643</value>
      </entry>
      <entry>
        <key>RawMessageDelivery</key>
        <value>false</value>
      </entry>
      <entry>
        <key>TopicArn</key>
        <value>arn:aws:sns:eu-west-1:123456789643:terraform-test-topic</value>
      </entry>
      <entry>
        <key>Endpoint</key>
        <value>https://myuser:mypassword@mydomain.com</value>
      </entry>
      <entry>
        <key>EffectiveDeliveryPolicy</key>
        <value>{&quot;healthyRetryPolicy&quot;:{&quot;minDelayTarget&quot;:20,&quot;maxDelayTarget&quot;:20,&quot;numRetries&quot;:3,&quot;numMaxDelayRetries&quot;:0,&quot;numNoDelayRetries&quot;:0,&quot;numMinDelayRetries&quot;:0,&quot;backoffFunction&quot;:&quot;linear&quot;},&quot;sicklyRetryPolicy&quot;:null,&quot;throttlePolicy&quot;:null,&quot;guaranteed&quot;:false}</value>
      </entry>
      <entry>
        <key>Protocol</key>
        <value>https</value>
      </entry>
      <entry>
        <key>ConfirmationWasAuthenticated</key>
        <value>false</value>
      </entry>
      <entry>
        <key>SubscriptionArn</key>
        <value>arn:aws:sns:eu-west-1:123456789643:terraform-test-topic:ed16f123-61e3-4cbe-a9er-hzv9fz8vh</value>
      </entry>
    </Attributes>
  </GetSubscriptionAttributesResult>
</GetSubscriptionAttributesResponse>

In the end, the endpoint stored in the state is the correct one, and no diff is exposed.
The obfuscated endpoint thing is just happening when getting subscriptions, which match the console behaviour.

radeksimko · 2017-07-12T14:25:55Z

Thus, I think we could probably set up a testacc using CloudFront pointing to an ELB with a VM...
Thus, we could use Radek's code, and check the auth + subscription (a lot of stuff there).

Can't we just use the API Gateway + Lambda? Booting a VM for this purpose feels a bit heavy.

To better explain why I didn't implement DiffSuppressFunc:
when the check to find a valid subscription is made, we are calling snsconn.ListSubscriptionsByTopic, which returns this kind of response

I see, so we only do this *** masking to verify we have/haven't already subscribed to the topic because GetSubscriptionAttributes doesn't provide such information. That's 👌 then.

Ninir · 2017-07-14T15:39:45Z

@radeksimko Well, I tried to build a Basic-protected API Gateway using API Gateway + a custom Authorizer (Lambda). Everything works as expected using the cli, but I can't get the subscription to pass it. Not sure what is issuing here...

If you want, we can discuss it, and I can provide all the work i've made.
Is the test required for this to pass?

radeksimko · 2017-07-16T06:43:48Z

@Ninir I built a few API Gateway integrations in the past using Terraform, none of them used custom authorizer, but I'm confident we can build it.

Just ping me on Slack and/or share the config with me when you have a moment, I'm happy to pair up on this.

Is the test required for this to pass?

I'd prefer to have this covered by a test.

Ninir · 2017-07-20T09:10:40Z

Using API GW+Lambda+SNS is the easiest way to test things, indeed.

As discussed on Slack, it was not possible to set-up an acc test until we could specify headers on responses returned by API Gw.
Now that the API GW Gateway Response resource has been developed & integrated, i was able to set a WWW-Authenticate header on the 401 response returned by API Gateway when having a custom authorizer.

Here is what SNS does when requesting a HTTPS password-protected endpoint, like https://john:doe@mydomain.com/myroute:

Send a request to https://mydomain.com/myroute (without credentials)
If the endpoint returns a WWW-Authenticate: Basic header, SNS then sends a second request to the endpoint WITH credentials

The actual code is written in NodeJS, but could probably be rewritten in Python. @radeksimko Does it really matter?

Also, here are the test results:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription_'                             
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSTopicSubscription_ -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_importBasic
--- PASS: TestAccAWSSNSTopicSubscription_importBasic (16.05s)
=== RUN   TestAccAWSSNSTopicSubscription_basic
--- PASS: TestAccAWSSNSTopicSubscription_basic (14.39s)
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (40.12s)
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (66.68s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	137.276s

radeksimko

This is now looking good with the exception of one tiny issue which is causing the acceptance test to fail in our environment.

Regions... 🤷‍♂️ 😃

radeksimko · 2017-07-24T08:23:12Z

aws/resource_aws_sns_topic_subscription_test.go

+resource "aws_api_gateway_authorizer" "test" {
+  name                             = "tf-acc-test-api-gw-authorizer-%d"
+  rest_api_id                      = "${aws_api_gateway_rest_api.test.id}"
+  authorizer_uri                   = "arn:aws:apigateway:eu-west-1:lambda:path/2015-03-31/functions/${aws_lambda_function.authorizer.arn}/invocations"


Do you mind changing this to

${aws_lambda_function.authorizer.invoke_arn}

? That will make it a bit easier to read and (more importantly) make it work in us-west-2 region which we use for acceptance tests 😉

radeksimko · 2017-07-24T08:24:46Z

The actual code is written in NodeJS, but could probably be rewritten in Python. @radeksimko Does it really matter?

It doesn't matter at all here in this context, I think.

Ninir · 2017-07-24T09:10:12Z

Guh... @radeksimko Need holidays! 😄

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSTopicSubscription_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSTopicSubscription_ -timeout 120m
=== RUN   TestAccAWSSNSTopicSubscription_importBasic
--- PASS: TestAccAWSSNSTopicSubscription_importBasic (17.10s)
=== RUN   TestAccAWSSNSTopicSubscription_basic
--- PASS: TestAccAWSSNSTopicSubscription_basic (14.75s)
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (46.41s)
=== RUN   TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (67.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	67.292s

radeksimko · 2017-07-24T09:56:06Z

No worries, thanks for all the effort in taking it to the finish line. 👍 😃

ghost · 2020-04-11T17:49:22Z

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!

Ninir changed the title ~~SNS: Fixed the password-protected HTTPS endpoints autoconfirm~~ [WIP] SNS: Fixed the password-protected HTTPS endpoints autoconfirm Jun 14, 2017

radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Jun 15, 2017

catsby added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 7, 2017

Ninir force-pushed the f-sns-protected-endpoint branch from 49e5ca4 to 9f7d419 Compare July 11, 2017 17:47

Ninir force-pushed the f-sns-protected-endpoint branch from 9f7d419 to 57e2ec9 Compare July 11, 2017 17:58

Ninir changed the title ~~[WIP] SNS: Fixed the password-protected HTTPS endpoints autoconfirm~~ SNS: Fixed the password-protected HTTPS endpoints autoconfirm Jul 11, 2017

Ninir mentioned this pull request Jul 17, 2017

Added API Gateway Gateway response resource #1168

Merged

3 tasks

Ninir added 2 commits July 19, 2017 20:37

SNS: Fixed the password-protected HTTPS endpoints autoconfirm

0ad35f3

Added acceptance test for HTTPS password-protected endpoints

a615648

Ninir force-pushed the f-sns-protected-endpoint branch from 57e2ec9 to a615648 Compare July 20, 2017 08:53

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 20, 2017

radeksimko suggested changes Jul 24, 2017

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 24, 2017

Prefered usage of invoke_arn attribute

7042d08

radeksimko approved these changes Jul 24, 2017

View reviewed changes

radeksimko merged commit 361012b into hashicorp:master Jul 24, 2017

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 24, 2017

Ninir deleted the f-sns-protected-endpoint branch July 24, 2017 09:57

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Ninir commented Jun 14, 2017 •

edited

Loading

catsby commented Jul 7, 2017

Ninir commented Jul 10, 2017

Ninir commented Jul 11, 2017 •

edited

Loading

radeksimko commented Jul 12, 2017

Ninir commented Jul 14, 2017

radeksimko commented Jul 16, 2017 •

edited

Loading

Ninir commented Jul 20, 2017 •

edited

Loading

radeksimko left a comment

radeksimko Jul 24, 2017

radeksimko commented Jul 24, 2017

Ninir commented Jul 24, 2017

radeksimko commented Jul 24, 2017

ghost commented Apr 11, 2020

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

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

Conversation

Ninir commented Jun 14, 2017 • edited Loading

Description

Related issues:

TODOs

catsby commented Jul 7, 2017

Ninir commented Jul 10, 2017

Ninir commented Jul 11, 2017 • edited Loading

radeksimko commented Jul 12, 2017

Ninir commented Jul 14, 2017

radeksimko commented Jul 16, 2017 • edited Loading

Ninir commented Jul 20, 2017 • edited Loading

radeksimko left a comment

Choose a reason for hiding this comment

radeksimko Jul 24, 2017

Choose a reason for hiding this comment

radeksimko commented Jul 24, 2017

Ninir commented Jul 24, 2017

radeksimko commented Jul 24, 2017

ghost commented Apr 11, 2020

Ninir commented Jun 14, 2017 •

edited

Loading

Ninir commented Jul 11, 2017 •

edited

Loading

radeksimko commented Jul 16, 2017 •

edited

Loading

Ninir commented Jul 20, 2017 •

edited

Loading