Re: Code review request: 8012615: Realm.getRealmsList returns realms list in wrong
On 8/29/13 10:37 AM, Valerie (Yu-Ching) Peng wrote:
Thanks for the clarification on MIT's result on the test cases.
So, for majority of the test cases inside the regression test, we return
different results from what MIT returns? That's a little unsettling.
Well that's about the chaining Java does. Since that's the harder part
to implement, I add more tests for it.
How about for the more complicated valid capaths definition?
Yes I can. However, since the MIT style just reads one value, I doubt it
can be more complicated. It can be longer of course.
We for sure will return the same results as MIT's, right?
I think so. At least for the first 4 cases in the test, which is from
MIT's own krb5.conf example, the results are the same.
It seems strange/confusing to have all these invalid capaths definition
as regression test.
I can see the value of the infinite-loop case to ensure that no OOME
occurs. But why should we care about the rest?
Yes, I believe there is no right answer to an invalid capaths. But since
we had to deal with them anyway, I'd like to keep these test cases to
make sure the implementation is consistent.
I hope that we won't be bound by these "non-interoperable" (or
vendor-specific) interpretation in the future.
As for your "learning from MIT" comment, if I understand it correctly,
it means that we will no longer treating the missing definition as "=."
but rather go hierarchy, right?
Correct.
Thanks
Max
Valerie
On 08/22/13 21:55, Weijun Wang wrote:
On 8/23/13 10:39 AM, Valerie (Yu-Ching) Peng wrote:
1. Line 255, "returns if keys exists" should be "returns true if key
exists".
2. Line 257, "@see get" should be "@see get0"?
I meant looking at the how IAE is thrown in get. Updated to
* @throws IllegalArgumentException if any of the keys is illegal
* (See {@link #get})
3. You may want to add the following to the public getAll(String...
keys) method.
@throws IllegalArgumentException ...
Yes.
looks fine
Before I looked at Realm.java, I looked at the test first to understand
the expected realm list result.
Well, judging from the test, I feel the parsing of these CA path
settings are too relaxed.
You are allowing all sorts of short cuts and user errors. When has
capaths been implemented?
I wonder what happens when MIT implementation run into these invalid
capath settings.
Is their implementation also interpret them like what you have here?
Below are all differences using the krb5.conf in test
- 1 --
TIVOLI.COM = {
IBM.COM = IBM_LDAPCENTRAL.COM MOONLITE.ORG
IBM_LDAPCENTRAL.COM = LDAPCENTRAL.NET
LDAPCENTRAL.NET = .
}
TIVOLI.COM -> IBM.COM
MIT: [TIVOLI.COM, IBM_LDAPCENTRAL.COM]
java: [TIVOLI.COM, LDAPCENTRAL.NET, IBM_LDAPCENTRAL.COM,
MOONLITE.ORG]
Here, the value for IBM.COM is two values on the same line, this is no
legal in MIT. It simply read the first one. Java reads both and
combines it with another value
Hmm, interesting.
-- 2 ---
B1.COM = {
B2.COM = .
B3.COM = B2.COM
B4.COM = B3.COM
}
B1.COM -> B4.COM
MIT : [B1.COM, B3.COM]
java: [B1.COM, B2.COM, B3.COM]
Java combines 2 values
--- 3 ---
C1.COM = {
C3.COM = C2.COM
}
C1.COM -> C2.COM
MIT : [C1.COM, COM]
java: [C1.COM]
When MIT cannot find the sRealm as key, it goes hierarchy. Java only
does this when there is no cRealm sub-section
4 ---
D1.COM = {
D2.COM=D1.COM
}
D1.COM -> D2.COM
MIT : [D1.COM, D1.COM]
java: [D1.COM]
MIT returns dup realms
--- 5 -
E1.COM = {
E2.COM = E2.COM
E3.COM = E4.COM
E3.COM = .
}
E1.COM -> E2.COM
MIT : [E1.COM, E2.COM]
java: [E1.COM]
MIT returns dup realms (please remember path does not include last realm)
E1.COM -> E3.COM
MIT : [E1.COM, E4.COM, .]
java: [E1.COM, E4.COM]
"." should only appear as single value for one key. MIT does not
notice it and blindly returns
--- 6
A9.PRAGUE.XXX.CZ = {
PRAGUE.XXX.CZ = .
ROOT.XXX.CZ = PRAGUE.XXX.CZ
SERVIS.XXX.CZ = ROOT.XXX.CZ
}
A9.PRAGUE.XXX.CZ -> SERVIS.XXX.CZ
MIT : [A9.PRAGUE.XXX.CZ, ROOT.XXX.CZ]
java: [A9.PRAGUE.XXX.CZ, PRAGUE.XXX.CZ, ROOT.XXX.CZ]
Java combines 2 values
For 1, 2, 6, Java combines and MIT reads single key. Since we decided
to do this, it's OK.
4, 5, wrong config, Java manages to return a path as leasts look
normal, MIT returns ugly path.
3, This is probably something we can learn from MIT. If you agree,
I'll update webrev.
I think your new comment on line 71 is more confusing.
Can you just say "D2=D1 is the same as D2=."?
Yes. D1 should not appear as a target when it is the client realm. So
"D2=D1" is just ignored, which is the same as "D2=.".
As said above, if we decide to choose MIT's way to require a sRealm
key to use capaths, "D2=D1" will still be regarded the same as "D2=.",
but it will be different from no D2 l
Re: Code review request: 8012615: Realm.getRealmsList returns realms list in wrong
Thanks for the clarification on MIT's result on the test cases.
So, for majority of the test cases inside the regression test, we return
different results from what MIT returns? That's a little unsettling. How
about for the more complicated valid capaths definition? We for sure
will return the same results as MIT's, right?
It seems strange/confusing to have all these invalid capaths definition
as regression test.
I can see the value of the infinite-loop case to ensure that no OOME
occurs. But why should we care about the rest?
I hope that we won't be bound by these "non-interoperable" (or
vendor-specific) interpretation in the future.
As for your "learning from MIT" comment, if I understand it correctly,
it means that we will no longer treating the missing definition as "=."
but rather go hierarchy, right?
Valerie
On 08/22/13 21:55, Weijun Wang wrote:
On 8/23/13 10:39 AM, Valerie (Yu-Ching) Peng wrote:
1. Line 255, "returns if keys exists" should be "returns true if key
exists".
2. Line 257, "@see get" should be "@see get0"?
I meant looking at the how IAE is thrown in get. Updated to
* @throws IllegalArgumentException if any of the keys is illegal
* (See {@link #get})
3. You may want to add the following to the public getAll(String...
keys) method.
@throws IllegalArgumentException ...
Yes.
looks fine
Before I looked at Realm.java, I looked at the test first to understand
the expected realm list result.
Well, judging from the test, I feel the parsing of these CA path
settings are too relaxed.
You are allowing all sorts of short cuts and user errors. When has
capaths been implemented?
I wonder what happens when MIT implementation run into these invalid
capath settings.
Is their implementation also interpret them like what you have here?
Below are all differences using the krb5.conf in test
- 1 --
TIVOLI.COM = {
IBM.COM = IBM_LDAPCENTRAL.COM MOONLITE.ORG
IBM_LDAPCENTRAL.COM = LDAPCENTRAL.NET
LDAPCENTRAL.NET = .
}
TIVOLI.COM -> IBM.COM
MIT: [TIVOLI.COM, IBM_LDAPCENTRAL.COM]
java: [TIVOLI.COM, LDAPCENTRAL.NET, IBM_LDAPCENTRAL.COM,
MOONLITE.ORG]
Here, the value for IBM.COM is two values on the same line, this is no
legal in MIT. It simply read the first one. Java reads both and
combines it with another value
Hmm, interesting.
-- 2 ---
B1.COM = {
B2.COM = .
B3.COM = B2.COM
B4.COM = B3.COM
}
B1.COM -> B4.COM
MIT : [B1.COM, B3.COM]
java: [B1.COM, B2.COM, B3.COM]
Java combines 2 values
--- 3 ---
C1.COM = {
C3.COM = C2.COM
}
C1.COM -> C2.COM
MIT : [C1.COM, COM]
java: [C1.COM]
When MIT cannot find the sRealm as key, it goes hierarchy. Java only
does this when there is no cRealm sub-section
4 ---
D1.COM = {
D2.COM=D1.COM
}
D1.COM -> D2.COM
MIT : [D1.COM, D1.COM]
java: [D1.COM]
MIT returns dup realms
--- 5 -
E1.COM = {
E2.COM = E2.COM
E3.COM = E4.COM
E3.COM = .
}
E1.COM -> E2.COM
MIT : [E1.COM, E2.COM]
java: [E1.COM]
MIT returns dup realms (please remember path does not include last realm)
E1.COM -> E3.COM
MIT : [E1.COM, E4.COM, .]
java: [E1.COM, E4.COM]
"." should only appear as single value for one key. MIT does not
notice it and blindly returns
--- 6
A9.PRAGUE.XXX.CZ = {
PRAGUE.XXX.CZ = .
ROOT.XXX.CZ = PRAGUE.XXX.CZ
SERVIS.XXX.CZ = ROOT.XXX.CZ
}
A9.PRAGUE.XXX.CZ -> SERVIS.XXX.CZ
MIT : [A9.PRAGUE.XXX.CZ, ROOT.XXX.CZ]
java: [A9.PRAGUE.XXX.CZ, PRAGUE.XXX.CZ, ROOT.XXX.CZ]
Java combines 2 values
For 1, 2, 6, Java combines and MIT reads single key. Since we decided
to do this, it's OK.
4, 5, wrong config, Java manages to return a path as leasts look
normal, MIT returns ugly path.
3, This is probably something we can learn from MIT. If you agree,
I'll update webrev.
I think your new comment on line 71 is more confusing.
Can you just say "D2=D1 is the same as D2=."?
Yes. D1 should not appear as a target when it is the client realm. So
"D2=D1" is just ignored, which is the same as "D2=.".
As said above, if we decide to choose MIT's way to require a sRealm
key to use capaths, "D2=D1" will still be regarded the same as "D2=.",
but it will be different from no D2 line at all.
What happens for the infinite loop case, i.e. G?
What should (G1, G2) returns? G1 G3?
Yes. I first find G3, and look at G3=G2. Any realm that is either
client, or target, or has appeared in the path is ignored.
In this case, the config itself has a problem so there is no correct
answer. Just want to make sure the search does not leads to a real
infinite loop and then OOME.
Thanks
Max
Thanks,
Valerie
On 07/29/13 02:11, Weijun Wang wrote:
Hi Valerie
Please review the capaths code change at
http://cr.openjdk.java.net/~weijun/8012615/webrev.01/
It includes:
Config.java
===
Add m
Re: Code review request: 8012615: Realm.getRealmsList returns realms list in wrong
I can't speak to the code, but your description of what *should* happen is correct. I have no experience with complex X-relam setups (which have security issues even if the code is correct), but I do know that the MIT code does not "nest" its processing of the capath configuration which causes some non-intuitive behavior. I expect that Heimdal does the same thing as MIT, but if you have time it might be nice to confirm that. If there are differences, I would welcome a discussion on the Heimdal list. On Jul 29, 2013, at 2:11 AM, Weijun Wang wrote: > Hi Valerie > > Please review the capaths code change at > > http://cr.openjdk.java.net/~weijun/8012615/webrev.01/ > > It includes: > > Config.java > === > > Add method to check if a sub-stanza exists. > > Realm.java > == > > Rewrite reading cross-realm path for both [capaths] and hierarchy. The > [capaths] part implements the chaining process. > > CredentialsUtils.java > = > > When reading cross-realm TGT for a path A->B->C->D->SERVERREALM, the current > impl first gets krbtgt/SERVERREALM@A, and then fallback to krbtgt/D@A, > krbtgt/C@A and krbtgt/B@A. In fact, since the capath is already there, > krbtgt/B@A should be the first to check. I don't know about the history of > this code and dare not change much. But I at least reverse the order of the > fallback (what the code calls inner loop) to try krbtgt/B@A first. > > Tried on a local setup of 5 MIT KDC realms configured with a one-direction > cross-auth from K1 to K5. The MIT kvno command starts with kinit in K1 and > goes thru krbtgt/K2@K1, krbtgt/K3@K2, krbtgt/K4@K3, krbtgt/K5@K4 and finally > get service ticket to host/host.k5@K5. Now Java can do the same with the same > krb5.conf. > > Thanks > Max
hg: jdk8/tl/langtools: 8010310: [javadoc] Error processing sources with -private
Changeset: 189942cdf585 Author:jjg Date: 2013-08-28 15:40 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/189942cdf585 8010310: [javadoc] Error processing sources with -private Reviewed-by: vromero, mcimadamore ! src/share/classes/com/sun/tools/javac/code/Symbol.java ! src/share/classes/com/sun/tools/javadoc/JavadocMemberEnter.java + test/tools/javadoc/nonConstExprs/Test.java
hg: jdk8/tl/jdk: 8023155: Ensure functional consistency across Random, ThreadLocalRandom, SplittableRandom
Changeset: b1f41565b806 Author:psandoz Date: 2013-08-28 22:11 +0200 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b1f41565b806 8023155: Ensure functional consistency across Random, ThreadLocalRandom, SplittableRandom Reviewed-by: mduigou Contributed-by: Doug Lea , Paul Sandoz ! src/share/classes/java/util/Random.java ! src/share/classes/java/util/concurrent/ThreadLocalRandom.java ! test/java/util/Random/RandomStreamTest.java + test/java/util/Random/RandomTest.java ! test/java/util/SplittableRandom/SplittableRandomTest.java + test/java/util/concurrent/ThreadLocalRandom/ThreadLocalRandomTest.java
hg: jdk8/tl/langtools: 8014566: Remove @ignore tags from MethodReference66 and InInterface when 8013875 is fixed
Changeset: 7de7100c30ce Author:henryjen Date: 2013-08-28 10:17 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/7de7100c30ce 8014566: Remove @ignore tags from MethodReference66 and InInterface when 8013875 is fixed Reviewed-by: briangoetz, jjg ! test/tools/javac/lambda/MethodReference66.java ! test/tools/javac/lambda/lambdaExecution/InInterface.java
hg: jdk8/tl/jdk: 8023275: Wrapping collections should override default methods
Changeset: ade440668f94 Author:henryjen Date: 2013-08-26 22:32 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ade440668f94 8023275: Wrapping collections should override default methods Reviewed-by: mduigou, psandoz ! src/share/classes/java/util/Collections.java + test/java/util/Collections/Wrappers.java
Re: [kitten] Fwd: Kitten Digest, Vol 104, Issue 14
> "Mayank" == Mayank Upadhyay writes: Mayank>Hi Weijun, You point out a legitimate problem, but I want Mayank> to understand a couple of assumptions: 1. Why allow only Mayank> initSecContext() and acceptSecContext() to have this new Mayank> behavior? Imagine a mechanism built on top of TLS which is Mayank> renegotiating the session intermixed with actual payload, Mayank> and had some error it wanted to communicate to the peer Mayank> (e.g., a TLS Alert). Is there any particular reason you'd Mayank> like to avoid that scenario? 2. I didn't quite follow the Hi. RFC 2743 doesn't allow the abstract wrap or getmic apis to generate an error token. I'd object to adding that behavior to the java bindings without adding it to the abstract API. So, for the current abstract API, only initSecContext and acceptSecContext can have this issue.
hg: jdk8/tl/jdk: 8023528: Rename Comparator combinators to disambiguate overloading methods
Changeset: be2d25a277a7 Author:henryjen Date: 2013-08-21 20:41 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/be2d25a277a7 8023528: Rename Comparator combinators to disambiguate overloading methods Reviewed-by: mduigou, smarks ! src/share/classes/java/util/Comparator.java ! test/java/util/Comparator/BasicTest.java ! test/java/util/Map/EntryComparators.java ! test/java/util/function/BinaryOperator/BasicTest.java
hg: jdk8/tl/jdk: 8023713: ZipFileSystem crashes on old zip file
Changeset: 690b2931baef Author:sherman Date: 2013-08-28 09:46 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/690b2931baef 8023713: ZipFileSystem crashes on old zip file Summary: to handle extra data field copy correctly even the extra data does not follow the spec Reviewed-by: alanb, martin, chegar ! src/share/classes/java/util/zip/ZipOutputStream.java ! test/java/util/zip/TestExtraTime.java
hg: jdk8/tl/jdk: 8022594: Potential deadlock in of sun.nio.ch.Util/IOUtil
Changeset: 378acd4d03c8 Author:alanb Date: 2013-08-28 15:50 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/378acd4d03c8 8022594: Potential deadlock in of sun.nio.ch.Util/IOUtil Reviewed-by: chegar ! src/macosx/classes/sun/nio/ch/KQueueArrayWrapper.java ! src/macosx/classes/sun/nio/ch/KQueueSelectorImpl.java ! src/share/classes/sun/nio/ch/AbstractPollSelectorImpl.java ! src/share/classes/sun/nio/ch/DatagramChannelImpl.java ! src/share/classes/sun/nio/ch/FileChannelImpl.java ! src/share/classes/sun/nio/ch/IOUtil.java ! src/share/classes/sun/nio/ch/Net.java ! src/share/classes/sun/nio/ch/ServerSocketChannelImpl.java ! src/share/classes/sun/nio/ch/SocketChannelImpl.java ! src/share/classes/sun/nio/ch/Util.java ! src/solaris/classes/sun/nio/ch/DatagramDispatcher.java ! src/solaris/classes/sun/nio/ch/DevPollArrayWrapper.java ! src/solaris/classes/sun/nio/ch/DevPollSelectorImpl.java ! src/solaris/classes/sun/nio/ch/EPoll.java ! src/solaris/classes/sun/nio/ch/EPollArrayWrapper.java ! src/solaris/classes/sun/nio/ch/EPollPort.java ! src/solaris/classes/sun/nio/ch/EPollSelectorImpl.java ! src/solaris/classes/sun/nio/ch/FileDispatcherImpl.java ! src/solaris/classes/sun/nio/ch/InheritedChannel.java ! src/solaris/classes/sun/nio/ch/KQueue.java ! src/solaris/classes/sun/nio/ch/KQueuePort.java ! src/solaris/classes/sun/nio/ch/NativeThread.java ! src/solaris/classes/sun/nio/ch/PollArrayWrapper.java ! src/solaris/classes/sun/nio/ch/SinkChannelImpl.java ! src/solaris/classes/sun/nio/ch/SolarisEventPort.java ! src/solaris/classes/sun/nio/ch/SourceChannelImpl.java ! src/solaris/classes/sun/nio/ch/UnixAsynchronousServerSocketChannelImpl.java ! src/solaris/classes/sun/nio/ch/UnixAsynchronousSocketChannelImpl.java ! src/solaris/classes/sun/nio/ch/sctp/SctpChannelImpl.java ! src/solaris/classes/sun/nio/ch/sctp/SctpMultiChannelImpl.java ! src/solaris/classes/sun/nio/ch/sctp/SctpServerChannelImpl.java ! src/windows/classes/sun/nio/ch/DatagramDispatcher.java ! src/windows/classes/sun/nio/ch/FileDispatcherImpl.java ! src/windows/classes/sun/nio/ch/FileKey.java ! src/windows/classes/sun/nio/ch/Iocp.java ! src/windows/classes/sun/nio/ch/PipeImpl.java ! src/windows/classes/sun/nio/ch/SocketDispatcher.java ! src/windows/classes/sun/nio/ch/WindowsAsynchronousFileChannelImpl.java ! src/windows/classes/sun/nio/ch/WindowsAsynchronousServerSocketChannelImpl.java ! src/windows/classes/sun/nio/ch/WindowsAsynchronousSocketChannelImpl.java ! src/windows/classes/sun/nio/ch/WindowsSelectorImpl.java
hg: jdk8/tl/jdk: 8023717: (process) ProcessBuilder should catch SecurityException rather than AccessControlException
Changeset: 2efa310226f7 Author:alanb Date: 2013-08-28 14:07 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2efa310226f7 8023717: (process) ProcessBuilder should catch SecurityException rather than AccessControlException Reviewed-by: wetmore, alanb Contributed-by: marti...@google.com ! src/share/classes/java/lang/ProcessBuilder.java
Re: [JDK 8] Code review request 7188657, There should be a way to reorder the JSSE ciphers
On 8/28/2013 5:57 PM, Florian Weimer wrote: > On 08/28/2013 11:02 AM, Xuelei Fan wrote: >> Hi, >> >> Please review this update to support cipher suites reorder: >> >> webrev: http://cr.openjdk.java.net/~xuelei/7188657/webrev.00/ >> >> Two new methods are added to SSLParameters: >> public final void setUseCipherSuitesOrder(boolean honorOrder); >> public final boolean getUseCipherSuitesOrder(); >> >> If SSLParameters.getUseCipherSuitesOrder() return true, the local cipher >> suites order returned in SSLParameters.getCipherSuites() should be >> honored during SSL/TLS handshaking. > > The documentation should say this parameter only applies to the server > side because that's the party that picks the cipher suite. > It is the initial motivation to update the behavior of server cipher suite selection. However, we noted that we never specify the ordering of cipher suites in ClientHello message. Although Oracle provider honor the order of SSLParameters.getCipherSuites() for year, but we never say how actually do it. It's good time to specify the ordering in client side also in this update. This API will not impact client behavior of Oracle provider. However, it can be an instinctive guide for third party's provider implementation, and a clear spec for application to enforce the cipher suites ordering. > I wonder if an enum (with members LOCAL and PEER, and perhaps > UNSPECIFIED) would be more appropriate than a boolean flag. I understand your concerns. It's pretty confusing when one think SSLParameters in both client and server sides. The confusing happens not only on this pair of methods, but also on some old methods, for example s/getProtocols(). But I think if we think the method from one side, client or server, each time, the meaning may be easy to understand. In client side, setUseCipherSuitesOrder() means to honor the local/client cipher suites order; In server side, setUseCipherSuitesOrder() means to honor the local/server cipher suites order. Per your suggestion, as PEER cannot apply to client side, it might be a little confusing for client side application developers. Thanks for the support! Regards, Xuelei
Re: [JDK 8] Code review request 7188657, There should be a way to reorder the JSSE ciphers
On 08/28/2013 11:02 AM, Xuelei Fan wrote: Hi, Please review this update to support cipher suites reorder: webrev: http://cr.openjdk.java.net/~xuelei/7188657/webrev.00/ Two new methods are added to SSLParameters: public final void setUseCipherSuitesOrder(boolean honorOrder); public final boolean getUseCipherSuitesOrder(); If SSLParameters.getUseCipherSuitesOrder() return true, the local cipher suites order returned in SSLParameters.getCipherSuites() should be honored during SSL/TLS handshaking. The documentation should say this parameter only applies to the server side because that's the party that picks the cipher suite. I wonder if an enum (with members LOCAL and PEER, and perhaps UNSPECIFIED) would be more appropriate than a boolean flag. -- Florian Weimer / Red Hat Product Security Team
[JDK 8] Code review request 7188657, There should be a way to reorder the JSSE ciphers
Hi, Please review this update to support cipher suites reorder: webrev: http://cr.openjdk.java.net/~xuelei/7188657/webrev.00/ Two new methods are added to SSLParameters: public final void setUseCipherSuitesOrder(boolean honorOrder); public final boolean getUseCipherSuitesOrder(); If SSLParameters.getUseCipherSuitesOrder() return true, the local cipher suites order returned in SSLParameters.getCipherSuites() should be honored during SSL/TLS handshaking. Considering the potential compatibility issues of third party's implementation, I won't define the behaviors if SSLParameters.getUseCipherSuitesOrder() return false. For Oracle provider, SunJSSE, if getUseCipherSuitesOrder() returns false, the order of SSLParameters.getCipherSuites() is honored in client side, and the order of the requested cipher suites in client handshake message is honored in server side. Thanks, Xuelei
