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 support for SAML 1 Tokens #12

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

kcdragon
Copy link
Contributor

@kcdragon kcdragon commented Jul 8, 2014

This pull requests add support for SAML 1 Tokens. There are a couple things to note:

  • I based the saml1_example.xml on the service I am communicating with, but replaced most of the values with the corresponding values in acs_example.xml
  • Your comments in auth_callback.rb imply that you did not expect "audience" to vary between SAML 1 and SAML 2. I needed these to differ because SAML 1 has namespaces attached to the elements.
  • I needed to make a change to xml_security.rb because the ID in the "check digests" section is named "AssertionID" instead of "ID".

Let me know if you have any feedback about how this could be improved. Thanks.

@kbeckman
Copy link
Owner

kbeckman commented Jul 8, 2014

Mike, thanks for the PR! This was a big chunk of work... I should be able to get to this a bit later this week. Stay tuned... ...and thanks again!

@kbeckman
Copy link
Owner

Hey, Mike. Sorry, It took me a bit longer to get started with this than I had hoped... I was on the road this most of this last week. I did take a pretty long look at this last night though. The refactoring for the SAML 2.0 tokens seemed to work just fine, but the SAML 1.1 tokens did not. Your last bullet item (check for AssertionID or ID) seemed to be the problem. I used Azure ACS to issue SAML 1.1 tokens for the test and it used "AssertionID" rather than just "ID".

I'm curious, what token issuer did you use? Was it a commercial product or something grown internally?

@kcdragon
Copy link
Contributor Author

@kbeckman Sorry for taking so long to get back to you.

The token issuer is ADFS (Active Directory Federated Services). I looked around for a little bit and the examples on Wikipedia and Oracle have the AssertionID field. Oracle Wikipedia

Is it possible that this field varies even within SAML 1.1? I'm thinking we could either:

  1. Check for ID and then if that attribute is not present, check for Assertion ID
  2. Create a config option to specify the attribute to use for checking digests

def token
@token ||= begin
case settings[:saml_version]
when '1'
Copy link
Contributor

Choose a reason for hiding this comment

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

If you set saml_version as integer, this will not work, it would be better to case settings[:saml_version].to_s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants