Pr/630 - DisableSso flag, Delete option, skip when running on non windows #636

hazendaz · 2018-07-01T18:42:13Z

This applies all changes of Pr/630 with exception to the browser being forced to windows only.

hazendaz · 2018-07-01T18:44:51Z

@MoreHeapSpace @pedroneil Please take a quick look over this PR adjustment.

The only item pulled out was the disabling if browser was non windows. I agree on that it should be separate init param and not tied to the windows check. I did rewrite the windows check to be more performant so it was not constantly checking that it was on windows from system params and confirming using 'win' which is generally better than 'Windows', although I just released I forgot to handle casing so I'll push that in a moment.

thanks,

Jeremy

- Added ability to disable SSO through filter init param 'disableSSO'. This is usefull if you require to bypass the SSO process for troubleshooting/diagnostics and provide login by some other means - Resume filter chain if deployed on windows platform

…s IE will strip the body on challenge: https://stackoverflow.com/questions/40918017/ie-sends-empty-post-body-randomly-while-using-ntlm-authentication-using-angular https://support.microsoft.com/en-us/help/2749007/an-unexpected-401-1-status-is-returned-when-using-pre-authentication-h

hazendaz · 2018-07-01T19:15:21Z

I"m changing 'enableSSO' to disableSSO'. The reason behind that is that this would otherwise disable entirely unless user explicitely sets enableSSO to true in init params. As such it would be a breaking change and I do not believe we should force setting of that value by default. So 'disableSSO' makes more sense. Adjustment coming shortly plus build will be fixed here :)

hazendaz · 2018-07-01T19:28:48Z

reference #630

dblock · 2018-07-02T22:14:19Z

Source/JNA/waffle-jna/src/main/java/waffle/servlet/NegotiateSecurityFilter.java

@@ -106,6 +112,20 @@ public void doFilter(final ServletRequest sreq, final ServletResponse sres, fina
        NegotiateSecurityFilter.LOGGER.debug("{} {}, contentlength: {}", request.getMethod(), request.getRequestURI(),
                Integer.valueOf(request.getContentLength()));

+        // If we are not in a windows environment, resume filter chain
+        if (!NegotiateSecurityFilter.isWindows()) {


I'm late to this PR, sorry, but what's the scenario for this? Why would one ever run this filter on a non-windows system? And if that system supports SSO then why would this be skipping it?

I can see using vintela and waffle with both activated. This would short ciruit one vs the other allowing them to both remain activated without having to comment one or the other filter out. There are other means to achieve the same but I think trying to do it centrally while waffle doesn't support *nix seems appropriate. I did downgrade the log to debug in this case as I felt info was too high. But then again I'm imagining this the way I'm looking to leverage the feature so not sure of requesters ambitions here :)

dblock · 2018-07-02T22:16:23Z

Source/JNA/waffle-jna/src/main/java/waffle/util/AuthorizationHeader.java

     */
    public boolean isNtlmType1PostAuthorizationHeader() {
-        if (!"POST".equals(this.request.getMethod()) && !"PUT".equals(this.request.getMethod())) {
+        if (!"POST".equals(this.request.getMethod()) && !"PUT".equals(this.request.getMethod())
+                && !"DELETE".equals(this.request.getMethod())) {


AFAIK the NTLM spec says these two methods must be POST or PUT and that the browser will only send those two preemptively - where/when has this changed and with what browser are we seeing it?

Issue was stated to be with IE. There was a link on the original PR, possibly on here still as well.

I'm now not convinced this was fixing anything. The two links I read entirely through and it's regarding a POST without a post body during pre-authentication. That is quite different situation than a DELETE being used. I'll look to remove this unless there is compelling reason for it to stay. @MoreHeapSpace Can you elaberate as to why DELETE solves an issue here?

Sorry for the late reply. I've encountered this problem during pre-authentication on a DELETE action that contains a body. Yes the articles do only document about POST, sorry should have clarified that the same process happens with the DELETE action

If the spec doesn't call for DELETE we should be very careful adding it. What's the client that's exhibiting this behavior?

Agreed, and the client is IE 11

I suspect the same will happen the PATCH method. In fact with any request that contains a body. I have yet to test these as my rest api doesn't make use of that method.

While the NTLM spec only refers to POST or PUT, I believe IE will lock the body all requests

dblock · 2018-07-02T22:18:58Z

Docs/ServletSingleSignOnSecurityFilter.md

@@ -32,6 +32,7 @@ Filter Options

 The filter can be configured with the following `init-param` options. 

+* disableSSO: Allows for the enabling/disabling of Single-SignOn Security Filter.This is useful if you require to bypass the SSO process for troubleshooting/diagnostics and provide login by some other means.


Feels to me that you never would want this feature and if you need to disable SSO for troubleshooting or diagnostics you would comment out/delete the filter.

This is also why the spec for these filters doesn't include an enabled feature, it can be accomplished as easily bu other means.

Finally, this is a negative flag (disable), which is an anti-pattern and if for some reason something like this should be added it probably would be enabled defaulting to true.

I can imagine this need for a newly onboarded team member that needs to debug and code within the app but is waiting on getting proper AD access to the app. Sure option is to disable filter by other means but this is nicer I think. Plus some automation can be put around the process, such as a bash script that would ask the user if they wanted it on or off.

We have an on-prem offering of our solution where our client's have the ability to enable or disable the use of SSO, and we automate the restarting of the server with whatever selection they've made. Makes it much easier than having a client find and go through xml files to comment/remove xml fragments.

That makes sense, so my comment is really only about naming of this option and the location where it's implemented.

dblock · 2018-07-04T06:19:10Z

I'll leave this to you @hazendaz, but I still recommend reverting the entire PR even if a version with these options has shipped. There're no useful explanations of why these features are needed from @MoreHeapSpace #630, I am not personally satisfied with a newly onboarded team member example since I've never seen one such team member. I do see a ton of risks though.

I worry that there's code being introduced that is skipping security filters under various conditions that get increasingly complicated or fragile. This increases security risks and more branches are not even tested (eg. there're no tests that the filer is skipped on linux for example because these filters aren't supposed to even run on Linux).

The OS check for the word "win" in the OS string is a great example of how bad this can get and how that kind of code gets buried deep. When someone gets hacked tomorrow because a non-educated user is toggling enableSSO for "testing purposes" because they see an auth popup in a browser against a production server.

dblock · 2018-07-04T06:22:43Z

@pedroneil I'm curious, how is the enable/disable flag helping you? Why are you installing the filter on a non-Windows environment?

Enabling SSO on *nix against Active Directory with Samba would be great. I've spent two years trying to make that work on and off without success when I was actively maintaining this library.

pedroneil · 2018-07-05T22:06:31Z

@dblock I don't and won't ever use disableSSO and I didn't want the OS checks.

My comment on #630; I should have just written, "please don't". rather make the comments that I did trying to influence a "not pull"

When I deploy the playbooks would use os family and create different web deployments for linux and windows.

PS:
I do understand how some Enterprise deployments someone might only have operator access where all a person can do is change Servlet and Filter parameters and they do not have permissions to "remove" a filter from the chain.

gstanchev · 2018-07-05T22:20:26Z

You can set the protected URI to some bogus location which essentially disables the filter just as easy as setting a parameter to false or true. One reason to have enabled/disabled is if you have some tool that actively manages your filters (we have written such tool and it is considerably easier to set a boolean param than moving XML in and out of the deployment descriptor or commenting the filter defs).

pedroneil · 2018-07-05T22:28:02Z

similarly would be to use the excludePatterns ./. which will exclude everything and there would be no need to introduce a new init-param

hazendaz · 2018-07-06T00:58:31Z

While I normally would not like any ability to disable the filter, working examples I see far too often already have extended and disabled anyways in very inconsistent ways. So given, it's just as easy to disable the entire thing through other means, for automation purposes, I can see some limited value to having a disableSSO. I'm not yet convinced that exposes a security risk here when it's just as easy to do something really stupid and turn it off in a production environment. Hence why I changed the original request so SSO was not otherwise always disabled unless asked for. Although that does call into question the entire original PR.

For the not windows short circuit, I see more value on that and it fits a use case I'm looking at. However, it is easily manipulated check. I do have a flip type of check elsewhere to short circuit vintela in a similar way on windows and a unit test that will pretend to be linux on windows. I think the only way to make that stronger is to not suggest there is another authentication mechanism but to enforce it through some further registration.

Now going further, seeing the other holes here with the exclude pattern urls raises my interest even more. I think we should ultimately prevent a short circuit like that which is rather undocumented.

At the moment, a lot of food for thought.

MoreHeapSpace and others added 2 commits July 1, 2018 15:13

hazendaz force-pushed the pr/630 branch from 373cd09 to df0e433 Compare July 1, 2018 19:27

hazendaz mentioned this pull request Jul 1, 2018

Pull Request to merge the following #630

Closed

[docs] Update changelog

7ac577f

hazendaz force-pushed the pr/630 branch from df0e433 to 7ac577f Compare July 1, 2018 22:16

hazendaz changed the title ~~Pr/630 - EnableSso flag, Delete option, skip when running on non windows~~ Pr/630 - DisableSso flag, Delete option, skip when running on non windows Jul 1, 2018

hazendaz merged commit 15660fa into master Jul 2, 2018

dblock reviewed Jul 2, 2018

View reviewed changes

hazendaz mentioned this pull request Jul 4, 2018

Use jna to determine OS type #637

Open

This was referenced Jul 6, 2018

HTTP Authentication and Authorization Failure always sending 401 Challenge #638

Open

Implement an enabled flag for all filters #639

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr/630 - DisableSso flag, Delete option, skip when running on non windows #636

Pr/630 - DisableSso flag, Delete option, skip when running on non windows #636

hazendaz commented Jul 1, 2018

hazendaz commented Jul 1, 2018

hazendaz commented Jul 1, 2018

hazendaz commented Jul 1, 2018

dblock Jul 2, 2018

hazendaz Jul 4, 2018

dblock Jul 2, 2018

hazendaz Jul 4, 2018

hazendaz Jul 6, 2018

MoreHeapSpace Jul 9, 2018

dblock Jul 9, 2018

MoreHeapSpace Jul 9, 2018

MoreHeapSpace Jul 9, 2018

dblock Jul 2, 2018 •

edited

hazendaz Jul 4, 2018

MoreHeapSpace Jul 9, 2018

dblock Jul 9, 2018

dblock commented Jul 4, 2018 •

edited

dblock commented Jul 4, 2018

pedroneil commented Jul 5, 2018

gstanchev commented Jul 5, 2018

pedroneil commented Jul 5, 2018

hazendaz commented Jul 6, 2018

		@@ -32,6 +32,7 @@ Filter Options

		The filter can be configured with the following `init-param` options.

		* disableSSO: Allows for the enabling/disabling of Single-SignOn Security Filter.This is useful if you require to bypass the SSO process for troubleshooting/diagnostics and provide login by some other means.

Pr/630 - DisableSso flag, Delete option, skip when running on non windows #636

Pr/630 - DisableSso flag, Delete option, skip when running on non windows #636

Conversation

hazendaz commented Jul 1, 2018

hazendaz commented Jul 1, 2018

hazendaz commented Jul 1, 2018

hazendaz commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock Jul 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Jul 4, 2018 • edited

dblock commented Jul 4, 2018

pedroneil commented Jul 5, 2018

gstanchev commented Jul 5, 2018

pedroneil commented Jul 5, 2018

hazendaz commented Jul 6, 2018

dblock Jul 2, 2018 •

edited

dblock commented Jul 4, 2018 •

edited