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

Service whitelist #87

Closed
wants to merge 2 commits into from
Closed

Service whitelist #87

wants to merge 2 commits into from

Conversation

dyson
Copy link

@dyson dyson commented Mar 5, 2012

Added a service whitelist. This was originally added against my already patched (#76) rubycas-server which you can see here (https://github.com/dyson/rubycas-server/tree/awesome) and I pulled out the diff and applied to the current rubycas-master.

If my initial patch makes it in, this patch will conflict. You can view my awesome branch listed above to see how they should both exist together.

I haven't tested this patch as I don't have a server running rubycas-server without my initial patch applied. It is simple enough however.

@dyson
Copy link
Author

dyson commented Mar 5, 2012

Also this fails invisibly to the user if a non allowed service tries to validate against the rubycas-server. It is logged as a warn that it has happened however. Don't know if this is a massive issue relating to user experience or not.

@zuk
Copy link
Member

zuk commented Apr 19, 2012

Thanks! Can we get someone to QA this?

@Slotos
Copy link

Slotos commented Jul 9, 2012

From the looks of it this code provides "false security". It is susceptible to DNS spoofing by design. To remove this vulnerability it should implement service authentication, the easiest one being pre-generated key pairs.

Take this scenario:

  • user visits service impostor using spoofed IP
  • service impostor redirects user to CAS for authentication
  • CAS provides authentication and redirects user back to service impostor
  • service impostor receives service ticket and validates it, receiving all private data that this mechanism is supposed to protect

@dyson
Copy link
Author

dyson commented Jul 12, 2012

This is very true. It is purely a whitelist to be a step up from the current offering of no list.

I have rolled my own SSO solution now so have no interest in patching this further. Shared keys would be extremely simple to add though. I was originally using it in a intranet solution that has since changed requirements somewhat and DNS spoofing wasn't a concern.

@zuk
Copy link
Member

zuk commented Jul 13, 2012

Thanks for picking that up Slotos. I'm gonna hold off on merging this until we can address this. I wonder if just enforcing valid SSL for all services would solve the problem (at least prevent DNS spoofing).

@Slotos
Copy link

Slotos commented Jul 13, 2012

Depends on what you mean by that.

SSL 3.0 allows for client authentication, it should suffice for service authentication.
Relying on client to verify SSL certificate doesn't help however. Even though it will decrease the chance of successful attempt somewhat.

@adamcrown
Copy link
Contributor

I think we really need something like this. Not having this feature at all is a much bigger security issue than having it with potential DNS spoofing vulnerabilities.

I think that having URLs without any wildcards is just too restrictive. We do our client CAS authentication at a Rack level so the service URL could be any URL within the app.

I think a good solution to both these problems would be to have an IP range whitelist. That way DNS is taken out of the equation and you're granting access to a server rather than to one specific URL.

@adamcrown
Copy link
Contributor

Since this has been sitting here unmerged for quite a while and there are some concerns about the implementation I've created a new pull request. It uses IP addresses instead of URLs to get around DNS spoofing concerns and to easily allow multiple services.

@tpickett66
Copy link
Contributor

Closing as a duplicate to 148

@tpickett66 tpickett66 closed this Dec 21, 2012
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.

5 participants