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

Remote idp metadata #136

Merged
merged 8 commits into from
Oct 30, 2014
Merged

Remote idp metadata #136

merged 8 commits into from
Oct 30, 2014

Conversation

bonyiii
Copy link
Contributor

@bonyiii bonyiii commented Jun 11, 2014

Download and parse IdP metadata.
Based on https://github.com/onelogin/ruby-saml/pull/21/files

@Lordnibbler
Copy link
Contributor

@pitbulk @inakidelamadrid @luisvm can you review? lgtm 👍 thanks @bonyiii for documentation

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 9, 2014

👍 except my comment

@bonyiii
Copy link
Contributor Author

bonyiii commented Oct 10, 2014

I updated the pr accordingly.

@Lordnibbler
Copy link
Contributor

@luisvm @pwnetrationguru can you review please? lgtm 👍

@luisvm
Copy link
Contributor

luisvm commented Oct 16, 2014

👍 looks good!

@pwnetrationguru
Copy link
Contributor

Not validating a cert feels risky to me, but it defaults to validate and doesn't call parse_remote anywhere except a test so a user would have to open themselves up by calling parse_remote('xxx', false) so I think its ok.

Is metadata exchange part of the SAML 2.0 spec? If not in the SAML 2.0 spec, where is this being widely used and discussed? Maybe @pitbulk can speak to that.

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 16, 2014

When entities exchange metadatas, the issuer can sign it so the receptor can validates that the content is not modified, but the receptor may know previously the public cert in order to validate the signature, or use a cert that is related to a specific domain.

If not a MITM can take the metadata, sign it with a valid cert and hack the system.

In SAML world, entities exchanges the metadatas manually in a first approach, later they uses metadata-registration components that will handle all the issues related to the validation of the metadata.

Said this, Im ok with this PR

@pwnetrationguru
Copy link
Contributor

@pitbulk, can we discuss this in our next meeting. @Lordnibbler and I have some questions to clear up.

pitbulk
pitbulk reviewed Oct 28, 2014
View reviewed changes
README.md
@@ -53,8 +53,6 @@ def saml_settings
end
```

What's left at this point, is to wrap it all up in a controller and point the initialization and consumption URLs in OneLogin at that. A full controller example could look like this:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove it?

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 28, 2014

I'm ok with that except remove the part of the doc that I commented.

@bonyiii
Copy link
Contributor Author

bonyiii commented Oct 28, 2014

I restored those line in the readme.

@Lordnibbler
Copy link
Contributor

@pitbulk @pwnetrationguru did you decide you were confident to merge this?

@pwnetrationguru
Copy link
Contributor

👍

Lordnibbler added a commit that referenced this pull request Oct 30, 2014
@Lordnibbler
Merge pull request #136 from bonyiii/remote_idp_metadata
8fa4c1d 
Remote idp metadata
@Lordnibbler Lordnibbler merged commit 8fa4c1d into SAML-Toolkits:master Oct 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bonyiii @Lordnibbler @pitbulk @luisvm @pwnetrationguru