Add SAML flow support ADFS endpoint "/adfs/services/trust/13/usernamemixed" #504

Didainius · 2020-05-08T12:21:08Z

This PR adds support for SAML login using Microsoft Active Directory Services as Identity Provider (IdP).

It introduces two new provider section variables use_saml_adfs and saml_rpt_id to alter login flow.

Testing

PR adds a few new variables to configuration struct:

SamlUser        string `json:"samlUser,omitempty"`
SamlPassword    string `json:"samlPassword,omitempty"`
SamlCustomRptId string `json:"samlCustomRptId,omitempty"`

It allows to triggers explicitly a test TestAccVcdSamlAuth designed to try SAML authentication and can be useful if local users are used for main testing.

More details about flow and how it works is in go-vcloud-director SDK PR (vmware/go-vcloud-director#304) as this is where it all is handled. The main point of SDK PR is to make it as easy as possible to use new features.

This PR demonstrates it by showing how little SDK user needs to do.

vbauzys

I couldn't test as don't have ADF server

vbauzys · 2020-05-14T08:30:16Z

CHANGELOG.md

@@ -6,6 +6,8 @@ IMPROVEMENTS:
 * Removed code that handled specific cases for API 29.0 and 30.0. This library now supports VCD versions from 9.5 to 10.1 included [GH-499]
 * Added command line flags to test suite, corresponding to environment variables listed in TESTING.md [GH-505]
 * `resource/vcd_vapp_vm` allows creating VM from multi VM vApp template [GH-501]
+* Add support for SAML auth with Active Directory Federation Services (ADFS) as IdP using
+  "/adfs/services/trust/13/usernamemixed" endpoint. [GH-502]


[GH-502] -> [GH-504]

dataclouder

First pass. More after a new round of tests

dataclouder · 2020-05-19T07:00:40Z

vcd/auth_saml_test.go

+	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
+)
+
+// TestAccVcdSamlAuth explicitly tests a simple operation using SAML auth when explicit SAML


Suggested change

// TestAccVcdSamlAuth explicitly tests a simple operation using SAML auth when explicit SAML

// TestAccVcdSamlAuth tests a simple operation using SAML auth when explicit SAML

vcd/auth_saml_test.go

dataclouder · 2020-05-19T07:09:00Z

vcd/config_test.go

+		UseSamlAdfs     bool   `json:"useSamlAdfs"`
+		CustomAdfsRptId string `json:"customAdfsRptId,omitempty"`
+
+		// The below `SamlUser`, `SamlPassword` and `SamlCustomRptId` variables are optional and are


Suggested change

// The below `SamlUser`, `SamlPassword` and `SamlCustomRptId` variables are optional and are

// The variables `SamlUser`, `SamlPassword` and `SamlCustomRptId` are optional and are

dataclouder · 2020-05-19T07:10:47Z

vcd/sample_vcd_test_config.json

@@ -6,6 +6,14 @@
    "user": "root",
    "password": "somePassword",
    "token": "Access token to be used instead of username/password",
+
+    "//": "Below 3 fields allow to set SAML credentials for tests that specifically use it.",


Suggested change

"//": "Below 3 fields allow to set SAML credentials for tests that specifically use it.",

"//": "The 3 fields below allow to set SAML credentials for tests that specifically use it.",

dataclouder · 2020-05-19T07:11:15Z

vcd/sample_vcd_test_config.json

+
+    "//": "Below 3 fields allow to set SAML credentials for tests that specifically use it.",
+    "//": "May be useful when local user credentials are used by default.",
+    "//": "The below credentials will authenticate to Org specified in vcd.org parameter.",


Suggested change

"//": "The below credentials will authenticate to Org specified in vcd.org parameter.",

"//": "These credentials will authenticate to the Org specified in vcd.org parameter.",

lvirbalas

Neat PR. Have one user-facing field naming/typing to resolve.

vcd/config_test.go

lvirbalas · 2020-05-19T09:07:48Z

website/docs/index.html.markdown

  org                  = "System"
-  url                  = "${var.vcd_url}"
-  max_retry_timeout    = "${var.vcd_max_retry_timeout}"
-  allow_unverified_ssl = "${var.vcd_allow_unverified_ssl}"


Thank you for clearing this up!

lvirbalas · 2020-05-19T10:04:43Z

website/docs/index.html.markdown

@@ -186,7 +209,17 @@ The following arguments are used to configure the VMware vCloud Director Provide
   instead of username and password. When this is set, username and password will
    be ignored, but should be left in configuration either empty or with any custom values.
    A token can be specified with the `VCD_TOKEN` environment variable.
-
+
+* `use_saml_adfs` - (Optional) Set `true` to use SAML login flow with Active Directory Federation


In discussions about this field. Options so far:
saml_adfs_enabled
saml_adfs_auth_enabled
auth_type = "saml_adfs" | "native" (default) | "token" | "ldap"

CC: @dataclouder @vbauzysvmware

my vote auth_type if possible

lvirbalas

Last comments below, LGTM otherwise!

Please add a line about official support for LDAP auth type to CHANGELOG.md
Would like to see tests passing for @vbauzysvmware on his ADFS before merging
Travis is failing

dataclouder

LGTM

vbauzys · 2020-05-22T10:34:45Z

2. Would like to see tests passing for @vbauzysvmware on his ADFS before merging

Replicated ADFS and tested on my env.

…mixed" (#504) * Add SAML support with ADFS

Add SAML support with ADFS

a1d3140

Didainius self-assigned this May 8, 2020

ghost added size/XL dependencies documentation labels May 8, 2020

Didainius added 3 commits May 11, 2020 11:54

Pull later govcd version

f6ab56d

[skip ci] Cleanup docs

0f7e51c

Add TestAccVcdSamlAuth for explicit SAML testing

c35ee95

ghost added size/XXL and removed size/XL labels May 13, 2020

Didainius marked this pull request as ready for review May 13, 2020 14:08

Didainius requested review from dataclouder, vbauzys and lvirbalas May 13, 2020 14:08

Didainius added 5 commits May 13, 2020 17:23

Pull in newer govcd

8c4efe0

Merge master

01cfe96

[skip ci] fix indent

7306fdb

[skip ci] - fix invalid comment

d1a11c4

Fix 'go test -tags auth -v'

269fe43

vbauzys approved these changes May 14, 2020

View reviewed changes

Didainius added 2 commits May 15, 2020 08:00

Bump govcd

74e6484

merge master

5fc49bb

ghost added size/XL and removed size/XXL labels May 18, 2020

dataclouder suggested changes May 19, 2020

View reviewed changes

lvirbalas requested changes May 19, 2020

View reviewed changes

Didainius added 3 commits May 19, 2020 14:11

[skip ci] address text based comments

9cb03d1

Fix TestAccVcdSamlAuth when VCD_TOKEN is set

228b6a9

Add auth_type and tests for all types of auth

57c771a

ghost removed the size/XL label May 20, 2020

ghost added the size/XXL label May 20, 2020

Didainius added 5 commits May 20, 2020 14:00

S[skip ci] huffle field possitions in docs

aa363aa

[skip ci] Rename incorrect env variable

9f4aa3f

[skip ci] Remove comments

aff9743

Add auth_type=integrated support

912c695

Changelog note for LDAP, fix test-artifact generation

3003a10

lvirbalas approved these changes May 21, 2020

View reviewed changes

Didainius added 4 commits May 21, 2020 11:17

bump govcd

aebf825

Switch to dynamic sysorg in TestAccAuth

fee2fd1

Remove tag

1578589

Remove leftover and fix comment

52c9045

dataclouder approved these changes May 21, 2020

View reviewed changes

Didainius added 3 commits May 22, 2020 09:23

Bump govcd

f520eb8

Fix 'getTestVCDFromJson' initialization

f3561ff

Add one more auth config test

f4288ed

Didainius added 2 commits May 22, 2020 14:05

Prioritize env variable VCD_TOKEN over config in testing

bfcf67e

Include new govcd version

b11da61

Didainius merged commit 8292bdb into vmware:master May 25, 2020

Didainius deleted the saml3 branch May 25, 2020 10:46

vbauzys referenced this pull request in vbauzys/terraform-provider-vcd_archive3 Jul 1, 2020

Add SAML flow support ADFS endpoint "/adfs/services/trust/13/username…

e278db7

…mixed" (#504) * Add SAML support with ADFS

vbauzys referenced this pull request in vbauzys/terraform-provider-vcd_archive3 Jul 1, 2020

Add SAML flow support ADFS endpoint "/adfs/services/trust/13/username…

28e1d13

…mixed" (#504) * Add SAML support with ADFS

vbauzys referenced this pull request in vbauzys/terraform-provider-vcd_archive3 Jul 1, 2020

Add SAML flow support ADFS endpoint "/adfs/services/trust/13/username…

16a0968

…mixed" (#504) * Add SAML support with ADFS

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SAML flow support ADFS endpoint "/adfs/services/trust/13/usernamemixed" #504

Add SAML flow support ADFS endpoint "/adfs/services/trust/13/usernamemixed" #504

Didainius commented May 8, 2020 •

edited

Loading

vbauzys left a comment

vbauzys May 14, 2020

Didainius May 19, 2020

dataclouder left a comment

dataclouder May 19, 2020

Didainius May 19, 2020

dataclouder May 19, 2020

Didainius May 19, 2020

dataclouder May 19, 2020

Didainius May 19, 2020

dataclouder May 19, 2020

Didainius May 19, 2020

lvirbalas left a comment

lvirbalas May 19, 2020

lvirbalas May 19, 2020

vbauzys May 19, 2020

lvirbalas left a comment

dataclouder left a comment

vbauzys commented May 22, 2020

	// TestAccVcdSamlAuth explicitly tests a simple operation using SAML auth when explicit SAML
	// TestAccVcdSamlAuth tests a simple operation using SAML auth when explicit SAML

	// The below `SamlUser`, `SamlPassword` and `SamlCustomRptId` variables are optional and are
	// The variables `SamlUser`, `SamlPassword` and `SamlCustomRptId` are optional and are

	"//": "Below 3 fields allow to set SAML credentials for tests that specifically use it.",
	"//": "The 3 fields below allow to set SAML credentials for tests that specifically use it.",

	"//": "The below credentials will authenticate to Org specified in vcd.org parameter.",
	"//": "These credentials will authenticate to the Org specified in vcd.org parameter.",

Add SAML flow support ADFS endpoint "/adfs/services/trust/13/usernamemixed" #504

Add SAML flow support ADFS endpoint "/adfs/services/trust/13/usernamemixed" #504

Conversation

Didainius commented May 8, 2020 • edited Loading

Testing

vbauzys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dataclouder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

dataclouder left a comment

Choose a reason for hiding this comment

vbauzys commented May 22, 2020

Didainius commented May 8, 2020 •

edited

Loading