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

aws_ec2_client_vpn_endpoint: add federated auth #14171

Merged
merged 11 commits into from
Sep 1, 2020
Merged

aws_ec2_client_vpn_endpoint: add federated auth #14171

merged 11 commits into from
Sep 1, 2020

Conversation

jgeurts
Copy link
Contributor

@jgeurts jgeurts commented Jul 14, 2020

This is an addendum to #13769, to add a test and documentation. Apologies if there is a better way to accomplish this - I wasn't able to commit to the fork source for #13769

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #13401

Release note for CHANGELOG:

aws_ec2_client_vpn_endpoint: add SAML SSO IdP federated authentication support

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'

...

Joe Rayhawk and others added 3 commits June 15, 2020 18:06
aws_ec2_client_vpn_endpoint: add federated auth
e4d74de 
While AWS *can* act as an SSO IdP, I don't think there's a public API for
enabling it. Automated testing might be troublesome.
@jgeurts
Add documentation and test for federated-authentication
56bce56
@jgeurts
Merge remote-tracking branch 'origin/master' into f-aws_ec2_client_vp…
2c64d86 
…n_endpoint-federated-saml-idp-authentication

# Conflicts:
#	aws/resource_aws_ec2_client_vpn_endpoint_test.go
@jgeurts jgeurts requested a review from a team July 14, 2020 17:46
@ghost ghost added size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. needs-triage Waiting for first response or review from a maintainer. service/ec2 Issues and PRs that pertain to the ec2 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jul 14, 2020
@jgeurts jgeurts closed this Jul 15, 2020
@jgeurts jgeurts reopened this Jul 15, 2020
@jgeurts jgeurts mentioned this pull request Jul 17, 2020
Closed
@jgeurts jgeurts mentioned this pull request Aug 19, 2020
Closed
@divyaraghavann
Copy link

Any updates?

@jgeurts
Copy link
Contributor Author

jgeurts commented Aug 27, 2020 •
edited
Loading

@divyaraghavann I don't think the PR needs any more updates, unless I'm mistaken. Last week, @bflad mentioned he would get some eyes on this PR in the next week or so: #13401 (comment), so there should be some movement soon!

DrFaust92
DrFaust92 suggested changes Aug 27, 2020
View reviewed changes
Copy link
Collaborator

@DrFaust92 DrFaust92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor changes

aws/resource_aws_ec2_client_vpn_endpoint_test.go Outdated Show resolved Hide resolved
aws/resource_aws_ec2_client_vpn_endpoint_test.go Outdated Show resolved Hide resolved
jgeurts added 3 commits August 27, 2020 10:40
@jgeurts
Merge remote-tracking branch 'origin/master' into f-aws_ec2_client_vp…
d244ed6 
…n_endpoint-federated-saml-idp-authentication
@jgeurts
Update legacy expression syntax
bc210ad
@jgeurts
Merge remote-tracking branch 'origin/master' into f-aws_ec2_client_vp…
166a55b 
…n_endpoint-federated-saml-idp-authentication

# Conflicts:
#	aws/resource_aws_ec2_client_vpn_endpoint_test.go
@DrFaust92 DrFaust92 added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Aug 27, 2020
@jgeurts
Copy link
Contributor Author

jgeurts commented Aug 27, 2020

Thanks @DrFaust92! Those issues should be resolved! I also merged master to the branch as it was a bit behind.

@divyaraghavann
Copy link

Awesome, thanks y'all! 👍

@bflad bflad self-assigned this Aug 28, 2020
@bflad bflad added this to the v3.5.0 milestone Aug 28, 2020
bflad
bflad suggested changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality looks implemented well but unfortunately the acceptance testing is currently failing. We'll need to get that fixed up before we can merge this. Please reach out if you have any questions or do not have time/ability to fix it up. 👍

aws/resource_aws_ec2_client_vpn_endpoint_test.go
return testAccEc2ClientVpnEndpointConfigAcmCertificateBase() + fmt.Sprintf(`
resource "aws_iam_saml_provider" "default" {
name = "myprovider-%s"
saml_metadata_document = file("./test-fixtures/saml-metadata.xml")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running this acceptance test, it looks like EC2 Client VPN is validating the actual certificate included in this SAML Metadata:

TestAccAwsEc2ClientVpn_serial/Endpoint_federated: resource_aws_ec2_client_vpn_endpoint_test.go:243: Step 1/2 error: terraform failed: exit status 1
stderr:
Error: Error creating Client VPN endpoint: InvalidParameterValue: Cert with DN C=USA, ST=CA, L=San Francisco, O=Salesforce.com, OU=00D24000000pAoA, CN=SelfSignedCert_02Sep2015_182653 expired at Sat Sep 02 12:00:00 UTC 2017
  status code: 400, request id: e8518c9b-d44c-4bb8-a7a3-ff6552af5931

<?xml version="1.0" encoding="UTF-8"?><md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" xmlns:ds="http://www.w3.org/2000/09/xmldsig#" entityID="https://terraform-dev-ed.my.salesforce.com" validUntil="2025-09-02T18:27:19.710Z">
   <md:IDPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
      <md:KeyDescriptor use="signing">
         <ds:KeyInfo>
            <ds:X509Data>
               <ds:X509Certificate>MIIErDCCA5SgAwIBAgIOAU+PT8RBAAAAAHxJXEcwDQYJKoZIhvcNAQELBQAwgZAxKDAmBgNVBAMMH1NlbGZTaWduZWRDZXJ0XzAyU2VwMjAxNV8xODI2NTMxGDAWBgNVBAsMDzAwRDI0MDAwMDAwcEFvQTEXMBUGA1UECgwOU2FsZXNmb3JjZS5jb20xFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xCzAJBgNVBAgMAkNBMQwwCgYDVQQGEwNVU0EwHhcNMTUwOTAyMTgyNjUzWhcNMTcwOTAyMTIwMDAwWjCBkDEoMCYGA1UEAwwfU2VsZlNpZ25lZENlcnRfMDJTZXAyMDE1XzE4MjY1MzEYMBYGA1UECwwPMDBEMjQwMDAwMDBwQW9BMRcwFQYDVQQKDA5TYWxlc2ZvcmNlLmNvbTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzELMAkGA1UECAwCQ0ExDDAKBgNVBAYTA1VTQTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAJp/wTRr9n1IWJpkRTjNpep47OKJrD2E6rGbJ18TG2RxtIz+zCn2JwH2aP3TULh0r0hhcg/pecv51RRcG7O19DBBaTQ5+KuoICQyKZy07/yDXSiZontTwkEYs06ssTwTHUcRXbcwTKv16L7omt0MjIhTTGfvtLOYiPwyvKvzAHg4eNuAcli0duVM78UIBORtdmy9C9ZcMh8yRJo5aPBq85wsE3JXU58ytyZzCHTBLH+2xFQrjYnUSEW+FOEEpI7o33MVdFBvWWg1R17HkWzcve4C30lqOHqvxBzyESZ/N1mMlmSt8gPFyB+mUXY99StJDJpnytbY8DwSzMQUo/sOVB0CAwEAAaOCAQAwgf0wHQYDVR0OBBYEFByu1EQqRQS0bYQBKS9K5qwKi+6IMA8GA1UdEwEB/wQFMAMBAf8wgcoGA1UdIwSBwjCBv4AUHK7URCpFBLRthAEpL0rmrAqL7oihgZakgZMwgZAxKDAmBgNVBAMMH1NlbGZTaWduZWRDZXJ0XzAyU2VwMjAxNV8xODI2NTMxGDAWBgNVBAsMDzAwRDI0MDAwMDAwcEFvQTEXMBUGA1UECgwOU2FsZXNmb3JjZS5jb20xFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xCzAJBgNVBAgMAkNBMQwwCgYDVQQGEwNVU0GCDgFPj0/EQQAAAAB8SVxHMA0GCSqGSIb3DQEBCwUAA4IBAQA9O5o1tC71qJnkq+ABPo4A1aFKZVT/07GcBX4/wetcbYySL4Q2nR9pMgfPYYS1j+P2E3viPsQwPIWDUBwFkNsjjX5DSGEkLAioVGKRwJshRSCSynMcsVZbQkfBUiZXqhM0wzvoa/ALvGD+aSSb1m+x7lEpDYNwQKWaUW2VYcHWv9wjujMyy7dlj8E/jqM71mw7ThNl6k4+3RQ802dMa14txm8pkF0vZgfpV3tkqhBqtjBAicVCaveqr3r3iGqjvyilBgdY+0NR8szqzm7CD/Bkb22+/IgM/mXQuL9KHD/WADlSGmYKmG3SSahmcZxznYCnzcRNN9LVuXlz5cbljmBj</ds:X509Certificate>
            </ds:X509Data>
         </ds:KeyInfo>
      </md:KeyDescriptor>
      <md:NameIDFormat>urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified</md:NameIDFormat>
      <md:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="https://terraform-dev-ed.my.salesforce.com/idp/endpoint/HttpPost"/>
      <md:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://terraform-dev-ed.my.salesforce.com/idp/endpoint/HttpRedirect"/>
   </md:IDPSSODescriptor>
</md:EntityDescriptor>

Are you able to run this test, @jgeurts, and potentially fix the issue? We'll need to double check other tests that use that file as well after updates or potentially create it dynamically with templatefile().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an account that I can run the acceptance tests on at the moment. I might be able to set one up, but that would take a few days to get working. In the mean time, I updated the saml metadata X509 cert to expire in 50 years. I hope that solves things for the time being!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for that update -- seems to work well for all the existing tests and the new one 👍

bflad
bflad approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you so much @jgeurts 🚀

Output from acceptance testing:

--- PASS: TestAccAWSCognitoIdentityPool_samlProviderArns (41.61s)

--- PASS: TestAccAwsEc2ClientVpn_serial (5.60s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_basic (18.01s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/AuthorizationRule_groups (81.88s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Route_disappears (546.04s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/NetworkAssociation_securityGroups (555.75s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Route_description (544.37s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/AuthorizationRule_disappears (43.63s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Route_basic (573.48s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/AuthorizationRule_basic (37.60s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_splitTunnel (28.37s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_withLogGroup (27.13s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_federated (16.50s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/AuthorizationRule_Subnets (102.24s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_tags (39.76s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/NetworkAssociation_disappears (526.89s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/NetworkAssociation_basic (548.73s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_withDNSServers (26.48s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_disappears (14.29s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_mutualAuthAndMsAD (1767.10s)
    --- PASS: TestAccAwsEc2ClientVpn_serial/Endpoint_msAD (1716.55s)

--- PASS: TestAccAWSIAMSamlProvider_basic (41.36s)
--- PASS: TestAccAWSIAMSamlProvider_disappears (15.38s)

--- PASS: TestAccAWSWorkLinkFleet_IdentityProvider (83.09s)

@bflad bflad merged commit 387d016 into hashicorp:master Sep 1, 2020
bflad added a commit that referenced this pull request Sep 1, 2020
@bflad
Update CHANGELOG for #14171
ef22307
@ghost
Copy link

ghost commented Sep 3, 2020

This has been released in version 3.5.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 Oct 1, 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 as resolved and limited conversation to collaborators Oct 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DrFaust92 DrFaust92 DrFaust92 requested changes

@bflad bflad bflad approved these changes

Assignees

@bflad bflad

Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

Client VPN Endpoint - Add Federated Authentication
4 participants
@jgeurts @divyaraghavann @bflad @DrFaust92