Use only securityContext.isContinue() to decide if SC_UNAUTHORIZED re… #339

ASunc · 2016-04-06T10:14:40Z

…sponse is needed. This is needed for SPNEGO situations where browser sends correct authentication information right in the first request. This is different than with NTLM, but this
change works with NTLM also, because isContinue() is true with NTLM if additional negotiation is needed.

This should fix second issue (noticed by @OlivierJaquemet ) in #167.

I have tested this change with Tomcat 8, both with Valve and Filter configurations.
PR #338 is needed before this.

Please test, if it looks ok then I'll make similar changes to tomcat 6/7 code.

OlivierJaquemet · 2016-04-06T12:34:34Z

I'll check the behavior with those PRs asap.

dblock · 2016-04-06T17:07:20Z

👍

OlivierJaquemet · 2016-04-07T09:02:54Z

I do confirm proper behavior of empty POST ajax request with this PR

Tested against master with the 2 PR
both on my local environement and ... on our production environement (yeah I know... rock & roll style, but I trust you guys and it was a success :)
both with Negotiate NTLMSSP (using a dumb domain without spn) or Negotiate Kerberos (with proper domain)

In every case the authentication work properly and empty ajax requests work like a charm.

👍 @AriSuutariST

Do you still maintain Waffle-1.7.x branch and if so will you include this PR ?
(I did see the notification in the 1.8 changelog indicating "Official notification dropping long-term support on 1.7.x branch", but I could not find any details on its exact meaning)

hazendaz · 2016-04-07T10:43:19Z

Support for 1.7 was noted to end after a year as we moved to java 7. However given the nature of the issues, I'm willing to backport fixes if needed. If you are able to use java 7 though and there are no other requests for Java 6 then I'd be inclined not to backport.

Sent from Outlook Mobile

On Thu, Apr 7, 2016 at 2:02 AM -0700, "Olivier Jaquemet" notifications@github.com wrote:

I do confirm proper behavior of empty POST ajax request with this PR

Tested against master with the 2 PR
both on my local environement and ... on our production environement (yeah I know... rock & roll style, but I trust you guys and it was a success :)
both with Negotiate NTLMSSP (using a dumb domain with spn) or Negotiate Kerberos (with proper domain)

In every case the authentication work properly and empty ajax requests work like a charm.

👍 @AriSuutariST

Do you still maintain Waffle-1.7.x branch and if so will you include this PR ?
(I did see the notification in the 1.8 changelog indicating "Official notification dropping long-term support on 1.7.x branch", but I could not find any details on its exact meaning)

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#339 (comment)

dblock · 2016-04-07T11:18:54Z

@hazendaz @AriSuutariST thanks!

OlivierJaquemet · 2016-04-07T12:12:40Z

@hazendaz

We do have some customers still using 1.6, however I will certainly be able to force them to upgrade given our EOL policy and Java EOL.
Also I did check the dependencies of 1.7 vs 1.8 and could not find any conflict with our other libraries.

So don't bother to merge in 1.7.x. unless other users request it, thanks for proposing.

dblock · 2016-04-07T14:54:08Z

I'm merging this.

dblock · 2016-04-07T14:55:08Z

Actually wait, this needs a CHANGELOG entry too, @OlivierJaquemet. Thanks.

While you're at it, try to make those CHANGELOGs look alike, now a bit all over the place, they used to be like so:

[issue](link): Bla bla bla - [@name](link).

hazendaz · 2016-04-08T00:37:03Z

@AriSuutariST Think the change log need was from you. If the tomcat variations are pretty much the same, please make it to all the tomcat versions. If it's slightly different, maybe another PR for that since this part is the core.

ASunc · 2016-04-12T06:54:30Z

I made the change for all tomcat versions. However, the change causes troubles in NegotiateAuthenticatorTests.testChallengePost. When I run only that single test, it passes without problems. But when I run all the tests the token sent by this test is accepted right without any extra negotiation (same way as seen with browser also) and the test fails because it expects 401 response & keep-alive header. Maybe something needs to be done to test so it's results are not affected by previous tests.

ASunc · 2016-04-12T12:15:24Z

One potential fix for unit test would be to use "NTLM" security package instead of "Negotiate" in NegotiateAuthenticatorTests.testChallengePost(), as the problem with test occurs only when "Negotiate" package chooses to use Kerberos.

dblock · 2016-04-27T20:11:39Z

@hazendaz can you please make a call on merging this or not given the note on the test (and hit the merge button if)? I don't want to break everything and I am no longer building this project ;)

hazendaz · 2016-04-27T21:03:38Z

@dblock I'll take a closer look at this as soon as I can. Maybe I'll take another crack at getting tests working on appveyor. It certainly would be nicer if we didn't see all green when things are red ;)

ASunc · 2016-04-28T05:26:14Z

I tried to find solution for fixing the tests (other than switching tests from Negotiate to NTLM), but I was unable to find one. Some kind of caching that occurs when Negotiate uses Kerberos should be somehow disabled, otherwise tests affect each others execution.

hazendaz · 2016-04-29T04:04:04Z

OK - We should let this sit a while longer then so we can figure out a best way approach at resolving it. I know in the past I put catches in the tests for errors I didn't quite understand. This sort of sounds like a related type issue. Possibly there is something we can do to fix them all. We definitely don't want tests leaking between runs.

hazendaz · 2016-12-20T05:13:11Z

@AriSuutariST Is it possible for you to rebase this and possible see if it's any better at this point? I'm close to releasing 1.8.2 and wondering if this is possible to pull in.

hazendaz · 2016-12-20T05:13:39Z

Issue appears to only be change log. I restructured it a bit so technically should be easy to resolve.

ASunc · 2016-12-20T06:26:01Z

@hazendaz I'll try to check this today.

ASunc · 2016-12-20T06:49:44Z

When building after rebase, things look different now. It still fails (for me) at
Tests run: 8, Failures: 0, Errors: 1, Skipped: 3, Time elapsed: 0.32 sec <<< FAILURE! - in waffle.windows.auth.WindowsAuthProviderTests
testSecurityContextsExpire(waffle.windows.auth.WindowsAuthProviderTests) Time elapsed: 0.066 sec <<< ERROR!
com.sun.jna.platform.win32.Win32Exception: The logon attempt failed
at waffle.windows.auth.WindowsAuthProviderTests.testSecurityContextsExpire(WindowsAuthProviderTests.java:230)

But also master branch ends up with same test failure. Maybe someone else could try building this branch to see if it is only me ?

hazendaz · 2016-12-21T05:00:55Z

I'll try to build this on another machine and see what I get. I've had better luck on a new laptop I just got and have been getting setup. I'll let you know.

…sponse is needed.

ASunc · 2017-01-02T08:36:13Z

@hazendaz I rebased this again and tried building on another laptop. Unfortunately it results in same error, for this branch and also for master branch. I starting to wonder if there are any requirements currently for tests to pass successfully that my environment is missing (currently W10 laptops that belong to domain) ?

hazendaz · 2017-01-03T03:04:00Z

I did cut the cord on 1.8.2 release. I'll have to come back to this. Builds work successfully on two of my machines. The .net fails on one of them. Not sure what the issue is. I'm not part of any domain on either. And the one I've always had issues with either java or .net. After many fixes this year the .net was all that was left failing then my machine crashed and I had to rebuild from scratch so I assumed it would start working...but no go. Anyway, I'll try to take a crack at this later this week or next weekend. Really depends on how much my 3 week vacation impacts me at my day job. I'm back tomorrow :(

tjstuart · 2018-03-28T05:51:16Z

Has this been abandoned due to comments in #346? Is this ultimately the fix for #346?

I recently tried upgrading 1.7.3 to 1.8.3 and the "IE sends 0 bytes" issue occurs via AJAX whereas this did not occur in 1.7.3.

I always thought that #39 resolved the "IE 0 bytes issue" as back in 2012 issue #39 was the reason we upgraded from an earlier version. Is this PR to fix some regression since 2012? Or is it a different issue that just behaves the same?

Finally, is there some way I can test this PR without having to build? Anyone can send a waffle-jna.jar with the fix for testing?

hazendaz · 2018-03-31T18:28:55Z

@tjstuart This PR would need entirely rebased before any testing. I don't have time right now to get it rebased. You should however be able to check this out in github desktop and run a rebase against it. However, not how sure how much of a merge conflict issue will be resent as a lot has changed from 2016. You might be better just pulling master and building from there then checking to see if the issue still exists.

tjstuart · 2018-04-03T02:22:41Z

@hazendaz - Pulled the fix and rebased on top of the waffle-1.8.3 tag. I can confirm that the issue is fixed. Please note that I only tested the jna negotiate filter.

hazendaz · 2018-04-03T05:15:09Z

Great! Can you push that back up as a new PR? Also add an entry to change log and I'll merge right away. Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: tjstuart <notifications@github.com> Sent: Monday, April 2, 2018 10:22:41 PM To: Waffle/waffle Cc: Jeremy Landis; Mention Subject: Re: [Waffle/waffle] Use only securityContext.isContinue() to decide if SC_UNAUTHORIZED re… (#339) @hazendaz<https://github.com/hazendaz> - Pulled the fix and rebased on top of the waffle-1.8.3 tag. I can confirm that the issue is fixed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#339 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AA7hoz4tNqH82JXPsLwO0SjUmn-3iDbqks5tktzxgaJpZM4IA7x0>.

tjstuart · 2018-04-03T05:23:53Z

@hazendaz - can do new PR etc. but note that I only tested the NegotiateSecurityFilterProvider, none of the tomcat* changes (although the change appears to be the same across all).

hazendaz · 2018-04-05T00:02:01Z

@tjstuart I'm good with that. I was unable to test the issue and it needed the change log. So the fact that you were able to test and confirm is very helpful in applying it.

hazendaz · 2018-04-07T16:20:48Z

Merged via #609

This was referenced May 2, 2016

Internet Explorer bodyless POST optimization with SPNEGO header and accept-incomplete not handled #346

Open

Java SPNego bug #167

Open

Use only securityContext.isContinue() to decide if SC_UNAUTHORIZED re…

a59f76c

…sponse is needed.

tjstuart mentioned this pull request Apr 5, 2018

Use only securityContext.isContinue() to decide if SC_UNAUTHORIZED re… #609

Merged

hazendaz closed this Apr 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use only securityContext.isContinue() to decide if SC_UNAUTHORIZED re… #339

Use only securityContext.isContinue() to decide if SC_UNAUTHORIZED re… #339

ASunc commented Apr 6, 2016

OlivierJaquemet commented Apr 6, 2016

dblock commented Apr 6, 2016

OlivierJaquemet commented Apr 7, 2016

hazendaz commented Apr 7, 2016

dblock commented Apr 7, 2016

OlivierJaquemet commented Apr 7, 2016

dblock commented Apr 7, 2016

dblock commented Apr 7, 2016

hazendaz commented Apr 8, 2016

ASunc commented Apr 12, 2016

ASunc commented Apr 12, 2016

dblock commented Apr 27, 2016 •

edited

Loading

hazendaz commented Apr 27, 2016

ASunc commented Apr 28, 2016

hazendaz commented Apr 29, 2016

hazendaz commented Dec 20, 2016

hazendaz commented Dec 20, 2016

ASunc commented Dec 20, 2016

ASunc commented Dec 20, 2016

hazendaz commented Dec 21, 2016

ASunc commented Jan 2, 2017

hazendaz commented Jan 3, 2017

tjstuart commented Mar 28, 2018 •

edited

Loading

hazendaz commented Mar 31, 2018

tjstuart commented Apr 3, 2018 •

edited

Loading

hazendaz commented Apr 3, 2018 via email

tjstuart commented Apr 3, 2018 •

edited

Loading

hazendaz commented Apr 5, 2018

hazendaz commented Apr 7, 2018

Use only securityContext.isContinue() to decide if SC_UNAUTHORIZED re… #339

Use only securityContext.isContinue() to decide if SC_UNAUTHORIZED re… #339

Conversation

ASunc commented Apr 6, 2016

OlivierJaquemet commented Apr 6, 2016

dblock commented Apr 6, 2016

OlivierJaquemet commented Apr 7, 2016

hazendaz commented Apr 7, 2016

dblock commented Apr 7, 2016

OlivierJaquemet commented Apr 7, 2016

dblock commented Apr 7, 2016

dblock commented Apr 7, 2016

hazendaz commented Apr 8, 2016

ASunc commented Apr 12, 2016

ASunc commented Apr 12, 2016

dblock commented Apr 27, 2016 • edited Loading

hazendaz commented Apr 27, 2016

ASunc commented Apr 28, 2016

hazendaz commented Apr 29, 2016

hazendaz commented Dec 20, 2016

hazendaz commented Dec 20, 2016

ASunc commented Dec 20, 2016

ASunc commented Dec 20, 2016

hazendaz commented Dec 21, 2016

ASunc commented Jan 2, 2017

hazendaz commented Jan 3, 2017

tjstuart commented Mar 28, 2018 • edited Loading

hazendaz commented Mar 31, 2018

tjstuart commented Apr 3, 2018 • edited Loading

hazendaz commented Apr 3, 2018 via email

tjstuart commented Apr 3, 2018 • edited Loading

hazendaz commented Apr 5, 2018

hazendaz commented Apr 7, 2018

dblock commented Apr 27, 2016 •

edited

Loading

tjstuart commented Mar 28, 2018 •

edited

Loading

tjstuart commented Apr 3, 2018 •

edited

Loading

tjstuart commented Apr 3, 2018 •

edited

Loading