third_party_auth SAML/Shibboleth Provider Support by bradenmacdonald · Pull Request #8018 · openedx/openedx-platform

bradenmacdonald · 2015-05-13T01:36:19Z

Description
This is the first part of implementing the new Shibboleth SSO support integrated into third_party_auth (TPA). See the technical spec for a lot more background and discussion.

This PR will merge the new code to a feature branch, feature/shibboleth-tpa

Changes in this PR
This first PR does the following:

Upgrades third_party_auth (TPA) to use the latest version of python-social-auth (this was a non-trivial upgrade)
Adds a new TPA provider class that can be used as a base class for writing SAML providers
Includes modified versions of python-saml and python-social-auth that do the heavy lifting of implementing SAML support. Changes are being contributed to those upstream projects.
Includes two example providers, both using TestShib, the public Shibboleth test server.
Modifies the TPA code so that multiple providers can use the same python-social-auth "backend" class.

New Requirement
This introduces one new requirement: python-saml, which also [currently] requires two new debian packages not installed on most edX instances: libxmlsec1-dev and swig.

Dependencies

https://github.com/edx/configuration/pull/2002
Fix creation of metadata with no SLS, when using settings.get_sp_metadata() SAML-Toolkits/python-saml#66 (external upstream PR)
NameIDFormat/NameIDPolicy Improvements SAML-Toolkits/python-saml#67 (external upstream PR)
Allow configuration of metadata caching/expiry via settings SAML-Toolkits/python-saml#68 (external upstream PR)
Allow metadata signing with SP key specified as config value, not file SAML-Toolkits/python-saml#69 (external upstream PR)
SAML support omab/python-social-auth#616 (external upstream PR)

Sandbox

Internal Code Review PR: https://github.com/edx/edx-platform/pull/7893

Upgrade Issue
When merged, this PR does cause a problem for existing installations: python-social-auth at one point converted from syncdb to south/django migrations, and bumping the version of python-social-auth that we use will result in this error next time paver update_db is run:

django.db.utils.DatabaseError: table "social_auth_usersocialauth" already exists

To fix this (on devstack), the user must manually run this command:

./manage.py lms migrate default 0001_initial --fake --settings=devstack

Unfortunately, I'm not aware of any way to avoid this issue other than this manual fix. New installs are not affected.

Testing Instructions:
Go to the sandbox link above and login via TestShib. Try registering then linking and unlinking TestShib accounts. If it doesn't work, it may be necessary to re-upload the metadata to TestShib's "Register" page. A link to the metadata is above.

To test locally, see https://github.com/edx/edx-platform/pull/7893

Not included:
The following will be coming in future PRs against this feature branch:

provider configuration will be moved to the database
Metadata fetching
Fix for CRI-9
Ability to specify default third_party_auth provider via query param
UX changes: Allow to configure multiple Shibboleth identity providers as "secondary" providers

openedx-webhooks · 2015-05-13T01:36:19Z

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?number=8018&repo=edx%2Fedx-platform

bradenmacdonald · 2015-05-13T07:11:06Z

@cpennington Can you please review this?

Build failure is due to Jenkins boxes lacking the new ubuntu package dependencies.

@mollydb and @e0d can you please review the new requirement I've included in this PR: python-saml + two ubuntu packages it requires (which we may be able to remove as python-saml's dependencies are likely to change in a future major release). Note I'm using a forked/patched version for now, but if the patches are accepted upstream we'll be able to use a pypi release.

CC @antoviaque

smarnach · 2015-05-13T10:08:10Z

👍 I reviewed this code already in #7893 and only had a quick look at the changes since then – looks good!

e0d · 2015-05-13T12:45:29Z

The apt requirements change LGTM, we'll still need to merge the configuration change, but need to keep in synch for those not using ansible.

openedx-webhooks · 2015-05-14T15:35:50Z

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=8018

openedx-webhooks · 2015-05-14T18:26:15Z

Thanks for the pull request, @bradenmacdonald! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: http://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=8018

antoviaque · 2015-05-20T09:00:35Z

@cpennington Do you have an idea when you will be able to review this? Ideally it would be good if @bradenmacdonald could work on the review comments this week, but let me know what is possible - there is a lot of code and depenencies on this project, so it's hard to keep moving forward with the rest of the requirements while this is still in review.

cpennington · 2015-05-20T12:22:57Z

@antoviaque @bradenmacdonald: I've been reviewing it, but haven't yet had any comments. I was just taking a look at the python-social-auth code (I've already reviewed the python-saml PRs).

cpennington · 2015-05-20T18:33:25Z

if cls.BACKEND_CLASS.name != pipeline['backend'], should this return False, or None (as is implicitly being done now?)

Good catch, it should return False for consistency.

cpennington · 2015-05-20T18:37:11Z

This has a lot of changes to the effect of s/strategy/request.backend/. Are those necessary changes, or some incidental refactoring?

bradenmacdonald · 2015-05-20T18:39:42Z

Those are all broken out separately as the first commit 925904e - they're required API changes for compatibility with the updated version of python-social-auth (0.2.7)

I tried to keep the commits fairly clear and separate to better document the changes I made.

cpennington · 2015-05-20T18:41:40Z

Ah, excellent. I hadn't looked at each commit individually.

Done w/ my review pass. A few very minor problems, but 👍 once those are resolved.

bradenmacdonald · 2015-05-20T19:16:22Z

Thanks @cpennington! Should I merge https://github.com/edx/configuration/pull/2002 now, or keep it pending until we eventually merge this whole feature branch? It would be nice to have working Jenkins tests on this feature branch in the meantime (I'm assuming that configuration PR is what will add the prereqs Jenkins needs)

cpennington · 2015-05-20T19:23:20Z

Yeah, I think merging now makes sense.

bradenmacdonald · 2015-05-20T20:06:03Z

@cpennington I've addressed your minor comments in bbb0ccb. I'm going to rebase/squash now and then once I check that the tests are passing (at least locally) post-rebase, I'll merge this to the feature branch.

I'll also open a new PR for tracking the feature branch status, all the component PRs, and any notes that will be needed when we merge the feature branch.

bradenmacdonald · 2015-05-21T05:36:08Z

Ran all tests locally and they're all passing.

third_party_auth SAML/Shibboleth Provider Support

ztraboo · 2015-07-10T19:47:14Z

@bradenmacdonald, thanks for pushing this third-party auth Shibboleth update out so that everyone can use it. I used the old version of Shibboleth SP and Apache with @johncox-google help here.

Clemson University has been using this thirdy-party auth with Shibboleth since beginning of last Fall 2014 but I never pushed my changes to everyone. I'm really interested in the 2. Support many Shibboleth institutional login options that you mentioned in your technical spec

bradenmacdonald · 2015-07-10T19:52:50Z

@ztraboo Interesting, thanks for sharing. The new implementation does not require Shibboleth SP or Apache, so it lets you have a simpler stack.

The "Support many Shibboleth institutional login options" feature is already in master and will be in the Cypress release. It was implemented in #8603.

Here's a screenshot:

Which leads to:

ztraboo · 2015-07-10T20:11:01Z

@bradenmacdonald, thanks for sharing. That's great news. Where are the configuration settings for the OAuth2 (Google, LinkedIn) providers set as previously noted in the edx third_party_auth page?

/edx/app/edxapp/lms.auth.json

"THIRD_PARTY_AUTH": {
    "Google": {
        "SOCIAL_AUTH_GOOGLE_OAUTH2_KEY": "google_key",
        "SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET": "google_secret"
    },
    "LinkedIn": {
        "SOCIAL_AUTH_LINKEDIN_OAUTH2_KEY": "linkedin_key",
        "SOCIAL_AUTH_LINKEDIN_OAUTH2_SECRET": "linkedin_secret"
    }
}

Any idea why I would be reinitializing the social_auth_usersocialauth database table? I think it has something to do with social.backends. Did you run into a migration issue like this?

bradenmacdonald · 2015-07-10T20:15:23Z

@ztraboo The configuration is now stored in the database and is edited via the Django admin. Your OAuth2 settings will be migrated automatically.

For details on that and the migration issue you've run into, see:
https://groups.google.com/forum/#!topic/edx-code/H1e03OPuH4g and
https://groups.google.com/forum/#!topic/edx-code/FPfScH93R6M

ztraboo · 2015-07-10T20:17:40Z

@bradenmacdonald Thank you for the support.

You mentioned that "The new implementation does not require Shibboleth SP or Apache..." so we don't need to setup Shibboleth SP anymore? You mentioned in your technical document that "IdP and SP must register with each other by exchanging XML metadata, which includes the public key of each service/provider". I remember us having to do that step. I read up to the Implementation Plan in that document so let me know if you say anything there about setup. I used these edx Shibboleth setup instructions to get it working previously. Can you update these notes?

bradenmacdonald · 2015-07-10T20:50:26Z

@ztraboo That's correct. You don't need to run the "Shibboleth SP" software, since edx-platform itself can now act as the SP.

The Docs team is working on new documentation for Shibboleth, and it should be included with the Cypress release. Once it's ready we should update that wiki page. In the meantime, OPS-820 has reasonably complete instructions and is the best I can offer.

ztraboo · 2015-07-10T21:04:30Z

@bradenmacdonald Here are some configuration settings that we did to have ansible install apache and shibboleth. I'm guessing that we still need to keep these around for the course Shibboleth restriction in the Studio > Advanced Setting > enrollment_domain setting (e.g. shib:<your_idp>). It looks like your technical document mentioned 4. Remove the legacy Shibboleth implementation. Has this been implemented yet?

Configuration Changes
playbooks/edx-east/edxapp.yml

playbooks/roles/apache/templates/lms.j2
playbooks/roles/nginx/templates/edx/app/nginx/sites-available/lms.j2

~~Do I need to submit to Jira administrators if I cannot access OPS-820?~~ I submitted a request.

bradenmacdonald · 2015-07-10T21:16:09Z

@ztraboo Please email me so we can continue this conversation and I can send you those instructions. No need to request access to JIRA.

For the record:

no, we have not yet remove the legacy shibboleth implementation.
Auto-enrollment and linking users to Shibboleth login is now achieved through different mechanisms than that document desccribes.

antoviaque · 2015-07-13T07:04:13Z

@ztraboo @bradenmacdonald If there is a conversation/support on this, it would be worth having it publicly on the openedx-ops mailing list, so it can be useful to others in the community.

bradenmacdonald · 2015-07-13T19:58:09Z

For those interested: All of the new SSO features will be thoroughly documented in the documentation update that accompanies the upcoming Cypress release. In the meantime, I have posted the information required to test out the new SSO/Shibboleth functionality at https://openedx.atlassian.net/wiki/display/OpenOPS/New+Shibboleth+Configuration

bradenmacdonald force-pushed the shibboleth-1 branch from d62855a to 925904e Compare May 13, 2015 03:09

cpennington reviewed May 20, 2015
View reviewed changes

bradenmacdonald added 4 commits May 20, 2015 13:14

Changes for compatibility with latest python-social-auth (0.2.7)

3ff710c

Remove some redundant code

3fe4dea

Framework toward implementation of SAML2 TPA Provider

7a8ed07

Changes required for multiple TPA providers to share one PSA backend

9f8365c

bradenmacdonald force-pushed the shibboleth-1 branch from bbb0ccb to 3ff710c Compare May 20, 2015 20:18

bradenmacdonald added a commit that referenced this pull request May 21, 2015

Merge pull request #8018 from open-craft/shibboleth-1

77ba27d

third_party_auth SAML/Shibboleth Provider Support

bradenmacdonald merged commit 77ba27d into openedx:feature/shibboleth-tpa May 21, 2015

bradenmacdonald mentioned this pull request May 21, 2015

Feature: Shibboleth/SAML SSO #8140

Merged

14 tasks

bradenmacdonald deleted the shibboleth-1 branch June 26, 2015 23:22

Uh oh!

Conversation

bradenmacdonald commented May 13, 2015

Uh oh!

openedx-webhooks commented May 13, 2015

Uh oh!

bradenmacdonald commented May 13, 2015

Uh oh!

smarnach commented May 13, 2015

Uh oh!

e0d commented May 13, 2015

Uh oh!

openedx-webhooks commented May 14, 2015

Uh oh!

openedx-webhooks commented May 14, 2015

Uh oh!

antoviaque commented May 20, 2015

Uh oh!

cpennington commented May 20, 2015

Uh oh!

cpennington May 20, 2015

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald May 20, 2015

Choose a reason for hiding this comment

Uh oh!

cpennington commented May 20, 2015

Uh oh!

bradenmacdonald commented May 20, 2015

Uh oh!

cpennington commented May 20, 2015

Uh oh!

bradenmacdonald commented May 20, 2015

Uh oh!

cpennington commented May 20, 2015

Uh oh!

bradenmacdonald commented May 20, 2015

Uh oh!

bradenmacdonald commented May 21, 2015

Uh oh!

ztraboo commented Jul 10, 2015

Uh oh!

bradenmacdonald commented Jul 10, 2015

Uh oh!

ztraboo commented Jul 10, 2015

Uh oh!

bradenmacdonald commented Jul 10, 2015

Uh oh!

ztraboo commented Jul 10, 2015

Uh oh!

bradenmacdonald commented Jul 10, 2015

Uh oh!

ztraboo commented Jul 10, 2015

Uh oh!

bradenmacdonald commented Jul 10, 2015

Uh oh!

antoviaque commented Jul 13, 2015

Uh oh!

bradenmacdonald commented Jul 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants