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.

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

Add SAML support with ADFS as IdP #304

Merged
merged 23 commits into from
May 25, 2020
Merged

Add SAML support with ADFS as IdP #304

merged 23 commits into from
May 25, 2020

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented May 8, 2020

This PR adds support to authenticate to Cloud Director using SAML authentication (by applying WithSamlAdfs() configuration option to NewVCDClient function). The identity provider (IdP) must be Active Directory Federation Services (ADFS) and "/adfs/services/trust/13/usernamemixed" endpoint must be enabled to make it work. Furthermore username must be supplied in ADFS friendly format - test@contoso.com' or 'contoso.com\test'.

Inner workings

In short - it works by finding ADFS login endpoint for vCD by querying vCD SAML redirect endpoint for specific Org and then submits authentication request to "/adfs/services/trust/13/usernamemixed" endpoint of ADFS server. Using ADFS response it constructs a SIGN token which vCD accepts for the "/api/sessions". After first initial "login" it grabs the regular X-Vcloud-Authorization token and uses it for further requests.

Testing

  • There is a unit test TestSamlAdfsAuthenticate which spawns mock servers and does not require ADFS or SAML being configured. It tests the flow based on mock endpoints.
  • Test_SamlAdfsAuth on the other hand can test and compare VDC retrieved using main authentication credentials vs the one retrieved using specific SAML credentials. This requires variables samlUser, samlPassword (and optionally samlCustomRptId if Relaying Party Trust ID must be overriden).

Note. SAML credentials can be used for main testing user (defined by user, password variables together with useSamlAdfs and optionally customAdfsRptId)as well. That would cause whole suite to work with SAML credentials.

Migration to X-Vmware-Vcloud-Access-Token:TOKEN

At the moment this codebase relies on x-vcloud-authorization token, however since API version 30 there is a new way to authenticate using bearer token (https://kb.vmware.com/s/article/56948). This PR does not change authentication mode as it is not directly related, but because "/api/sessions" endpoint returns both types of tokens now, SAML auth will have no additional trouble when we migrate to new token format

Examples

There is a working code example in /samples/saml_auth_adfs directory how to setup client using SAML auth.

@Didainius Didainius self-assigned this May 8, 2020
@Didainius Didainius added the enhancement New feature or request label May 8, 2020
@Didainius Didainius marked this pull request as ready for review May 11, 2020 08:44
Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

Mock test run success. Haven't anticipated to be this fast.

govcd/api.go Outdated Show resolved Hide resolved
govcd/api_vcd.go Show resolved Hide resolved
govcd/saml_auth.go Outdated Show resolved Hide resolved
govcd/saml_auth.go Outdated Show resolved Hide resolved
govcd/saml_auth.go Show resolved Hide resolved
govcd/saml_auth.go Show resolved Hide resolved
Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Thank you for an awesome PR! Have three asks in-line and one here.

Please add a section to TESTING.md describing the nuances of the SAML auth testing. You have most of that write-up already in some of form in different places, but would like to provide a perspective for the casual user running the tests.

Thanks!

README.md Outdated Show resolved Hide resolved
govcd/saml_auth.go Show resolved Hide resolved
govcd/saml_auth_test.go Show resolved Hide resolved
Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

Here are a few suggestions to improve readability.
The tests are passing

Makefile Outdated Show resolved Hide resolved
govcd/api_vcd_test.go Outdated Show resolved Hide resolved
govcd/api_vcd_test.go Outdated Show resolved Hide resolved
govcd/saml_auth.go Show resolved Hide resolved
}

// getSamlAuthToken generates a token request payload using function
// getSamlTokenRequestBody. This request is submited to ADFS server endpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// getSamlTokenRequestBody. This request is submited to ADFS server endpoint
// getSamlTokenRequestBody. This request is submitted to ADFS server endpoint

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

return
}

resp := []byte(`<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this long response could be stored in a file under ./test-resources instead of being merged as literal test here. The same suggestion is valid for other large strings of XML data in this file.
Moving the literal texts to external file would make the test more readable.

The comments in other files referring to saml_auth_unit_test.go should instead refer to the external file.

govcd/sample_govcd_test_config.yaml Outdated Show resolved Hide resolved
govcd/sample_govcd_test_config.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Thank you for all the additional write-ups, they are helpful!

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

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

LGTM

@vbauzys
Copy link
Contributor

vbauzys commented May 22, 2020

Replicated adfs and tested on my env.

@Didainius Didainius merged commit 819fed6 into vmware:master May 25, 2020
@Didainius Didainius deleted the saml3 branch May 25, 2020 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants