Scott,
What revised patch are we talking about? I haven't submitted any new
changes...
Thanks
Michael
On Mon, Mar 1, 2010 at 11:49 AM, Scott M. Holodak <sholo...@princeton.edu>wrote:
> Hi Michael,
>
>
>
> I’ve looked over your revised patch and I’m not sure this is the right
> approach. I definitely understand the utility of configuring the
> ServicePointManager to accept all SSL server certificates, but I’m not
> convinced that the CAS HttpModule is the place for it. The CAS module has a
> very specific purpose and the ServicePointManager change affects the entire
> application. In the case of an ‘accept all SSL certificate’ policy, the
> thread-safety issue can have a much bigger impact than simply unexpected
> behavior—there are serious security concerns. I understand that you intend
> for this to be a Debug-only option, but I still have serious reservations.
> Web servers are acting as clients for more and more purposes outside of
> ticket validation (mash-ups, web services, etc.).
>
>
>
> The quick setting/unsetting of the delegate feels like a bad idea. I don’t
> know of a way to safely implement a site-wide locking mechanism for these
> blocks of code that would prevent third-party code from interfering. I
> don’t believe Application.Lock/Application.Unlock were intended for this
> purpose (I think they are just to control access to the Application
> collection). Have a look at
> http://www.west-wind.com/weblog/posts/48909.aspx. Toward the end of the
> post, it says, “One thing to watch out for is that this an application
> global setting. There's one ServicePointManager and once you set this value
> any subsequent requests will inherit this policy as well, which may or may
> not be what you want. So it's probably a good idea to set the policy when
> the app starts and leave it be - otherwise you may run into odd behavior in
> some situations especially in multi-thread situations.” I think an
> HttpModule would classify as a ‘multi-threaded situation’. Also, resetting
> the delegate to null assumes that no applications have explicitly
> implemented an alternative certificate policy.
>
>
>
> I recommend we treat this as a documentation issue. We can instruct users
> on how to override the certificate validation behavior for testing purposes
> in global.asax with the caveats we’ve touched on.
>
>
>
> *Scott Holodak **Aspire **|** **A Plan for Princeton***
>
> *Princeton* *University* Office of Development
>
>
>
> *From:* Michael Hans [mailto:foliofnm...@gmail.com]
> *Sent:* Sunday, February 28, 2010 9:36 PM
>
> *To:* cas-dev@lists.jasig.org
> *Subject:* Re: [cas-dev] Quirky Dev Patch for Invalid SSL Exceptions in
> .NET
>
>
>
> Hello Scott,
>
> So I don't disagree with you but will say the following:
>
> >The threading issue makes me a little uncomfortable. Since this module is
> going to be processing
>
> >multiple requests at the same time, adding the delegate, running the
> request, and removing the
>
> >delegate can cause a race condition between threads
>
> I'm well aware of the threading issue and stated it when I sent the patch.
> The usecase, however far fetched is something i've seen but agree it's not
> common and should have just forwarded it to /dev/null
>
> >Is there a reason that you aren’t trusting the CA certificate instead or
> treating the self-signed cert as a
>
> >trusted CA cert?
>
> A lot of environments are painful to change or out of your control, I would
> never suggest this for production.
>
> >I think if we do decide to ship the feature as a part of the CAS client,
> we should distribute it as a
>
> >separate HttpModule and explain to users the benefits vs. security
> implications of using it.
>
> I think changing it as you originally suggested is a great plan personally,
> I'd rather do that and add a simple web.release.config transformation (which
> i'd assume most are doing to update their CAS server host). Off topic i've
> already applied your suggested changes and have a set of unit tests around
> cas 2.0 ticket processing, i'll patch them up and send when I get back to
> the office.
>
>
>
> Thanks
> Michael
>
>
>
> On Sun, Feb 28, 2010 at 5:31 PM, Scott M. Holodak <sholo...@princeton.edu>
> wrote:
>
> Hi Michael,
>
>
>
> The threading issue makes me a little uncomfortable. Since this module is
> going to be processing multiple requests at the same time, adding the
> delegate, running the request, and removing the delegate can cause a race
> condition between threads. Plus, removing the delegate by setting it to
> null assumes that there was no other delegate setup elsewhere in the
> application to handle certificate validation. You could add locking
> mechanisms around the code in the CAS module, but you would have no way of
> enforcing that locking protocol with third-party code. I think given that
> the ServicePointManager is the only way to handle the validation, you really
> will want to decide whether or not to allow bad certificates for the entire
> lifecycle of the application.
>
>
>
> I’m also a little apprehensive to add this to the module because it could
> change or break the behavior of third-party code. Plus there may also be
> security concerns with regard to accepting any SSL certificate without doing
> any checks at all (expired, wrong domain name, etc.). I’m not sure if this
> is the right approach to the problem. Is there a reason that you aren’t
> trusting the CA certificate instead or treating the self-signed cert as a
> trusted CA cert?
>
>
>
> As an alternative, you could consider adding the code to
> Application_OnStart or Init (not sure which) in global.asax or in a separate
> HttpModule.
>
>
>
> I think if we do decide to ship the feature as a part of the CAS client, we
> should distribute it as a separate HttpModule and explain to users the
> benefits vs. security implications of using it.
>
>
>
> Here is some example code—it would only wire up the delegate if the
> application is compiled in DEBUG mode.
>
>
>
> void Application_OnStart() {
>
> InitializeCertificateValidator();
>
> }
>
>
>
> [Conditional(“DEBUG”)]
>
> void InitializeCertificateValidator() {
>
> ServicePointManager.ServerCertificateValidationCallback =
>
> delegate(Object obj, X509Certificate certificate, X509Chain chain,
> SslPolicyErrors errors)
>
> {
>
> return true;
>
> };
> }
>
>
>
> Anyone have any comments/insight?
>
>
>
> *Scott Holodak **Aspire **|** **A Plan for Princeton*
>
> *Princeton* *University* Office of Development
>
>
>
> *From:* Michael Hans [mailto:foliofnm...@gmail.com]
> *Sent:* Sunday, February 28, 2010 4:02 PM
>
>
> *To:* cas-dev@lists.jasig.org
> *Subject:* Re: [cas-dev] Quirky Dev Patch for Invalid SSL Exceptions in
> .NET
>
>
>
> Hey Scott,
> I'm all for it being re-factored to meet the project standards but
> wanted to provide some context on why I wrapped those calls vs single
> initializing it. An odd use-case i've seen is :If the app itself has some
> custom ssl validating logic it uses in the callback it will remove yours
> once the delegate is set. I've wrapped (in which I agree is not pretty) to
> avoid anything down the process chain from knowing the cas module was
> involved. This is a remote use-case I admit and since it's strictly for
> development purposes I have no objections to moving it into the Initialize
> method.
>
> Thanks
> Michael
>
> On Sun, Feb 28, 2010 at 2:01 PM, Scott M. Holodak <sholo...@princeton.edu>
> wrote:
>
> Hi Michael,
>
> I'm looking at your patch and had a few questions/comments. I'd like to
> re-work your code a little bit so that it gets around the thread safety
> issue.
>
> I think CasAuthentication.Initialize() would be the place to add the
> certificate validation delegate (Initialize() is thread-safe). Also, since
> any changes to web.config will cause the application to recycle (and
> CasAuthentication.Initialize() to pick up the changes), I don't think it
> would require the removal capability (resetting the delegate to null).
> Also, you wouldn't have to add and remove the functionality around each of
> the outbound web requests (i.e., in ticket validators, proxy retriever).
>
> Any questions/comments/objections?
>
> -Scott
>
>
> -----Original Message-----
> From: Michael Hans [mailto:foliofnm...@gmail.com]
> Sent: Friday, February 26, 2010 4:00 PM
> To: cas-dev@lists.jasig.org
> Subject: Re:[cas-dev] Quirky Dev Patch for Invalid SSL Exceptions in .NET
>
> Your right, I didn't consider the fact that an issue could be tabled and
> scheduled for future development or implementation. Jira ticket created
> http://www.ja-sig.org/issues/browse/NETC-10
>
> Thanks
> Michael
> --
>
> You are currently subscribed to cas-dev@lists.jasig.org as:
> sholo...@princeton.edu To unsubscribe, change settings or access archives,
> see http://www.ja-sig.org/wiki/display/JSG/cas-dev
>
>
> --
> You are currently subscribed to cas-dev@lists.jasig.org as:
> foliofnm...@gmail.com
> To unsubscribe, change settings or access archives, see
> http://www.ja-sig.org/wiki/display/JSG/cas-dev
>
>
>
> --
>
>
>
> You are currently subscribed to cas-dev@lists.jasig.org as:
> sholo...@princeton.edu
>
> To unsubscribe, change settings or access archives, see
> http://www.ja-sig.org/wiki/display/JSG/cas-dev
>
> --
>
> You are currently subscribed to cas-dev@lists.jasig.org as:
> foliofnm...@gmail.com
>
> To unsubscribe, change settings or access archives, see
> http://www.ja-sig.org/wiki/display/JSG/cas-dev
>
>
>
> --
>
>
> You are currently subscribed to cas-dev@lists.jasig.org as:
> sholo...@princeton.edu
>
> To unsubscribe, change settings or access archives, see
> http://www.ja-sig.org/wiki/display/JSG/cas-dev
>
> --
> You are currently subscribed to cas-dev@lists.jasig.org as:
> foliofnm...@gmail.com
> To unsubscribe, change settings or access archives, see
> http://www.ja-sig.org/wiki/display/JSG/cas-dev
>
>
--
You are currently subscribed to cas-dev@lists.jasig.org as:
arch...@mail-archive.com
To unsubscribe, change settings or access archives, see
http://www.ja-sig.org/wiki/display/JSG/cas-dev
