getTicket is an anti-pattern

[agrasley]

The getTicket method seems like an anti-pattern to me. The whole point of using third party authentication like CAS is that your app never has to deal with sensitive user information, i.e., passwords. The getTicket method breaks that by requiring you to receive a user's username and password before passing it on to CAS. This makes the CAS authentication process fundamentally more insecure, and essentially requires all servers that use this package to implement SSL. Otherwise, your server exposes user passwords to man-in-the-middle attacks over a plain HTTP connection. While it's generally a good thing in principle to implement SSL on all of your sites, I'm unsure if it's wise for a library like byu-cas to assume that it will always be used in an SSL-enabled environment.

The getTicket method just seems fundamentally at odds with my understanding of the CAS authentication process. The way I understand it, an application should only be concerned with validating a user ticket. The only part an application should play in obtaining that ticket is redirecting a user to the proper login endpoint on the CAS server. Adding a layer in between the user and the CAS login not only brings with it a lot of security concerns as I outlined above, but it's also just unnecessarily complex. Why use your node process on something that is much better done by the client browser? You can do the exact same thing as getTicket with a simple link element to BYU's CAS server and then a quick read of the ticket query parameter when the user gets redirected back to your site. It's easier on your server resources, far more secure since you never handle a user password, and it provides a lot fewer potential points of failure if something goes wrong.

My recommendation is to deprecate the getTicket method, only keeping it for legacy projects that rely on it. The documentation should make it clear that getTicket is a security risk if you use it in a non-SSL environment. The validate method is great, since it fits the general pattern established by CAS that a third-party application should only be concerned with validating user tickets. If our servers are doing something more than that, we're probably doing something very wrong.

[webnard]

Point taken.

The reason we have the getTicket method is so we can easily, automatically, and publicly test the module without the need of third-party tools. Your security concerns are valid, so I'll put a warning in for the latest patch. I don't want to completely remove it, however, because I don't believe it's devoid of all utility (like automation).

What are your thoughts about preventing the function from loading except in cases where an INSECURE=true environment variable is present? This will allow people to experiment with automation while making the risks a little more obvious.

[agrasley]

Yeah, I agree that it's not devoid of use, since there are a lot of cases where you'd like to have automated control of the CAS login. My concern was that people would use the library without those security concerns being noted in the documentation. For testing or automation in a safe environment, it's obviously not a big deal. It's only really a big concern if you use it in a public-facing server, which is what I was trying to do when I found the problem.

I like the idea of environment variables being used to control its behavior so that you don't use it in an unsafe way. One potential solution would just be to make it fail or complain loudly in cases where NODE_ENV="production". Unless, that is, if there are legitimate production uses of the method, then it would probably be best just to tie it to some variable like INSECURE like you suggested.

[webnard]

Good thinking. I'll make the changes later this evening and publish them.

Thanks for the help.

On Tue, Apr 12, 2016 at 4:41 PM, Alexander Grasley <notifications@github.com

wrote:

Yeah, I agree that it's not devoid of use, since there are a lot of cases
where you'd like to have automated control of the CAS login. My concern was
that people would use the library without those security concerns being
noted in the documentation. For testing or automation in a safe
environment, it's obviously not a big deal. It's only really a big concern
if you use it in a public-facing server, which is what I was trying to do
when I found the problem.

I like the idea of environment variables being used to control its
behavior so that you don't use it in an unsafe way. One potential solution
would just be to make it fail or complain loudly in cases where
NODE_ENV="production". Unless, that is, if there are legitimate
production uses of the method, then it would probably be best just to tie
it to some variable like INSECURE like you suggested.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#2 (comment)

ian hunter

[webnard]

Patch version 1.0.3 issues a warning, and version 2.0.0 disables getTicket by default. Let me know if I've missed anything.