Fwd: 8078439: 8048194: possible bug in commit for these two fixes

[Weijun Wang]

Hi Darwin

Thanks for looking into this file. Most of your words are correct, but 
I'd like to explain what was behind those changesets.

See comments below.

On 5/21/2015 3:58 PM, Seán Coffey wrote:
> FYI,
>
> concerns from Darwin on the 8078439: 8048194 fixes.
>
> regards,
> Sean.
>
> -------- Forwarded Message --------
> Subject: 	8078439: 8048194: possible bug in commit for these two fixes
> Date: 	Wed, 20 May 2015 16:28:29 -0700
> From: 	Darwin Felix <darwinfelix at yahoo.com>
> To: 	jdk8u-dev at openjdk.java.net
> CC: 	darwinfelix at users.sourceforge.net
>
>
>
> 8078439: 8048194: possible bug in commit for these two fixes
>
> It appears that this commit has a bug:
>
> http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/d777e2918a77/src/share/classes/sun/security/jgss/spnego/SpNegoContext.java
> Fundamentally, it appears that this commit desires to honor the initiator/client's preferred mechanism - first one in the list of mechanisms the client is offering to the target/server.
> However, the commit does not allow for the condition where the server can only support a different mechanism from the client's offering/list (server not able to support mechList[0] but is able to support mechList[N]).

In SPNEGO, the client always sends a mechToken using mechList[0]. Before 
this change for JDK-8048194, even if the server does not support 
mechList[0] and prefer mechList[n], it blindly calls 
GSS_acceptSecContext() on the mechToken as if it's for mechList[n], 
which will fail immediately. After the fix, server will not touch 
mechToken anymore. It sends its own preferred mech (mech_wanted, which 
is mechList[n]) back to client and wait for the client to send a 
mechToken for this mech in the 2nd round.

>
> In my humble opinion, based on my quick glance of the code, this commit is broken because the server is no longer allowed to select from one of the other mechanisms from the client's list.

The server does select one (line 532) and inform the client about it 
(line 591).

>
>
> This next commit also appears to have a bug:
>
> http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/9ab9f20e9bdd/src/share/classes/sun/security/jgss/spnego/SpNegoContext.java
> It appears that this commit also desires to honor the initiator/client's preferred mechanism.
> However, this commit also appears to suffer from the condition where the server can only support a different mechanism from the client's offering/list (server not able to support mechList[0] but is able to support mechList[N]).

As I described in the bug comment for JDK-8078439, client sends 
Microsoft's krb5 OID as mechList[0] but Java only understands the 
standard krb5 OID. After the *previous* fix, it sends back the standard 
OID and wait for the client to send a mechToken for it. It will be 
something like

   c: Here is a mechToken for MS krb5 OID
   s: Sorry, I don't know that. Please send a token for standard krb5 OID
   c: Sure, here it is
   s: Good, here is my response token

However, we are seeing clients that only expect one round of 
communication and when it cannot see a response token as the 1st reply 
it just fails. Thus the 2nd fix:

   c: Here is a mechToken for MS krb5 OID
   s: Wow, MS krb5 OID, I understand now. Here is my response token
   [A happy c]

BTW, before these 2 fixes, it was

   c: Here is a mechToken for MS krb5 OID. I also know standard krb5
   s: I know about standard krb5. Here is my response token
   [A happy c]

>
>
>
> Perhaps the above two (2) commits should be reverted?
>
> An alternative approach to supporting/honoring the initiator/client's preferred mechanism:
>
> Prior to the above two (2) commits, SpNegoContext.java suffered from the condition where it did not make an attempt/consider to use the initiator/client's preferred mechanism. Instead, the code would find a mechanism from the client's list that the server can support. Meaning, the server's supported/preferred mechanisms is the outer loop and the client's supported/preferred mechanism is the inner loop (preference specified by its order in the list).

Exactly, and this is not correct. The reason I didn't touch this is that 
Java only supports krb5 now, so whichever is outer or inner loop does 
not really make a difference. We will fix it once we support a new mech.

>
> Here's what the code looked like prior to the above two (2) commits:
>
> http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/1a3de3cdc684/src/share/classes/sun/security/jgss/spnego/SpNegoContext.java
> Perhaps, if we simply place the client's list as the outer loop and the server's list as the inner loop, we might be able to honor the client's preferred mechanism while ensuring that the chosen mechanism is one that the server can support.
>
> For example, have a look at the implementation of the method named
>
> SpNegoContext.negotiate_mech_type(supported_mechSet, mechList)
>
> Notice that the server's list is chosen as being the outer loop. It might make more sense to have the client's list be the outer loop.
>
>
> If possible, I would like to discuss the possibility of reverting the above two commits and instead implement my proposed change (mechList as the outer loop and supported_mechSet as the inner loop) to the method named negotiate_mech_type.

But what JDK-8048194 describes still exists:

   c: Here is a mechToken for XYZ mech. BTW, I also support krb5
   s: Aha, I also support krb5, let me accept your mechToken...
      bang... token not parseable... fail...
   [ c still waiting ]

Thanks
Max

>
>
> thanks,
> -darwin
>
>
>