TLS ALPN Proposal v3
Greetings,
Xuelei wrote:
I think Brad would consider our information for his design.
I did, and thanks for the all of the detailed discussion,
Simone/Michael/Xuelei. I've taken into account the feedback from the
previous discussion back in June. Here is v3 of the public ALPN API.
http://cr.openjdk.java.net/~wetmore/8051498/webrev.04/
Main points:
1. SSLBase/SSLFunction gone
2. New ApplicationProtocolSelector class with two select methods:
select(SSLSocket), select(SSLEngine)
throws SSLHandshakeException instead
of SSLProtocolException (oops/sorry!)
3. @since 1.9 changes to @since 9 (changes for JDK 9)
4. SSLSession.getCipherSuite()
a getHandshakeSession.getCipherSuite() no longer dynamic value
ALPN Selector will be called after suite has been set.
5. Psuedo code in the SunJSSEimplementation for ALPN selection.
6. Working HTTP 2 1.1 client/server configuration example (test).
Various responses, I'll try to attribute the original author correctly.
:) If I missed a point you feel is important, please restate.
Simone wrote:
ExtendedSSLSession
public ListString getReceivedApplicationProtocols() {
This is a constant once the application protocols are received, so it
can be surely retrieved from SSLParameters.
I don't understand why it is replicated here ?
SSLParameters is a configuration class which is used to configure
SSLSockets/SSLEngines. SSLSession/ExtendedSSLSession is a class which
holds negotiated Session values. getReceivedApplicationProtocols()
represents the Application Protocol values received from the peer, thus
belongs in the SSLSession.
One other point that is not always obvious to folks is that a single
SSLParameters object can be used to initialize multiple
SSLEngine/SSLSocket objects. You can configure sockets as part of a
customer SSLSocketFactory, or by using SSLParameters.
Regarding the old SSLFunction/SSLBase:
I'm not sure about this one being a marker interface.
I could understand if it extracted the common functionality of
SSLEngine and SSLSocket, but a marker interface does not really add
much, and perhaps I would prefer it entirely gone.
Previously it was suggested that the ALPN selector could be a
@FunctionalInterface (Simone?) and generic (Sean?), so I was trying to
accommodate that. Both are gone now. We could still introduce a
SSLBase with these methods:
---begin---
public abstract String[] getEnabledCipherSuites()
public abstract void setEnabledCipherSuites(String[] suites)
public abstract String[] getEnabledProtocols()
public abstract void setEnabledProtocols(String[] protocols)
public abstract boolean getEnableSessionCreation()
public abstract void setEnableSessionCreation(boolean flag)
public SSLSession getHandshakeSession()
minor tweak for SSLSocket about getSession()
public abstract void setNeedClientAuth(boolean need)
public abstract boolean getNeedClientAuth()
public abstract SSLSession getSession()
minor tweak needed in SSLSocket
public abstract String[] getSupportedCipherSuites()
public abstract String[] getSupportedProtocols()
public abstract boolean getUseClientMode()
public abstract void setUseClientMode(boolean mode)
public abstract boolean getWantClientAuth()
public abstract void setWantClientAuth(boolean want)
public SSLParameters getSSLParameters()
public void setSSLParameters(SSLParameters params)
---end---
which would eliminate the SSLEngine/SSLSocket-specific methods in
ApplicationProtocolSelector.java, but I'm not sure it's worth the effort
to isolate all these methods just to lose one method in the ALPN selector.
To do this as a @FunctionInterface, I wanted to use
java.util.function.Function, but needed it to return a checked
Exception. Anyway, it's gone for now.
On the same note, why is SSLFunction generic at all ?
This was an internal idea that in the future there might be additional
SSL functions we could do as lambdas.
Xuelei/Simone wrote:
Per my understanding, application protocol should be negotiated before
cipher suite and protocol version negotiated.
This is not possible for HTTP/2.
Application protocol negotiation MUST happen *after* the TLS protocol
and the TLS cipher are negotiated.
Yes, that's my understanding as well.
Simone wrote:
Currently, IIUC, the cipher selection is an iterative process where a
cipher is attempted until one is negotiated.
Yes.
Simone wrote:
Yesterday a browser could open a page and browse the site over
http/1.1.
From my readings of the RFC, I agree with Simone and think the intent
of the RFC writers was that if a sufficient connection state does not
exist for HTTP/2, then it should be possible to fallback to something
else instead of killing the connection. If the implementation wants to
insist on HTTP/2 only, the ALPN selector can certainly enforce that, but
it needs to try the H2 ciphersuites first.
With this API, we can do either style.
Re: TLS ALPN Proposal v2
a Standard Name in our normal sense/usage. Usually
it's a mapping from a name string to some type of value (e.g. TLSv1 -
0x0301, SSL_RSA_WITH_3DES_EDE_CBC_SHA - 0x000a, SHA1withRSA -
1.2.840.113549.1.1.5. In this case, it's the actual value, and will
depends on the decoding.
src/java.base/share/classes/javax/net/ssl/SSLSession.java
=
String getCipherSuite()
---
Pretty hard to use this method with the new specification. It's not a
necessary update, see #C9.
I'll come back to this as per above.
Brad
Hope it helps!
Xuelei
On 6/3/2015 8:56 AM, Bradford Wetmore wrote:
Hi,
I have just posted the second iteration of the ALPN proposal which
hopefully addresses all of the comments raised here. It is in javadoc
format, but things can certainly be adjusted:
http://cr.openjdk.java.net/~wetmore/8051498/webrev.00/
Please see the archive [1] for previous design discussion. I will be
writing up usage examples in the next few days.
The significant changes:
ExtendedSSLSession
public ListString getReceivedApplicationProtocols() {
This will allow applications to examine which protocol names were
sent.
SSLParameters
mentions both ALPN/NPN as possible protocols. I removed the
discussion about server and client since ALPN/NPN essentially
reverse the roles.
mentions appropriate character sets for String-byte[] conversions
such as UTF-8 for ALPN.
The application protocol selector is now a @FunctionalInterface
(i.e. lambda-ready) called SSLFunction. It is to throw an
SSLException if no values are supported, or null if you want to
treat as an unknown extension.
Defined constants for HTTP/1.1 and HTTP/2.
SSLSession
Called out that getHandshakeSession's ciphersuite may vary until
selected.
SSLBase
A new marker interface that SSLEngine/SSLSocket will implement.
This will allow for a single SSLFunction instead of having
SSLFunctionSSLEngine and SSLFunctionSSLSocket. It does require
that the lambda do a instanceof, unless we were to move the common
Socket/Engine APIs into this class.
Thanks,
Brad
[1]
http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012183.html
Re: JDK 9 RFR of JDK-8083436: Doclint regression introduced by JDK-8043758
Or I think you could also just remove the args, since there is only one
compareUnsigned currently. Probably safer to use long, long.
Brad
On 6/3/2015 4:06 PM, Joseph D. Darcy wrote:
Hello,
Please review the patch below to address a recently introduced doclint
regression.
Thanks,
-Joe
diff -r 5f952ade41ff
src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java
--- a/src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java Wed
Jun 03 14:35:17 2015 -0700
+++ b/src/java.base/share/classes/javax/net/ssl/SSLEngineResult.java Wed
Jun 03 16:04:22 2015 -0700
@@ -280,7 +280,7 @@
* @apiNote Note that sequence number is an unsigned long and cannot
* exceed {@code -1L}. It is desired to use the unsigned
* long comparing mode for comparison of unsigned long
values
- * (see also {@link java.lang.Long#compareUnsigned()
+ * (see also {@link java.lang.Long#compareUnsigned(long,
long)
* Long.compareUnsigned()}).
* P
* For DTLS protocols, the first 16 bits of the sequence
@@ -300,7 +300,7 @@
* record; or ${@code -1L} if no record is produced or
consumed,
* or this operation is not supported by the underlying
provider
*
- * @see java.lang.Long#compareUnsigned(boolean, boolean)
+ * @see java.lang.Long#compareUnsigned(long, long)
*
* @since 1.9
*/
TLS ALPN Proposal v2
Hi,
I have just posted the second iteration of the ALPN proposal which
hopefully addresses all of the comments raised here. It is in javadoc
format, but things can certainly be adjusted:
http://cr.openjdk.java.net/~wetmore/8051498/webrev.00/
Please see the archive [1] for previous design discussion. I will be
writing up usage examples in the next few days.
The significant changes:
ExtendedSSLSession
public ListString getReceivedApplicationProtocols() {
This will allow applications to examine which protocol names were
sent.
SSLParameters
mentions both ALPN/NPN as possible protocols. I removed the
discussion about server and client since ALPN/NPN essentially
reverse the roles.
mentions appropriate character sets for String-byte[] conversions
such as UTF-8 for ALPN.
The application protocol selector is now a @FunctionalInterface
(i.e. lambda-ready) called SSLFunction. It is to throw an
SSLException if no values are supported, or null if you want to
treat as an unknown extension.
Defined constants for HTTP/1.1 and HTTP/2.
SSLSession
Called out that getHandshakeSession's ciphersuite may vary until
selected.
SSLBase
A new marker interface that SSLEngine/SSLSocket will implement.
This will allow for a single SSLFunction instead of having
SSLFunctionSSLEngine and SSLFunctionSSLSocket. It does require
that the lambda do a instanceof, unless we were to move the common
Socket/Engine APIs into this class.
Thanks,
Brad
[1] http://mail.openjdk.java.net/pipermail/security-dev/2015-May/012183.html
Re: TLS ALPN Proposal
Simone,
I'm sorry for the delay in responding, I've been getting familiar with
lambdas the last couple days, and how we might be able to apply it to
the ALPNSelector code. Interesting stuff.
To the question in this email. I'll leave the previous discussion for
context. See my responses inline.
On 5/27/2015 4:47 AM, Simone Bordet wrote:
Hi,
On Tue, May 26, 2015 at 8:46 PM, Bradford Wetmore
bradford.wetm...@oracle.com wrote:
I am not sure I follow this. Can you please sketch the steps/algorithm
that will be done in your proposed solution ?
I'm assuming you are talking about 1a and not 1b.
Sure, most of the heavy lifting takes place in ServerHandshaker.
ServerHandshaker.java
=
In the SunJSSE code:
// currently line 330.
private void clientHello(ClientHello mesg) throws IOException {
// Was an ALPNExtension received?
ALPNExtension alpnExt = (ALPNExtension)
mesg.extensions.get(ExtensionTyp.EXT_ALPN);
...
// Currently line 706 in JDK9.
session = new SSLSessionImpl(protocolVersion,
CipherSuite.C_NULL,
getLocalSupportedSignAlgs(),
sslContext.getSecureRandom(),
getHostAddressSE(), getPortSE());
...
session.setALPNNames(alpnExt.getNames());
...
// choose cipher suite and corresponding private key
// This function is at 987 currently.
chooseCipherSuite(mesg);
...
// Only adds an ALPN extension if non-empty.
m1.extensions.add(applicationProtocolSelector.select(...));
...
Above, chooseCipherSuite() iterates through the collection of suites. Inside
that, trySetCipherSuite() attempts to verify that the suite is acceptable to
use. Inside that, setupPrivateKeyAndChain() does the actual KeyManager
calls.
if (conn != null) {
alias = km.chooseServerAlias(algorithm, null, conn);
} else {
alias = km.chooseEngineServerAlias(algorithm, null, engine);
}
Now in the Application's Code:
If you want the KeyManager to take this info into account, you need to
create your own customer KM.
public String chooseEngineServerAlias(String keyType,
Principal[] issuers,
SSLEngine engine) {
ExtendedSSLSession session = engine.getHandshakeSession();
String protocol = session.getProtocol();
String ciphersuite = session.getCipherSuite();
ListString alpns =
session.getRequestedApplicationProtocolNames();
// Some logic for which key to use...
return choose(protocol, ciphersuite, alpns);
}
And then your actual ALPN selector:
public String select(...) throws SSLProtocolException {
ExtendedSSLSession session = engine.getHandshakeSession()
String ciphersuite = session.getCipherSuite();
ListString alpns =
session.getRequestedApplicationProtocolNames();
// Some logic for which key to use...
return choose(protocol, ciphersuite, alpns);
}
Hopefully that is clear.
Let me see if I understand correctly.
In chooseEngineServerAlias() I will have a proposed cipher and the
list of app protos from the client.
The goal would be to choose the alias based on the app proto, as stated by 7301.
However, the app proto is not yet chosen here. If I don't have other
constraints to choose the app proto (e.g. SNI), the usual algorithm
applies: pick the first server app proto that is in common with the
client.
Let's assume this is h2; but looking at the cipher, it's not good, so
we have to pick another app proto, say http/1.1. Now the cipher is
good and we return the alias.
This is similar to what happens now with Jetty's ALPN: the cipher will
be chosen, then the ALPN callback invoked and there, by looking at the
cipher, we know we have to use http/1.1.
Let's assume now I have the constraint to choose h2 (e.g. I have a SNI
of http2.domain.com).
In chooseEngineServerAlias() I will look at SNI, and therefore choose
h2, then look at the proposed cipher, which is not good, so return
null.
Method chooseEngineServerAlias() will be called repeatedly for other
ciphers, until a cipher good for h2 is found, otherwise it's an alert
120.
Correct. To expand a bit...
You will have the SSLSocket/SSLEngine which will give you access to the
Socket's attributes (e.g. local/remote IP address/ports), along with the
handshakeSession which is being negotiated. The handshakeSession will
give access to the selected TLS protocol version number plus the
received SNI and ALPN names. With my change to the ciphersuites, it
will now give you to the proposed ciphersuite, the one being probed
for valid key material. If your custom KeyManager don't like this
combination of protocol/ciphersuite/sni/alpn/Socket attributes, the
KeyManager can report back no valid key material is available, thus
Re: TLS ALPN Proposal
I am not sure I follow this. Can you please sketch the steps/algorithm
that will be done in your proposed solution ?
I'm assuming you are talking about 1a and not 1b.
Sure, most of the heavy lifting takes place in ServerHandshaker.
ServerHandshaker.java
=
In the SunJSSE code:
// currently line 330.
private void clientHello(ClientHello mesg) throws IOException {
// Was an ALPNExtension received?
ALPNExtension alpnExt = (ALPNExtension)
mesg.extensions.get(ExtensionTyp.EXT_ALPN);
...
// Currently line 706 in JDK9.
session = new SSLSessionImpl(protocolVersion,
CipherSuite.C_NULL,
getLocalSupportedSignAlgs(),
sslContext.getSecureRandom(),
getHostAddressSE(), getPortSE());
...
session.setALPNNames(alpnExt.getNames());
...
// choose cipher suite and corresponding private key
// This function is at 987 currently.
chooseCipherSuite(mesg);
...
// Only adds an ALPN extension if non-empty.
m1.extensions.add(applicationProtocolSelector.select(...));
...
Above, chooseCipherSuite() iterates through the collection of suites.
Inside that, trySetCipherSuite() attempts to verify that the suite is
acceptable to use. Inside that, setupPrivateKeyAndChain() does the
actual KeyManager calls.
if (conn != null) {
alias = km.chooseServerAlias(algorithm, null, conn);
} else {
alias = km.chooseEngineServerAlias(algorithm, null, engine);
}
Now in the Application's Code:
If you want the KeyManager to take this info into account, you need to
create your own customer KM.
public String chooseEngineServerAlias(String keyType,
Principal[] issuers,
SSLEngine engine) {
ExtendedSSLSession session = engine.getHandshakeSession();
String protocol = session.getProtocol();
String ciphersuite = session.getCipherSuite();
ListString alpns =
session.getRequestedApplicationProtocolNames();
// Some logic for which key to use...
return choose(protocol, ciphersuite, alpns);
}
And then your actual ALPN selector:
public String select(...) throws SSLProtocolException {
ExtendedSSLSession session = engine.getHandshakeSession()
String ciphersuite = session.getCipherSuite();
ListString alpns =
session.getRequestedApplicationProtocolNames();
// Some logic for which key to use...
return choose(protocol, ciphersuite, alpns);
}
Hopefully that is clear.
Brad
On 5/26/2015 1:00 AM, Simone Bordet wrote:
Hi,
On Tue, May 26, 2015 at 2:30 AM, Bradford Wetmore
bradford.wetm...@oracle.com wrote:
Darn those Chicken/Eggs [1]!
Yes, you are correct. The steps for the current server code:
1. The ClientHello is parsed, and the SNI matcher callback is called. It
does not return which value was matched in the ServerHello, just whether a
SNI name was matched or not:
The extension_data field of this extension SHALL be
empty.
2. Begin generating the ServerHello, choose the Protocol Version.
3. Iterate through the list of client's ciphersuites and call the Server's
KeyManager (KM) callback until the KM returns key material we can use. A
return string selects the proposed ciphersuite.
So we currently don't know the selected ciphersuite until the KM has been
called (possibly multiple times).
If we choose the ALPN before the ciphersuite, the ciphersuite selection may
end up being inappropriate (HTTP/2 blacklist). If we choose the ciphersuite
first, then the ALPN value wasn't used to drive the certificate selection.
Two suggestions in preferred order below.
In each of these cases, unfortunately there is currently no indication of
the proposed Ciphersuite, so we need to modify the behavior of
getHandshakeSession().getCipherSuite() to fill in the proposed CipherSuite
before the call to the KM. This seems ok with the current wording, but we'd
need to make that explicit. This value will change for each ciphersuite/KM
choice attempt.
Each suggestion below is followed by our previously proposed ALPN callback
to make the actual ALPN value selection:
1a. Add a parallel method to ExtendedSSLSession:
public ListString getRequestedApplicationProtocolNames();
along with the previously proposed selected name:
public String getApplicationProtocol()
(I'll be changing these names. I'm open to suggestions).
When the KM is called, the TLS protocol (e.g. TLSv1.2) has already been
selected.
Both of the major selection parameters (protocol/proposed ciphersuite) are
now available, and applications have access to the ordered ALPN list to see
what the client's requested values were.
-or-
1b. Keep API as is, and make two callbacks. This first is an advisory
value, the TLS protocol version and proposed
Re: TLS ALPN Proposal
On 5/22/2015 8:28 PM, Weijun Wang wrote: On 5/23/2015 9:13 AM, Bradford Wetmore wrote: Weijun wrote: But in the RFC the name is in uppercase and chars in string are all lowercases. ...deleted... - Compare with equalsIgnoreCase() Not following here, the spec is specific about the over-the-wire byte values, and http/1.1 != Http/1.1. Because the spec says o Identification Sequence: The precise set of octet values that identifies the protocol. This could be the UTF-8 encoding [RFC3629] of the protocol name. and the name is uppercase. What if someone really sends HTTP/1.1.getBytes(UTF-8)? I'm sorry, but I'm still not understanding your point. Looking at an existing ALPN directory entry: Protocol: HTTP/1.1 Identification Sequence: 0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31 (http/1.1) Reference: [RFC7230] The name of the Protocol is HTTP/1.1, but the Identification Sequence is 0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31 (http/1.1). I am proposing that the ListString be the values of the Identification Sequence, not the IANA Protocol Names. Is your opinion that the ALPN API String Protocol be the Protocol: and that we should internally map from HTTP/1.1 to http/1.1 before sending? Or that Identification Sequence HTTP/1.1 SHOULD BE treated the same as http/1.1? I think that's what you're saying, since I think you want to compare it using equalsIgnoreCase(). That will make future ALPN protocol name addition challenging. What if someone really sends HTTP/1.1.getBytes(UTF-8)? In my proposal, then they should send HTTP/1.1 instead of http/1.1. I'm really sorry if I'm still missing something. Brad
Re: TLS ALPN Proposal
Darn those Chicken/Eggs [1]! Yes, you are correct. The steps for the current server code: 1. The ClientHello is parsed, and the SNI matcher callback is called. It does not return which value was matched in the ServerHello, just whether a SNI name was matched or not: The extension_data field of this extension SHALL be empty. 2. Begin generating the ServerHello, choose the Protocol Version. 3. Iterate through the list of client's ciphersuites and call the Server's KeyManager (KM) callback until the KM returns key material we can use. A return string selects the proposed ciphersuite. So we currently don't know the selected ciphersuite until the KM has been called (possibly multiple times). If we choose the ALPN before the ciphersuite, the ciphersuite selection may end up being inappropriate (HTTP/2 blacklist). If we choose the ciphersuite first, then the ALPN value wasn't used to drive the certificate selection. Two suggestions in preferred order below. In each of these cases, unfortunately there is currently no indication of the proposed Ciphersuite, so we need to modify the behavior of getHandshakeSession().getCipherSuite() to fill in the proposed CipherSuite before the call to the KM. This seems ok with the current wording, but we'd need to make that explicit. This value will change for each ciphersuite/KM choice attempt. Each suggestion below is followed by our previously proposed ALPN callback to make the actual ALPN value selection: 1a. Add a parallel method to ExtendedSSLSession: public ListString getRequestedApplicationProtocolNames(); along with the previously proposed selected name: public String getApplicationProtocol() (I'll be changing these names. I'm open to suggestions). When the KM is called, the TLS protocol (e.g. TLSv1.2) has already been selected. Both of the major selection parameters (protocol/proposed ciphersuite) are now available, and applications have access to the ordered ALPN list to see what the client's requested values were. -or- 1b. Keep API as is, and make two callbacks. This first is an advisory value, the TLS protocol version and proposed ciphersuite will be available in getHandshakeSession(). The second callback sets the final value that will be sent. I think 1.a is my preference. To answer some of the other questions. On 5/25/2015 3:08 AM, Michael McMahon wrote: 2) The notion of client preference needs to be made explicit. This could just be a matter of javadoc given that ListString is ordered. So, it could be enough to say the same order is used in the protocol. Yes, I'll add that. 3) It's a shame that the RFC didn't mandate UTF8 encoded byte sequences for the protocol name, because it's theoretically possible that non UTF8 byte sequences could get registered, but that's not a concern for HTTP/2 at least. No. Not sure what we can do about that, short of going back to the byte[] option. Given that IANA operates mainly in English, I would expect the namespaces will probably be ASCII, but that is just conjecture. This would be possible, IIUC, using sslEngine.getHandshakeSession().getRequestedServerNames() in the ApplicationProtocolSelector implementation. Yes. but I understand it's mentioned in RFC 7301. Yes, see the last sentence section 1. Brad [1] https://www.youtube.com/watch?v=ixgf5SlvOB4feature=youtu.bet=27
Re: TLS ALPN Proposal
Thanks for the thorough reviews and comments, I really appreciate it and
always learn something. FunctionalInterface (@since 1.8) is something I
haven't really explored yet, so off to the books.
I'm glad this ALPN approach seems worth pursing. I have several
different comments I'll combine into this single message.
On 5/22/2015 9:12 AM, Simone Bordet wrote:
ExtendedSSLSession:
---
gets the negotiated protocol String
If I understand correctly, SSLParameters will have methods that can
only be used by one side of the TLS protocol (e.g. client or server,
but not both).
It's already like this for other things (e.g. SNI) so it will work,
but I had hoped for some kind of reworking in this area too, but I'm
digressing.
I don't have a better idea/suggestion for this, protocol advertisement
and selection are two very different things. At this point, I am
thinking it should remain in SSLParameters.
I considered putting the selector into the SSLContext initialization:
sslContext.init(KeyManager, TrustManager, SecureRandom, Selector);
but that means 1 selector for all SSLSockets/SSLEngines that get
generated from that SSLContext. That is constricting if the selector
wanted to keep any connection-specific values. It could go into
SSLSocket/SSLEngine, but starting in JDK 6, we've been lumping all set*
configuration parameters into SSLParameters, so that once configured,
devs can reuse one SSLParameters object without having to reconstruct it
(source code or at runtime) from scratch.
BTW, there was a previous question about when to use SSLSocket.set*()
and sslSocket.setSSLParameters(params). I gave a partial answer in:
http://mail.openjdk.java.net/pipermail/security-dev/2014-November/011430.html
I recently filed:
https://bugs.openjdk.java.net/browse/JDK-8080799
Provide guidance on repeated configuration parameter APIs
to address this. I don't think we need to deprecate the set() methods,
but having a note here will alleviate confusion.
/*
* A callback class on the server side that is responsible for
* choosing which Application Protocol should be used in a SSL/TLS
* connection.
*/
public abstract class ApplicationProtocolSelector {
/*
* SSLSocket callback to choose which Application Protocol value
* to return to a SSL/TLS client
*
* @param sslSocket the SSLSocket for this connection
* @param protocols the list of protocols advertised by the client
* @param protocolVersion the negotiated protocol version
Egads, this is confusing. protocols is the ALPN protocols,
protocolVersion is the TLS version number. I'll fix this.
* @param ciphersuite the negotiated ciphersuite
* @return a non-empty protocol String to send to the client,
* or null if no protocol value (i.e. extension) should be sent.
* @throws SSLProtocolException if the connection should be aborted
* because the the server supports none of the protocols that
* the client advertised.
Whoops, I put exactly the Exception I didn't want to use! I was
originally thinking SSLHandshakeException. See below for more comments.
*/
public String select(SSLSocket sslSocket, String[] protocols,
String protocolVersion, String ciphersuite)
throws SSLProtocolException;
We are currently getting the protocolVersion and the cipherSuite via:
[sslSocket|sslEngine].getHandshakeSession().get[Protocol|CipherSuite]()
If this is correct, perhaps there is no need to pass those 2
parameters to select() ?
Good point. Yes, those are no longer needed.
There was a suggestion to have access to the clientHello information in
order to guide ciphersuite/protocol selection, however, there isn't a
way to control that part of the internals at this time, so I didn't
include it.
As an aside, if we do develop the Handshake API (other mail) and allow
for handshake message modification (not currently proposed, but hinted
at), the protocols/ciphersuites values could be adjusted before they are
handled. e.g. INBOUND callbacks would be triggered after the message
has been parsed and added to the handshake hash calculation, but before
clientHello() is actually called.
I would suggest that if SSLParameters.setApplicationProtocols() takes
a ListString, then also select() should take a ListString, rather
than a String[].
Since it's the server picking the protocols, would be handy to have
the client protocols in a ListString in order to call contains(),
etc. on it.
Good points.
I would suggest to throw SSLException rather than the too specific
SSLProtocolException (which may also be misleading, since its javadoc
hints at a flaw in one of the protocol implementations, while in
this case it is just a failure to negotiate an *application* protocol
- perfectly fine from the point of view of the TLS protocol).
See above. I'll change it to SSLException for now, but I think it
TLS Handshake Message Proposal (Was: Re: JEP 244: TLS Application-Layer Protocol Negotiation Extension)
Hi Thomas,
After reviewing a lot of the back mail and the desires expressed, I have
two orthogonal proposals to make.
The first (next email) is an ALPN-specific API using a simple callback
selector which I think addresses most of the protocol selection concerns.
The second (below) is a more general Handshake Message framework that
will allow for insertion of other HandshakeMessage types (e.g.
SupplementalData/NewSessionTicket), packet capture (e.g. Fallback
Signaling CipherSuite/Channel Bindings), and for
adding/modifying/deleting TLS Extensions (both known/unknown). I was
trying to come up with something less-involved for just the
Hellos/Extensions, but it became clear that anything for those messages
could easily be extended to all. There's a lot of new APIs, but I find
it pretty straightforward to use. Sample code is below.
These two approaches are complementary: they don't depend on each other
at all. Given that we need to have ALPN support in just over a month
(to allow for test/JCK/HTTP-2 development), my colleagues are concerned
about taking on major API development at this date. Depending on
feedback, I'm thinking of proposing that we do the first approach for
JDK 9, and for the second, initiate a JEP and target the JDK 10 timeframe.
On 5/20/2015 12:15 PM, Thomas Lußnig wrote:
Hi,
1) There are two types of extensions:
a) That modify the directly how the engine works like
[max_fragment_length,heartbeat,encrypt_then_mac,extended_master_secret,SessionTicket,...]
As a third party, these will be impossible without making your own JDK
(or it being supported by the implementation).
b) That provide information (modify the network protocol) like
[npn,alpn,status_request,...]
Yes. I tried to call those out in my email.
2) Some of the extionsions could be called deprecated like heartbeat,
npn and compression
NPN/compression certainly, but I wasn't aware heartbeat was deprecated.
Possibly in the court of public opinion after Heartbleed. :) Is it
really deprecated in the wider TLS community?
signed_certificate_timestamp - could be done without ocsp interference
via extra handshake message like you can see it on https://suche.org
there are 3 ways
how this can be archived Included in Certificate, OCSP-Response, Extra
handshake Message.
extended_master_secret - would be hard to implement.
There are two ways to enable better plugin/develop:
+ Expose the client handshake to KeyManager/TrustManager/Client/Server
+ Generic way to add extra messages [status_request, user_mapping,
client_authz, server_authz, application_layer_protocol_negotiation,
status_request_v2, signed_certificate_timestamp,
npn,
TLS_FALLBACK_SCSV
Before I describe the approach, in recent discussions with my
colleagues, they were also concerned that this would require too much
intimate knowledge of the TLS protocol. The other major concern is that
this is fair amount of change to support a small number of situations.
Is it worth the time to work this up if no one will actually use it?
Of course, the big plus is that developers can now add functionality
that we just haven't been able to do. That said, this approach does
give developers full freedom to shoot themselves in the foot if they get
the messages (format/values) wrong. ;) We'll do what we can in
creating APIs for creating valid messages, and leave it to developers to
create the rest.
Here's the approach. This is just a first draft, lots of work to be
done here.
To provide access to the handshake, developers provide callbacks for
well-defined handshake points:
SSLSocket.setCallbacks(CallBack[] cbs)
SSLEngine.setCallbacks(CallBack[] cbs)
with:
CallBack(
int handshakeMessageType,
enum When {INBOUND/OUTBOUND_BEFORE/OUTBOUND_AFTER},
boolean extensions,
CallBackHandler)
where:
handshakeMessageType are the TLS numbers:
hello_request(0), client_hello(1), server_hello(2),
...etc...
finished(20), change_cipher_spec(254), (255)
Note: CCS is not a formal handshake message, but
is part of the handshake.
INBOUND message is from peer, called upon receipt
OUTBOUND_BEFORE message is for peer, called before generation
OUTBOUND_AFTER message is for peer, called after generation
message is also available for review
extensions if true and handshakeMessageType is
client_hello/server_hello, any bytes returned from the
handlers (see below) will be added as extensions.
If false or any other message type, any bytes are added as a
separate message (e.g SupplementalData for user_mapping)
If you duplicate an extension, it will replace the existing
one in the output message.
The CallBackHandler has:
byte[] callBackHandler.handle(
SSLSocket s, int
TLS ALPN Proposal
This is a fork of the previous thread:
Subject: TLS Handshake Message Proposal
(Was: Re: JEP 244: TLS Application-Layer Protocol
Negotiation Extension)
I broke this thread off to keep this proposal discussion together, but
if you're interested in the history, please see the previous thread.
This approach combines different suggestions from security-dev in a new
way, and simplifies the ALPN selection process quite a bit. I think it
addresses most of the concerns about selection that were raised.
This approach can be completely separated from the Handshake mechanism I
discussed in the previous message, so I'm thinking of this approach for
JDK 9 and targeting the more general handshake approach for JDK 10.
Here's the summary of API additions:
SSLParameters:
--
Client-side: gets/sets the list of protocol names to send.
Server-side: gets/sets a ApplicationProtocolSelector which is a
user-defined callback mechanism.
ApplicationProtocolSelector:
Server-side: callback methods for SSLSocket/SSLEngine which
are provided with handshake information for making
the selection
ExtendedSSLSession:
---
gets the negotiated protocol String
Specifics below. The javadoc obviously needs work.
class SSLParameters {
...deleted...
/**
* Gets the list of application protocols that will sent by
* the client.
*
* The list could be empty, in which case no protocols will be
* sent.
*
* @returns a list of application protocol names.
*/
public ListString getApplicationProtocols() { };
/**
* Sets the list of application protocols that will sent by
* the client.
*
* protocols could be empty, in which case no protocols will be
* sent.
*
* @param protocols a list of application protocol names
* @throws IllegalArgumentException if protocols is null.
*/
public void setApplicationProtocols(ListString protocols);
/**
* Gets the current server-side application layer protocol selector.
*
* @param the selector mechanism.
* @return the current application protocol selector, or null if
* there is not one.
*/
public ApplicationProtocolSelector
getApplicationProtocolSelector() {};
/**
* Sets the server-side application layer protocol selector.
*
* @param the selector mechanism, or null if protocol selection
* should not be done.
*/
public void setApplicationProtocolSelector(
ApplicationProtocolSelector selector) {};
}
/*
* A callback class on the server side that is responsible for
* choosing which Application Protocol should be used in a SSL/TLS
* connection.
*/
public abstract class ApplicationProtocolSelector {
/*
* SSLSocket callback to choose which Application Protocol value
* to return to a SSL/TLS client
*
* @param sslSocket the SSLSocket for this connection
* @param protocols the list of protocols advertised by the client
* @param protocolVersion the negotiated protocol version
* @param ciphersuite the negotiated ciphersuite
* @return a non-empty protocol String to send to the client,
* or null if no protocol value (i.e. extension) should be sent.
* @throws SSLProtocolException if the connection should be aborted
* because the the server supports none of the protocols that
* the client advertised.
*/
public String select(SSLSocket sslSocket, String[] protocols,
String protocolVersion, String ciphersuite)
throws SSLProtocolException;
/*
* SSLEngine callback to choose which Application Protocol to return
* to a SSL/TLS client.
*
* @param sslEngine the SSLEngine for this connection
* @param protocols the list of protocols advertised by the client
* @param protocolVersion the negotiated protocol version
* @param ciphersuite the negotiated ciphersuite
* @return a non-empty protocol String to send to the client,
* or null if no protocol value should be sent.
* @throws SSLProtocolException if the connection should be aborted
* because the the server supports none of the protocols that
* the client advertised.
*/
public String select(SSLEngine sslEngine, String[] protocols,
String protocolVersion, String ciphersuite)
throws SSLProtocolException;
}
If need be, we could include the received extensions or in a future
version of this class.
public class ExtendedSSLSession implements SSLSession {
...deleted...
/**
* Gets the application protocol negotiated for this connection.
*
* @returns the application protocol name or null if none was
* negotiated.
*/
public String getApplicationProtocol() { };
}
There
Re: JEP 244: TLS Application-Layer Protocol Negotiation Extension
Hi Simone/Thomas/David/others, Thomas wrote: would it not be an great idea to combine all these new extensions to an generic way how to handle the SSL Protocol Handshake ? I've finally been able to deep dive back into ALPN, specifically looking at the ideas of a general Hello extension framework, but also a more general handshaking framework that could support additional/future handshake message types (e.g. NPN/CertificateStatus/SupplementalData/SessionTicket). After reading lots of current/proposed/private TLS extension RFCs[1][4], I concur with much of what Xuelei wrote previously (see below). I also agree that having a Hello+extension explorer/modifier would have some utility for ALPN and a few others (including TLS_FALLBACK_SCSV), but many of the current extensions require modifications to the protocol/calculations and/or the KeyManager/TrustManagers and thus can't simply be bolted into SunJSSE using the current APIs/implementations. In most cases, it requires modification of the underlying SunJSSE implementation to support them, or else a strong working knowledge of the TLS protocol to make the right insertions in the right place. I have varying degrees of familiarity with these extensions, so I may have missed an obvious way to implement something and am certainly open to suggestions, but here's the current extension list and my notes: # Extension Name Reference = == = 0 server_name[RFC6066] Currently supported 1 max_fragment_length[RFC6066] No: Protocol I/O modifications needed to generate proper sized packets. 2 client_certificate_url [RFC6066] No: Requires new KM/TM API style that knows how to fetch URLs 3 trusted_ca_keys[RFC6066] Yes: pretty straightforward 4 truncated_hmac [RFC6066] No: Protocol modifications needed 5 status_request [RFC6066] Currently under development for JDK 9[2] 6 user_mapping [RFC4681] Possible: need to insert Supplemental Data handshake message type into right place in handshake, but probably not too difficult. 7 client_authz [RFC5878] 8 server_authz [RFC5878] Possible: also must insert Supplemental Data into the right places in handshake, but probably not too difficult. 9 cert_type [RFC6091] No: Currently only support X509KM/X509TM, not OpenPGP credentials. 10elliptic_curves[RFC4492] 11ec_point_formats [RFC4492] Currently supported 12srp[RFC5054] No: Requires new ciphersuites and ClientKeyExchange format support. 13signature_algorithms [RFC5246] Currently supported 14use_srtp [RFC5764] No: APIs do not expose the PRF and TLS master secret mechanisms. Might be able to talk RTP using socket overlays, but the other issues make this impossible. 15heartbeat [RFC6520] No: Don't support heartbeat message type. 16application_layer_protocol_negotiation [RFC7301] Currently under development for JDK 9[3] 17status_request_v2 [RFC6961] Currently under development for JDK 9[2] 18signed_certificate_timestamp [RFC6962] No: We might be able to support this extension, but it would be incomplete as OCSP doesn't support 1.3.6.1.4.1.11129.2.4.5. Clients must support X509v3 cert extension, TLS exension, and OCSP: servers only need to support one. 19client_certificate_type[RFC7250] 20server_certificate_type[RFC7250] No: We do support X509 certs, but we don't support Raw Public Keys with our current impl so this would not be a complete implementation. 21padding[draft-ietf-tls-padding] Yes: but would require educated guesses on how much padding to add. 22encrypt_then_mac [RFC7366] No: protocol modifications needed 23extended_master_secret [draft-ietf-tls-session-hash] No: would require changes to the PRF computation parameters. 35SessionTicket TLS [RFC4507] Possible: requires new SessionTicket message be sent in the right place, probably not too difficult. 13172 NPN[4] Possible: requires insertion of a NextProtocol message between CCS and Finished. 65281 renegotiation_info [RFC5746] Currently supported I also looked at extending the handshake examiner for channel bindings using either raw TLS X509Certs or Finished messages. It seems easier to me if developers had 2-4 methods with descriptive names rather than having to use a generic handshake
JEP Review Request: TLS Application-Layer Protocol Negotiation Extension
The draft JEP “TLS Application-Layer Protocol Negotiation Extension” is now available for community review: https://bugs.openjdk.java.net/browse/JDK-8051498 This JEP is to add support for the Application Layer Protocol Negotiation (ALPN) TLS Hello extension [1] in JSSE. ALPN provides a mechanism for declaring the application protocols that are supported over a TLS connection. We need this functionality to make JDK 9, so this JEP needs to get into the JEP pipeline soon. Community review is a precursor in the process before it can move to Submitted. For now, there is a simple API proposed (similar to JDK 8 SNI), but I'm parsing the discussions that took place on security-dev in August[2], September[3], and November 2014[4], and the current API is likely not flexible enough. Thanks, Brad [1] http://www.rfc-editor.org/rfc/rfc7301.txt [2] http://mail.openjdk.java.net/pipermail/security-dev/2014-August/thread.html [3] http://mail.openjdk.java.net/pipermail/security-dev/2014-September/thread.html Subject: TLS extensions API, ALPN and HTTP 2.0 [4] http://mail.openjdk.java.net/pipermail/security-dev/2014-November/thread.html Subject: ALPN API Proposal Subject: A fully fledged TLS Extensions API ? Subject: ALPN HTTP2
Re: Review request 8069551: Move java.security.acl from compact3 to java.base
Looks ok to me too. Brad On 1/30/2015 9:10 AM, Mandy Chung wrote: On 1/30/15 12:50 AM, Alan Bateman wrote: On 29/01/2015 20:58, Mandy Chung wrote: Webrev at: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8069551/webrev.00 java.security.acl is superceded by java.security package since 1.2. This patch proposes to move java.security.acl package to java.base module rather than complicating the module graph with a close-to-useless dedicated module. This looks okay. One small suggestion is to move the lines in unshuffle_list.txt up to where the other java.base paths are. Yes that's better. Fixed. thanks. Mandy
RFR: 8069038: javax/net/ssl/TLS/TLSClientPropertyTest.java needs to be updated for JDK-8061210
This is a P2 test bug impacting continuous integration: http://cr.openjdk.java.net/~wetmore/8069038/webrev.00 A recent change missed updating one of the tests. Pretty straight forward, also corrected one typo. I don't see this test in the JDK 8 workspace, it's apparently only a 9 thing. Brad
Re: RFR: 8069038: javax/net/ssl/TLS/TLSClientPropertyTest.java needs to be updated for JDK-8061210
Xuelei/Sean looked at this already (below). Brad On 1/23/2015 5:04 AM, Sean Mullan wrote: On 01/23/2015 02:21 AM, Bradford Wetmore wrote: JPRT passed. Off to bed. The fix looks fine to me. --Sean On 1/22/2015 10:02 PM, Xuelei Fan wrote: Looks fine to me. Thanks, Xuelei On 1/23/2015 10:28 AM, Bradford Wetmore wrote: This is a P2 test bug impacting continuous integration: http://cr.openjdk.java.net/~wetmore/8069038/webrev.00 A recent change missed updating one of the tests. Pretty straight forward, also corrected one typo. I don't see this test in the JDK 8 workspace, it's apparently only a 9 thing. Brad
Re: RFR 8044860: Vectors and fixed length fields should be verified for allowed sizes
Jamil, MAX_LENGTH probably could have been private, but not a big deal. Nice that it was only SessionID. I did a spot check on the TLS Extensions and TLS1.0-1.2, do you check on other related TLS RFCs? Brad On 1/22/2015 6:27 PM, Xuelei Fan wrote: Looks fine to me. Thanks! Xuelei On 1/23/2015 10:24 AM, Jamil Nimeh wrote: Hi Xuelei, et al.: Updated webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8044860/webrev.02 Thanks, --Jamil On 01/22/2015 04:26 PM, Xuelei Fan wrote: I may use SSLProtocolException if the size of session ID is bigger than 32. Otherwise, looks fine to me. Xuelei On 1/23/2015 2:35 AM, Jamil Nimeh wrote: Hi all, This review is to provide length checks on the session ID for SSL/TLS connections. It appears to be the only vector/array that needs additional length-checks to make sure it's not exceeding 32 bytes. Bug: https://bugs.openjdk.java.net/browse/JDK-8044860 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8044860/webrev.01 Thanks, --Jamil
Re: RFR 8044860: Vectors and fixed length fields should be verified for allowed sizes
Thanks for checking and for getting this bug knocked out. I find http://datatracker.ietf.org/wg/tls/documents/ to be a useful cross checking tool in situations like this. As long as you hit the major ones that we support, I'm happy. Thanks, Brad On 1/22/2015 10:17 PM, Jamil Nimeh wrote: I did check some of the other TLS RFCs, particularly 6066, 6961, 4492, 5288 and a few others. There are so many that I'm not 100% certain I caught them all, but not all apply to JSSE either. In all the RFCs I looked at, those vectors had upper bounds that matched the maximum value for its length field. --Jamil On 01/22/2015 09:57 PM, Bradford Wetmore wrote: Jamil, MAX_LENGTH probably could have been private, but not a big deal. Nice that it was only SessionID. I did a spot check on the TLS Extensions and TLS1.0-1.2, do you check on other related TLS RFCs? Brad On 1/22/2015 6:27 PM, Xuelei Fan wrote: Looks fine to me. Thanks! Xuelei On 1/23/2015 10:24 AM, Jamil Nimeh wrote: Hi Xuelei, et al.: Updated webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8044860/webrev.02 Thanks, --Jamil On 01/22/2015 04:26 PM, Xuelei Fan wrote: I may use SSLProtocolException if the size of session ID is bigger than 32. Otherwise, looks fine to me. Xuelei On 1/23/2015 2:35 AM, Jamil Nimeh wrote: Hi all, This review is to provide length checks on the session ID for SSL/TLS connections. It appears to be the only vector/array that needs additional length-checks to make sure it's not exceeding 32 bytes. Bug: https://bugs.openjdk.java.net/browse/JDK-8044860 Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8044860/webrev.01 Thanks, --Jamil
Re: RFR: JDK-8068748: missing US_export_policy.jar in jdk9-b44 is causing compilation errors building jdk9 source code
Looks good, thanks for fixing this! Brad On 1/15/2015 3:05 AM, Erik Joelsson wrote: Hello, Please review the open part of this patch, which changes the building of policy jars to happen even if BUILD_CRYPTO is false. Previously these weren't built as there were signed versions of these jars, but since we no longer sign them, there is no need to not build them. Bug: https://bugs.openjdk.java.net/browse/JDK-8068748 Webrev: http://cr.openjdk.java.net/~erikj/8068748/webrev.jdk.01/ The webrev diffs look a bit weird. The only thing I did was to remove the ifneq ($(BUILD_CRYPTO), no) and adjust the indentation. /Erik
Re: JDK 9 RFR of JDK-8069127: Suppress deprecation warnings in jdk.deploy.osx module
Looks good. Thanks.
Brad
On 1/15/2015 4:39 PM, joe darcy wrote:
Hello,
Please review these simple changes to address a few stray deprecation
warnings in the jdk.deploy.osx module:
JDK-8069127: Suppress deprecation warnings in jdk.deploy.osx module
http://cr.openjdk.java.net/~darcy/8069127.0/
The patch is
---
old/src/jdk.deploy.osx/macosx/classes/apple/security/KeychainStore.java
2015-01-15 16:36:49.547707664 -0800
+++
new/src/jdk.deploy.osx/macosx/classes/apple/security/KeychainStore.java
2015-01-15 16:36:49.359707672 -0800
@@ -911,6 +911,7 @@
return true;
}
+@SuppressWarnings(deprecation)
private byte[] fetchPrivateKeyFromBag(byte[] privateKeyInfo)
throws IOException, NoSuchAlgorithmException, CertificateException
{
byte[] returnValue = null;
@@ -971,6 +972,7 @@
return returnValue;
}
+@SuppressWarnings(deprecation)
private byte[] extractKeyData(DerInputStream stream)
throws IOException, NoSuchAlgorithmException,
CertificateException
{
Thanks,
-Joe
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
I only looked at test, looks good to me. I'd rarely ask to remove extra prints in tests. It adds initial debugging data points in case something breaks down the road. Brad On 1/4/2015 8:25 AM, Peter Levart wrote: Hi Brad, So I created another webrev (just removed the unneeded import and left-over System.out.println from test): http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.06/ I'll leave it here to rest for a couple of days and if no one objects, I'll push it to jdk9-dev. Thanks everybody for reviews and happy new year! Regards, Peter On 01/02/2015 11:58 PM, Bradford Wetmore wrote: On 1/1/2015 12:22 PM, Peter Levart wrote: Hi Brad, Here's next webrev which tries to cover all your comments: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.04/ I downloaded the webrev.05 (Chris' later followup email) and ran it through JPRT. The only error was the previously seen -Dseed which is not your problem. Again, I only ran: jdk_lang, jdk_math, jdk_util, jdk_io, jdk_net, jdk_nio, jdk_security*, jdk_rmi, jdk_text, jdk_time, jdk_other, core_tools. If you need anything else run, let me know. Looks like you have a committer status, will you be pushing this? I can, yes. As soon as we clear-out the remaining questions, right? Yes. The comments below are minor and shouldn't need another review cycle. I'm worried about the failure of the test you observed while running from NetBeans. Perhaps a 0.5s wait is sometimes not enough for ReferenceHandler thread to process (enqueue) a WeakReference. Since there is already a facility in place to help ReferenceHandler thread instead of wait for it, I used it in new version of the test. BTW, there's now an unnecessary import from java.lang.AssertionError added in webrev.04. TEST RESULT: Failed. Compilation failed: Compilation failed I changed the test to be self-contained now so one can run it without testlib in classpath. Thanks. It's compiling fine now. Two minor nits? SeedGenerator.java: Lines 507/518 Done that too. Thanks. Maybe issue multiple reads to exercise in1 and in2? e.g. 2 bytes of in1, 4 bytes of in2, then 2 bytes of in1? The 1st assert makes sure in1 == in2, so there's no point in invoking the same instance via two aliases. True. IIRC, when I ran this under NetBeans last week, the second testCaching didn't clear the WeakReference. This should not happen any more now that the test is helping to enqueue the WeakReferences instead of waiting for ReferenceHandler to enqueue them. Yes, that hit the refQueue.poll(). It's always interesting to work with core-libs folks, learn something new everyday. Mixing Lambdas/try-with. I need a time-machine for your CFV/jdk8 Committer: http://mail.openjdk.java.net/pipermail/jdk8-dev/2013-August/002896.html I vote yes. The test can now fail only if System.gc() does not trigger WeakReference processing in the VM. Can you give it a try on your NetBeans environment? One last comment. It's now 2015. ;) Brad
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
On 1/1/2015 12:22 PM, Peter Levart wrote: Hi Brad, Here's next webrev which tries to cover all your comments: http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.04/ I downloaded the webrev.05 (Chris' later followup email) and ran it through JPRT. The only error was the previously seen -Dseed which is not your problem. Again, I only ran: jdk_lang, jdk_math, jdk_util, jdk_io, jdk_net, jdk_nio, jdk_security*, jdk_rmi, jdk_text, jdk_time, jdk_other, core_tools. If you need anything else run, let me know. Looks like you have a committer status, will you be pushing this? I can, yes. As soon as we clear-out the remaining questions, right? Yes. The comments below are minor and shouldn't need another review cycle. I'm worried about the failure of the test you observed while running from NetBeans. Perhaps a 0.5s wait is sometimes not enough for ReferenceHandler thread to process (enqueue) a WeakReference. Since there is already a facility in place to help ReferenceHandler thread instead of wait for it, I used it in new version of the test. BTW, there's now an unnecessary import from java.lang.AssertionError added in webrev.04. TEST RESULT: Failed. Compilation failed: Compilation failed I changed the test to be self-contained now so one can run it without testlib in classpath. Thanks. It's compiling fine now. Two minor nits? SeedGenerator.java: Lines 507/518 Done that too. Thanks. Maybe issue multiple reads to exercise in1 and in2? e.g. 2 bytes of in1, 4 bytes of in2, then 2 bytes of in1? The 1st assert makes sure in1 == in2, so there's no point in invoking the same instance via two aliases. True. IIRC, when I ran this under NetBeans last week, the second testCaching didn't clear the WeakReference. This should not happen any more now that the test is helping to enqueue the WeakReferences instead of waiting for ReferenceHandler to enqueue them. Yes, that hit the refQueue.poll(). It's always interesting to work with core-libs folks, learn something new everyday. Mixing Lambdas/try-with. I need a time-machine for your CFV/jdk8 Committer: http://mail.openjdk.java.net/pipermail/jdk8-dev/2013-August/002896.html I vote yes. The test can now fail only if System.gc() does not trigger WeakReference processing in the VM. Can you give it a try on your NetBeans environment? One last comment. It's now 2015. ;) Brad
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
Just to followup, I've analyzed the whole PIT run. The second one's call stack is identical to: JDK-8067344: Adjust java/lang/invoke/LFCaching/LFGarbageCollectedTest.java for recent changes in java.lang.invoke So, really the only problem is the use of Asserts in your test case. Brad Looks like you have a committer status, will you be pushing this? I can, yes. As soon as we clear-out the remaining questions, right? Yes. The comments below are minor and shouldn't need another review cycle. I have started a JPRT job for you, I'll run it through core target which will give us: jdk_lang, jdk_math, jdk_util, jdk_io, jdk_net, jdk_nio, jdk_security*, jdk_rmi, jdk_text, jdk_time, jdk_other, core_tools. Anything else? I'm off tomorrow, I should have full results Wed. Here are the preliminary results for the jobs that have finished. jdk.testlibrary.Asserts is causing compilation errors, additional comments below: /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:33: error: package jdk.testlibrary does not exist import static jdk.testlibrary.Asserts.*; ^ /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:52: error: cannot find symbol assertEquals(bytes.length, nread, short read); ^ symbol: method assertEquals(int,int,String) location: class FileInputStreamPoolTest /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:53: error: cannot find symbol assertTrue(Arrays.equals(readBytes, bytes), ^ symbol: method assertTrue(boolean,String) location: class FileInputStreamPoolTest 3 errors TEST RESULT: Failed. Compilation failed: Compilation failed I'm also getting failures in the following test. I unfortunately have to leave now, so don't have time to look at this. But it did mention seed so I'm mentioning it here. TEST: java/lang/invoke/LFCaching/LFGarbageCollectedTest.java ACTION: main -- Failed. Execution failed: `main' threw exception: java.lang.Error: 36 of 39 test cases FAILED! Rerun the test with the same -Dseed= option as in the log file! REASON: User specified action: run main/othervm LFGarbageCollectedTest TIME: 213.406 seconds messages: command: main LFGarbageCollectedTest reason: User specified action: run main/othervm LFGarbageCollectedTest elapsed time (seconds): 213.406 STDOUT: -Dseed=6102311124531075592 -DtestLimit=2000 Number of iterations according to -DtestLimit is 153 (1989 cases) Code cache size is 251658240 bytes Non-profiled code cache size is 123058253 bytes Number of iterations limited by code cache size is 84 (1092 cases) Number of iterations limited by non-profiled code cache size is 41 (533 cases) Number of iterations is set to 41 (533 cases) Not enough time to continue execution. Interrupted. STDERR: Iteration 0: Tested LF caching feature with MethodHandles.foldArguments method. java.lang.AssertionError: Error: Lambda form is not garbage collected at LFGarbageCollectedTest.doTest(LFGarbageCollectedTest.java:81) at LambdaFormTestCase$TestRun.doIteration(LambdaFormTestCase.java:139) at LambdaFormTestCase$$Lambda$2/5042013.call(Unknown Source) at jdk.testlibrary.TimeLimitedRunner.call(TimeLimitedRunner.java:71) at LambdaFormTestCase.runTests(LambdaFormTestCase.java:201) at LFGarbageCollectedTest.main(LFGarbageCollectedTest.java:105) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
I'm looking at this version of the webrev. http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.03/ I just assigned 8047769 to you. My username is wetmore, Alan is alanb. On 12/24/2014 3:37 AM, Peter Levart wrote: Looks like you have a committer status, will you be pushing this? I can, yes. As soon as we clear-out the remaining questions, right? Yes. The comments below are minor and shouldn't need another review cycle. I have started a JPRT job for you, I'll run it through core target which will give us: jdk_lang, jdk_math, jdk_util, jdk_io, jdk_net, jdk_nio, jdk_security*, jdk_rmi, jdk_text, jdk_time, jdk_other, core_tools. Anything else? I'm off tomorrow, I should have full results Wed. Here are the preliminary results for the jobs that have finished. jdk.testlibrary.Asserts is causing compilation errors, additional comments below: /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:33: error: package jdk.testlibrary does not exist import static jdk.testlibrary.Asserts.*; ^ /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:52: error: cannot find symbol assertEquals(bytes.length, nread, short read); ^ symbol: method assertEquals(int,int,String) location: class FileInputStreamPoolTest /opt/jprt/T/P1/003505.brwetmor/s/jdk/test/sun/security/provider/FileInputStreamPool/FileInputStreamPoolTest.java:53: error: cannot find symbol assertTrue(Arrays.equals(readBytes, bytes), ^ symbol: method assertTrue(boolean,String) location: class FileInputStreamPoolTest 3 errors TEST RESULT: Failed. Compilation failed: Compilation failed I'm also getting failures in the following test. I unfortunately have to leave now, so don't have time to look at this. But it did mention seed so I'm mentioning it here. TEST: java/lang/invoke/LFCaching/LFGarbageCollectedTest.java ACTION: main -- Failed. Execution failed: `main' threw exception: java.lang.Error: 36 of 39 test cases FAILED! Rerun the test with the same -Dseed= option as in the log file! REASON: User specified action: run main/othervm LFGarbageCollectedTest TIME: 213.406 seconds messages: command: main LFGarbageCollectedTest reason: User specified action: run main/othervm LFGarbageCollectedTest elapsed time (seconds): 213.406 STDOUT: -Dseed=6102311124531075592 -DtestLimit=2000 Number of iterations according to -DtestLimit is 153 (1989 cases) Code cache size is 251658240 bytes Non-profiled code cache size is 123058253 bytes Number of iterations limited by code cache size is 84 (1092 cases) Number of iterations limited by non-profiled code cache size is 41 (533 cases) Number of iterations is set to 41 (533 cases) Not enough time to continue execution. Interrupted. STDERR: Iteration 0: Tested LF caching feature with MethodHandles.foldArguments method. java.lang.AssertionError: Error: Lambda form is not garbage collected at LFGarbageCollectedTest.doTest(LFGarbageCollectedTest.java:81) at LambdaFormTestCase$TestRun.doIteration(LambdaFormTestCase.java:139) at LambdaFormTestCase$$Lambda$2/5042013.call(Unknown Source) at jdk.testlibrary.TimeLimitedRunner.call(TimeLimitedRunner.java:71) at LambdaFormTestCase.runTests(LambdaFormTestCase.java:201) at LFGarbageCollectedTest.main(LFGarbageCollectedTest.java:105) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) In a couple places, there are a few lines 80 chars. (It's a pet peeve of mine, this makes side-by-side reviews difficult without a wide monitor. I realize not everyone shares this view, but they're probably not working on a laptop screen regularly.) I have wrapped the lines to contain them inside the 80 column margin. I and my scrubber/slider thank you. :) Two minor nits? SeedGenerator.java: Lines 507/518 Do you need to close the InputStream when last holder is GC'd, or do we just let the FileInputStream's finalizer take care of that? WeakReferenceUncloseableInputStream is enqueued when it is cleared, so at that time we have no access to the referent (UncloseableInputStream) any more. We could, in addition, strongly reference the underlying FileInputStream in the WeakReference subclass itself, but that would just prolong the life of FileInputStream (possibly forever if no further calls to FileInputStreamPool are made that expunge the references from the queue). So yes, we rely on FileInputStream's finalizer, but I don't see any other way that would be better than that. Makes sense, thanks. NativePRNG and URLSeedGenerator don't have a means of closing underlying resources either, so this is not making things any worse. Yes. Both of these current calls are contained within a AccessContrller.doPriviledged, the checkRead() seems redundant. That's right, but from
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
Hi Peter,
Looks like you have a committer status, will you be pushing this?
On 12/18/2014 5:23 AM, Peter Levart wrote:
Hi,
I propose a patch for the following issue:
https://bugs.openjdk.java.net/browse/JDK-8047769
Here's the webrev:
http://cr.openjdk.java.net/~plevart/jdk9-dev/FileInputStreamPool.8047769/webrev.01/
Looks good. A few minor comments.
In a couple places, there are a few lines 80 chars. (It's a pet peeve
of mine, this makes side-by-side reviews difficult without a wide
monitor. I realize not everyone shares this view, but they're probably
not working on a laptop screen regularly.)
Do you need to close the InputStream when last holder is GC'd, or do we
just let the FileInputStream's finalizer take care of that?
Both of these current calls are contained within a
AccessContrller.doPriviledged, the checkRead() seems redundant.
In your test case, if assertions are not enabled, the tests at 46/50/51
are noops, e.g. when run by hand. Generally should directly throw
Exceptions.
The patch uses a package-private FileInputStreamPool class that caches
open FileInputStream(s) so that at most one file descriptor is open for
a particular file. Reading from shared unbuffered FileInputStream in
multiple threads should be safe, right?
I would think (hope?) so, but I am not 100% sure. I tracked it down
into libjava native code:
io_util.c
=
fd = GET_FD(this, fid);
if (fd == -1) {
JNU_ThrowIOException(env, Stream Closed);
nread = -1;
} else {
nread = IO_Read(fd, buf, len);
which is then defined to handleRead, which calls something called
RESTARTABLE in io_util_md.c. Assuming I'm looking at the right code.
Core-libs folks?
sun/security/provider/PolicyFile/GrantAllPermToExtWhenNoPolicy.java:
Make sure that when no system policy and user policy files exist, the
built-in default policy will be used, which - amongst other things -
grants standard extensions the AllPermission.
sun/security/provider/PolicyParser/ExtDirsChange.java: standard
extensions path is hard-coded in default system policy file
sun/security/provider/PolicyParser/PrincipalExpansionError.java: parser
incorrectly ignores a principal if the principal name expands to nothing.
...they are unrelated to this patch - seems to be caused by recent
layout changes for modular runtime images.
Hopefully you saw my previous response. Repeating:
---begin---
They've been failing for a while:
https://bugs.openjdk.java.net/browse/JDK-8039280
These tests are all /manual so they don't show up in our regular runs
of JPRT/jtreg, which include -a.
-a | -automatic | -automagic
Any test with /manual will not be run
---end---
Thanks,
Brad
Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors
Peter, Thanks for looking into this. I'll in the middle of reviewing your change (and TLR/SplittableRandom reply), but have several appointments over the next few days. But did want to respond to: sun/security/provider/PolicyFile/GrantAllPermToExtWhenNoPolicy.java: Make sure that when no system policy and user policy files exist, the built-in default policy will be used, which - amongst other things - grants standard extensions the AllPermission. sun/security/provider/PolicyParser/ExtDirsChange.java: standard extensions path is hard-coded in default system policy file sun/security/provider/PolicyParser/PrincipalExpansionError.java: parser incorrectly ignores a principal if the principal name expands to nothing. ...they are unrelated to this patch - seems to be caused by recent layout changes for modular runtime images. They've been failing for a while: https://bugs.openjdk.java.net/browse/JDK-8039280 These tests are all /manual so they don't show up in our regular runs of JPRT/jtreg, which include -a. -a | -automatic | -automagic Any test with /manual will not be run Thanks, Brad
Re: [9] RFR 8043349: Consider adding aliases for Ucrypto algorithm-only Cipher transformations.
I think this is ok. I have a recollection our Cipher.getInstance() provider selection mechanism (getTransforms()) allows for missing options: AES//NoPadding AES/ECB/ But it's been a while since I've looked at this. These ucrypto values look like they must be completely specified. Is that something to look into for down the road? One other point, is there a reason why we're not including the Supported* values in ucrypto? Brad On 12/17/2014 3:18 PM, Valerie Peng wrote: Hi, Brad, Can you please review this straightforward Ucrypto fix? This is about adding aliases to the AES and RSA ciphers of OracleUcrypto provider. Webrev: http://cr.openjdk.java.net/~valeriep/8043349/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8043349 Thanks, Valerie
Re: Detecting whether an algorithm is supported without creating one?
On 12/11/2014 8:02 PM, Weijun Wang wrote:
I'd like to check if SHA-256 is supported without calling
MessageDigest.getInstance(SHA-256). Does such a method exist?
Not that I'm aware of.
Brad
My case is a multi-thread digestor like this:
class Digestor {
Digestor(String alg) throws NSAE;
@ThreadSafe byte[] digest(byte[]) throws Nothing;
}
So a Digestor is created and multiple threads can call the digest()
method. I would be glad if the constructor can throw an NSAE but not
creating a MessageDigest object because I don't know how to safely use
it inside digest().
Thanks
Max
Re: JDK 9 RFR of JDK-8066638: Suppress deprecation warnings in jdk.crypto module
Joe,
This looks good to me, but Valerie (PKCS11 owner) and Xuelei (TLS owner)
should also have a look at this.
Brad
On 12/4/2014 3:41 PM, joe darcy wrote:
Hello,
Please review my changes to fix
JDK-8066638: Suppress deprecation warnings in jdk.crypto module
http://cr.openjdk.java.net/~darcy/8066638.0/
Patch inline below.
(Background effort on the overall deprecation suppression effort written
up at
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-December/030085.html)
Thanks,
-Joe
---
old/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11Key.java
2014-12-04 15:39:05.353994901 -0800
+++
new/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11Key.java
2014-12-04 15:39:05.170086892 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2003, 2013, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2003, 2014, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -445,6 +445,7 @@
}
}
+@SuppressWarnings(deprecation)
private static class P11TlsMasterSecretKey extends P11SecretKey
implements TlsMasterSecret {
private static final long serialVersionUID =
-1318560923770573441L;
---
old/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11RSACipher.java
2014-12-04 15:39:05.865738926 -0800
+++
new/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11RSACipher.java
2014-12-04 15:39:05.685828917 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2003, 2013, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2003, 2014, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -169,6 +169,7 @@
}
// see JCE spec
+@SuppressWarnings(deprecation)
protected void engineInit(int opmode, Key key,
AlgorithmParameterSpec params, SecureRandom random)
throws InvalidKeyException,
InvalidAlgorithmParameterException {
@@ -461,6 +462,7 @@
}
// see JCE spec
+@SuppressWarnings(deprecation)
protected Key engineUnwrap(byte[] wrappedKey, String algorithm,
int type) throws InvalidKeyException,
NoSuchAlgorithmException {
---
old/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11Signature.java
2014-12-04 15:39:06.429456952 -0800
+++
new/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11Signature.java
2014-12-04 15:39:06.221560942 -0800
@@ -765,12 +765,14 @@
}
// see JCA spec
+@SuppressWarnings(deprecation)
protected void engineSetParameter(String param, Object value)
throws InvalidParameterException {
throw new UnsupportedOperationException(setParameter() not
supported);
}
// see JCA spec
+@SuppressWarnings(deprecation)
protected Object engineGetParameter(String param)
throws InvalidParameterException {
throw new UnsupportedOperationException(getParameter() not
supported);
---
old/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11TlsKeyMaterialGenerator.java
2014-12-04 15:39:06.989176978 -0800
+++
new/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11TlsKeyMaterialGenerator.java
2014-12-04 15:39:06.777282969 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2005, 2013, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2005, 2014, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -62,6 +62,7 @@
private long mechanism;
// parameter spec
+@SuppressWarnings(deprecation)
private TlsKeyMaterialParameterSpec spec;
// master secret as a P11Key
@@ -82,6 +83,7 @@
throw new InvalidParameterException(MSG);
}
+@SuppressWarnings(deprecation)
protected void engineInit(AlgorithmParameterSpec params,
SecureRandom random) throws
InvalidAlgorithmParameterException {
if (params instanceof TlsKeyMaterialParameterSpec == false) {
@@ -107,6 +109,7 @@
throw new InvalidParameterException(MSG);
}
+@SuppressWarnings(deprecation)
protected SecretKey engineGenerateKey() {
if (spec == null) {
throw new IllegalStateException
---
old/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11TlsMasterSecretGenerator.java
2014-12-04 15:39:07.540901004 -0800
+++
new/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11TlsMasterSecretGenerator.java
2014-12-04 15:39:07.337002994 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2005, 2007, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2005, 2014, Oracle and/or its affiliates. All
Re: A fully fledged TLS Extensions API ?
Simone, Your note was good timing. ALPN and HTTP/2 is on our radar for 9, and we're starting to look at options now, maybe even using some type of general extension API. ALPN is pretty straightforward and can be done easily, but there are advantages to having a full extensions API. We're also looking at the older threads for additional context. Brad PS. Just a quick note, the reason for SSLSocket/SSLEngine/SSLParameters having duplicate methods is purely historical. Up to JDK 1.5 we added new methods to SSLEngine/SSLSocket for each new configuration option, but in JDK 6 a parameter group seemed a better way to go, so the duplicate APIs were added. The group allowed for a single call, which made for easier config of multiple sockets. sslp = sslSocket1.getSSLParameters(); or sslp = new SSLParameters(); sslp.set...(); sslp.set...(); sslp.set...(); sslSocket2.setSSLParameters(sslp); sslSocket3.setSSLParameters(sslp); On 11/7/2014 5:06 AM, Simone Bordet wrote: Hi, I was writing the email about the ALPN API proposal when I realized that it would have been better to split the arguments in different emails. This email is about the idea to introduce in JDK 9 a fully fledged TLS Extensions API. Adding ALPN [0] support to JDK 9 requires, differently from other TLS extensions, to provide application code that will be run in the context of the TLS implementation, rather than just values such as booleans or strings. IMHO this chance can be lifted to provide a full TLS Extensions API. Alternative providers such as IAIK offer a private API such as [1]: SSLSocket.getActiveExtensions() SSLSocket.getPeerExtensions() Similarly, BouncyCastle offers [2]: DefaultTlsClient.getClientExtensions() In the JDK there is a class named SSLParameters that contains some of the configuration values for TLS. Some of those are duplicated in SSLSocket and SSLEngine (e.g. wantClientAuth, needClientAuth, etc), with some temporal dependency (call this before the other, if I have to trust the comments of SSLSocketImpl.setHost()). Eventually all values get forwarded to the handshaker, but from the API point of view it's not very clear what API one should call (the one on SSLEngine or the one on SSLParameters), nor where the ALPN setup should be done. The idea would then be to introduce a fully fledged TLS extensions API to handle all the TLS extensions related things, such as renegotiation, SNI, elliptic curves, NPN, ALPN, session tickets etc. Both applications and the JDK implementation would be able to leverage this new API. Note that the bulk of this API already exists in sun.security.ssl. My question is really whether JDK 9 could take in consideration such TLS Extension API be exposed publicly for applications to use it. See the other emails about ALPN and the interaction between ALPN and HTTP 2 for further examples of where this TLS Extension API would come handy. Thanks ! [0] http://tools.ietf.org/html/rfc7301 [1] http://javadoc.iaik.tugraz.at/isasilk/current/iaik/security/ssl/SSLSocket.html [2] https://www.bouncycastle.org/docs/docs1.5on/org/bouncycastle/crypto/tls/DefaultTlsClient.html
Re: CFV: New Security Group Member: Anthony Scarpino
Vote: Yes Brad On 11/3/2014 9:28 AM, Sean Mullan wrote: I hereby nominate Anthony Scarpino to Membership in the Security Group. Anthony is a member of the Java Security Libraries team at Oracle and has been an active contributor to the OpenJDK Security Group for approximately two years. Anthony was voted in as a JDK 8 committer in November, 2013, and has continued to play an active role in the JDK 8 update releases and JDK 9. Anthony has integrated many significant bug fixes and enhancements especially in the crypto area. He is also the owner of the JVM Hardware Crypto Acceleration JEP, which has recently been submitted for further evaluation. Votes are due by November 17, 2014, 10:00 AM PST. Only current Members of the Security Group [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list. For Lazy Consensus voting instructions, see [2]. Sean Mullan [1] http://openjdk.java.net/census#security [2] http://openjdk.java.net/groups/#member-vote
Re: Linux getrandom() support
Worth looking into, but no plans at the moment. Do you have a link? Brad On 10/5/2014 7:44 PM, Bernd wrote: Hello, Is there already support for the upcoming getrandom() syscall in Linux 3.17 kernel planned? I guess this would be a good feature for SSL and the strong SecureRandom variant (by setting the 128bit entropy required flag). It would be good if this is supported out of the box, especially to avoid problems when the dev files are missing in some container/virtualisation systems. Greetings Bernd
Re: RFR: JDK-8058845 : Update JCE environment for build improvements
javax/crypto/JceSecurity.java line 79: this could be (PrivilegedExceptionActionVoid) as the return value is ignored Good catch. It may be better to rename URLVerifier to ProviderVerifier as it verifies the security provider of the given codebase. URLVerifier might give an interpretation of verifying the given URL. Similarly, the verifyProviderJar method can be renamed to verifyProvider. Done. javax/crypto/URLVerifier.java line 117: should it be pae.getCause()? Yes, that would be a better one. Thanks for the review. brad
RFR: JDK-8058845 : Update JCE environment for build improvements
Hi Sean/Mandy/Erik/Magnus/Alan/David/others, Please review: JDK-8058845 : Update JCE environment for build improvements http://cr.openjdk.java.net/~wetmore/8058845/ This change is to alleviate some of the overly-complicated steps we (Oracle) have in building and maintaining the JCE jar files in JDK 9. Besides the Makefiles and a class rename, there is very little change to the Open code for the OpenJDK Security folks. The OpenJDK build folks shouldn't really notice any difference. I do wish we could completely do away with this code completely, but we (Oracle and closed licensees) do still need to follow the regulations. For the Iced Tea maintainers, this will necessitate a small incremental change to your JCE patch. Thanks, Brad
Re: RFR(XXS) : 8057813: Alterations to jdk_security3 test target
I'm not an expert in the JPRT/makefiles for test, but looks good. Brad On 9/9/2014 11:45 AM, Sean Mullan wrote: This seems like a good idea to me. Looks ok to me. --Sean On 09/09/2014 01:10 PM, Seán Coffey wrote: On some recent JPRT test jobs, I've noticed that the jdk_security3 test target is taking 40+ mins on some systems. Looking closer at test times, I see that sun/security/krb5 tests alone can take ~15-16 mins to run. I'd like to separate those tests out into their own test target (jdk_security4) so that we can better utilize idle clients during JPRT test runs. bug : https://bugs.openjdk.java.net/browse/JDK-8057813 webrev : http://cr.openjdk.java.net/~coffeys/webrev.8057813.jdk9/webrev/ regards, Sean.
Re: RFR 8054366: Broken link in SecureRandom.html
On 8/7/2014 3:59 PM, Sean Mullan wrote: On 08/07/2014 05:03 PM, Jamil Nimeh wrote: Hello all, This is just a quick broken-link fix for SecureRandom's javadoc. http://cr.openjdk.java.net/~ascarpino/8054366/webrev.01 The fix for the broken link looks fine. I think you should double-check with Brad as to whether changing the RFC reference is appropriate. Looking over the Appendix A (changes from 1750), I think this should be ok. These seem to be primarily additional suggestions/caveats, not requirements. Brad
Re: RFR: 8042982: Unexpected RuntimeExceptions being thrown by SSLEngine
BTW, if you feel like it in any backports, the casts to SSLHandshakeException weren't needed. Brad On 8/1/2014 11:46 AM, Rob McKenna wrote: Thanks Brad, patch updated, built tested. -Rob On 01/08/14 01:39, Bradford Wetmore wrote: Rob, Looks ok to me too. There are probably other places with RTE's we could fix, but this will solve the immediate problem. Two comments to consider: 1. Use a Multi-catch exception. JDK7+. 2. DHCrypt throws IOException. ECDHCrypt throws SSLException (which is an IOException). Since DHCrypt/ECDHCrypt are essentially the same kind of class, maybe update DHCrypt to throw the same? Brad On 7/25/2014 5:52 PM, Xuelei Fan wrote: Looks fine to me. Thanks, Xuelei On 7/22/2014 9:37 PM, Rob McKenna wrote: Hi folks, A simple change to use SSLHandshakeException instead of RuntimeException in getAgreedSecret in DHCrypt and ECDHCrypt. This will prevent these RuntimeExceptions from propagating to the application and allow application programmers to handle them as SSLHandshakeExceptions. http://cr.openjdk.java.net/~robm/8042982/webrev.01/ -Rob
Re: RFR: 8042982: Unexpected RuntimeExceptions being thrown by SSLEngine
Rob, Looks ok to me too. There are probably other places with RTE's we could fix, but this will solve the immediate problem. Two comments to consider: 1. Use a Multi-catch exception. JDK7+. 2. DHCrypt throws IOException. ECDHCrypt throws SSLException (which is an IOException). Since DHCrypt/ECDHCrypt are essentially the same kind of class, maybe update DHCrypt to throw the same? Brad On 7/25/2014 5:52 PM, Xuelei Fan wrote: Looks fine to me. Thanks, Xuelei On 7/22/2014 9:37 PM, Rob McKenna wrote: Hi folks, A simple change to use SSLHandshakeException instead of RuntimeException in getAgreedSecret in DHCrypt and ECDHCrypt. This will prevent these RuntimeExceptions from propagating to the application and allow application programmers to handle them as SSLHandshakeExceptions. http://cr.openjdk.java.net/~robm/8042982/webrev.01/ -Rob
Re: ThreadLocalRandom clinit troubles
On 6/26/2014 4:10 PM, Doug Lea wrote: This seems to be the best idea yet, assuming that people are OK with the changes to sun.security.provider.SeedGenerator and NativeSeedGenerator.java I've been meaning to review this thread, but have been chasing several urgent escalations. Brad
Re: RFR: 8047721: @since should have JDK version
I would prefer that JCE1.2 be pulled out completely in the Cipher*
classes. I will be sending you a separate note about JCE logistics.
Thanks for doing this cleanup.
Brad
On 6/20/2014 11:46 AM, Henry Jen wrote:
Hi,
Please review a trivial webrev to add JDK version to @since in a format
as Mark suggested[1].
http://cr.openjdk.java.net/~henryjen/jdk9/8047721/0/webrev/
[1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2014-June/000806.html
Appened is the diff as in the webrev.
Cheers,
Henry
diff --git a/src/share/classes/java/lang/Package.java
b/src/share/classes/java/lang/Package.java
--- a/src/share/classes/java/lang/Package.java
+++ b/src/share/classes/java/lang/Package.java
@@ -107,6 +107,7 @@
* loader to be found.
*
* @see ClassLoader#definePackage
+ * @since 1.2
*/
public class Package implements java.lang.reflect.AnnotatedElement {
/**
diff --git a/src/share/classes/javax/crypto/CipherInputStream.java
b/src/share/classes/javax/crypto/CipherInputStream.java
--- a/src/share/classes/javax/crypto/CipherInputStream.java
+++ b/src/share/classes/javax/crypto/CipherInputStream.java
@@ -170,7 +170,7 @@
* @return the next byte of data, or code-1/code if the end
of the
* stream is reached.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read() throws IOException {
if (ostart = ofinish) {
@@ -196,7 +196,7 @@
* the stream has been reached.
* @exception IOException if an I/O error occurs.
* @seejava.io.InputStream#read(byte[], int, int)
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read(byte b[]) throws IOException {
return read(b, 0, b.length);
@@ -217,7 +217,7 @@
* the stream has been reached.
* @exception IOException if an I/O error occurs.
* @seejava.io.InputStream#read()
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read(byte b[], int off, int len) throws IOException {
if (ostart = ofinish) {
@@ -254,7 +254,7 @@
* @param n the number of bytes to be skipped.
* @return the actual number of bytes skipped.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public long skip(long n) throws IOException {
int available = ofinish - ostart;
@@ -277,7 +277,7 @@
* @return the number of bytes that can be read from this
input stream
* without blocking.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int available() throws IOException {
return (ofinish - ostart);
@@ -292,7 +292,7 @@
* stream.
*
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void close() throws IOException {
if (closed) {
@@ -321,7 +321,7 @@
* codemark/code and codereset/code methods.
* @see java.io.InputStream#mark(int)
* @see java.io.InputStream#reset()
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public boolean markSupported() {
return false;
diff --git a/src/share/classes/javax/crypto/CipherOutputStream.java
b/src/share/classes/javax/crypto/CipherOutputStream.java
--- a/src/share/classes/javax/crypto/CipherOutputStream.java
+++ b/src/share/classes/javax/crypto/CipherOutputStream.java
@@ -114,7 +114,7 @@
*
* @param b the codebyte/code.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void write(int b) throws IOException {
ibuffer[0] = (byte) b;
@@ -138,7 +138,7 @@
* @exception NullPointerException if codeb/code is null.
* @exception IOException if an I/O error occurs.
* @seejavax.crypto.CipherOutputStream#write(byte[], int,
int)
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void write(byte b[]) throws IOException {
write(b, 0, b.length);
@@ -152,7 +152,7 @@
* @param off the start offset in the data.
* @param len the number of bytes to write.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void write(byte b[], int off, int len) throws IOException {
obuffer = cipher.update(b, off, len);
@@ -174,7 +174,7 @@
* the cipher's block size, no bytes will be written out.
*
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void flush() throws IOException {
if (obuffer != null) {
@@ -198,7 +198,7 @@
Re: ThreadLocalRandom clinit troubles
Martin,
Thanks for filing. I was positive there was already a bug for this, but
for the life of me I can't find it now. There's some other more minor
cleanup that needs to take place, but seems like I've been in
escalation/firefighting mode for more than a year now and it hasn't
bubbled to the top.
Brad
On 6/21/2014 9:05 PM, Martin Buchholz wrote:
While looking at NativePRNG, I filed
https://bugs.openjdk.java.net/browse/JDK-8047769
SecureRandom should be more frugal with file descriptors
If I run this java program on Linux
public class SecureRandoms {
public static void main(String[] args) throws Throwable {
new java.security.SecureRandom();
}
}
it creates 6 file descriptors for /dev/random and /dev/urandom, as shown
by:
strace -q -ff -e open java SecureRandoms | grep /dev/
[pid 20769] open(/dev/random, O_RDONLY) = 5
[pid 20769] open(/dev/urandom, O_RDONLY) = 6
[pid 20769] open(/dev/random, O_RDONLY) = 7
[pid 20769] open(/dev/random, O_RDONLY) = 8
[pid 20769] open(/dev/urandom, O_RDONLY) = 9
[pid 20769] open(/dev/urandom, O_RDONLY) = 10
Looking at jdk/src/solaris/classes/sun/security/provider/NativePRNG.java
it looks like 2 file descriptors are created for every variant of
NativePRNG, whether or not they are ever used. Which is wasteful. In
fact, you only ever need at most two file descriptors, one for
/dev/random and one for /dev/urandom.
Further, it would be nice if the file descriptors were closed when idle
and lazily re-created. Especially /dev/random should typically be used
at startup and never thereafter.
On Fri, Jun 20, 2014 at 7:59 AM, Alan Bateman alan.bate...@oracle.com
mailto:alan.bate...@oracle.com wrote:
On 20/06/2014 15:02, Peter Levart wrote:
And, as Martin pointed out, it seems to be used for tests that
exercise particular responses from NameService API to test the
behaviour of JDK classes. It would be a shame for those tests to
go away.
We've been talking about removing it for many years because it has
been so troublesome. If we really need to having something for
testing then I don't think it needs to be general purpose, we can
get right of the lookup at least.
-Alan.
Re: RFR: 8047721: @since should have JDK version
JCE (Cipher) changes look good to me.
Let me know if there's any problem with the point I mentioned in the
other email.
Brad
On 6/23/2014 1:50 PM, Henry Jen wrote:
OK, I'll remove all @since JCE line, as the class already has @since 1.4
as Joe pointed out earlier.
Uodated webrev at
http://cr.openjdk.java.net/~henryjen/jdk9/8047721/2/webrev/
Cheers,
Henry
On 06/23/2014 10:04 AM, Bradford Wetmore wrote:
I would prefer that JCE1.2 be pulled out completely in the Cipher*
classes. I will be sending you a separate note about JCE logistics.
Thanks for doing this cleanup.
Brad
On 6/20/2014 11:46 AM, Henry Jen wrote:
Hi,
Please review a trivial webrev to add JDK version to @since in a format
as Mark suggested[1].
http://cr.openjdk.java.net/~henryjen/jdk9/8047721/0/webrev/
[1]
http://mail.openjdk.java.net/pipermail/jdk9-dev/2014-June/000806.html
Appened is the diff as in the webrev.
Cheers,
Henry
diff --git a/src/share/classes/java/lang/Package.java
b/src/share/classes/java/lang/Package.java
--- a/src/share/classes/java/lang/Package.java
+++ b/src/share/classes/java/lang/Package.java
@@ -107,6 +107,7 @@
* loader to be found.
*
* @see ClassLoader#definePackage
+ * @since 1.2
*/
public class Package implements java.lang.reflect.AnnotatedElement {
/**
diff --git a/src/share/classes/javax/crypto/CipherInputStream.java
b/src/share/classes/javax/crypto/CipherInputStream.java
--- a/src/share/classes/javax/crypto/CipherInputStream.java
+++ b/src/share/classes/javax/crypto/CipherInputStream.java
@@ -170,7 +170,7 @@
* @return the next byte of data, or code-1/code if the end
of the
* stream is reached.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read() throws IOException {
if (ostart = ofinish) {
@@ -196,7 +196,7 @@
* the stream has been reached.
* @exception IOException if an I/O error occurs.
* @seejava.io.InputStream#read(byte[], int, int)
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read(byte b[]) throws IOException {
return read(b, 0, b.length);
@@ -217,7 +217,7 @@
* the stream has been reached.
* @exception IOException if an I/O error occurs.
* @seejava.io.InputStream#read()
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read(byte b[], int off, int len) throws IOException {
if (ostart = ofinish) {
@@ -254,7 +254,7 @@
* @param n the number of bytes to be skipped.
* @return the actual number of bytes skipped.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public long skip(long n) throws IOException {
int available = ofinish - ostart;
@@ -277,7 +277,7 @@
* @return the number of bytes that can be read from this
input stream
* without blocking.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int available() throws IOException {
return (ofinish - ostart);
@@ -292,7 +292,7 @@
* stream.
*
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void close() throws IOException {
if (closed) {
@@ -321,7 +321,7 @@
* codemark/code and codereset/code methods.
* @see java.io.InputStream#mark(int)
* @see java.io.InputStream#reset()
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public boolean markSupported() {
return false;
diff --git a/src/share/classes/javax/crypto/CipherOutputStream.java
b/src/share/classes/javax/crypto/CipherOutputStream.java
--- a/src/share/classes/javax/crypto/CipherOutputStream.java
+++ b/src/share/classes/javax/crypto/CipherOutputStream.java
@@ -114,7 +114,7 @@
*
* @param b the codebyte/code.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void write(int b) throws IOException {
ibuffer[0] = (byte) b;
@@ -138,7 +138,7 @@
* @exception NullPointerException if codeb/code is null.
* @exception IOException if an I/O error occurs.
* @seejavax.crypto.CipherOutputStream#write(byte[], int,
int)
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void write(byte b[]) throws IOException {
write(b, 0, b.length);
@@ -152,7 +152,7 @@
* @param off the start offset in the data.
* @param len the number of bytes to write.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public void
Re: RFR: 8047721: @since should have JDK version
Except for these two classes, none of the JCE APIs ever contained @since
until the JCE was put into JDK 1.4 back in 2002. The unbundled JCE
hasn't been shipped in probably almost a decade. None of the unbundled
JSSE/JGSS should have them either.
Carrying around this old information is just cruft, IMO.
Brad
On 6/23/2014 2:28 PM, Paul Benedict wrote:
What's the rationale for removing the secondary version? Or I guess the
question should really be: when are secondary versions useful? At least
in the EE specs, the EE version plus the spec version are listed in many
places like this.
Cheers,
Paul
On Mon, Jun 23, 2014 at 3:50 PM, Henry Jen henry@oracle.com
mailto:henry@oracle.com wrote:
OK, I'll remove all @since JCE line, as the class already has @since
1.4 as Joe pointed out earlier.
Uodated webrev at
http://cr.openjdk.java.net/~__henryjen/jdk9/8047721/2/__webrev/
http://cr.openjdk.java.net/~henryjen/jdk9/8047721/2/webrev/
Cheers,
Henry
On 06/23/2014 10:04 AM, Bradford Wetmore wrote:
I would prefer that JCE1.2 be pulled out completely in the Cipher*
classes. I will be sending you a separate note about JCE logistics.
Thanks for doing this cleanup.
Brad
On 6/20/2014 11:46 AM, Henry Jen wrote:
Hi,
Please review a trivial webrev to add JDK version to @since
in a format
as Mark suggested[1].
http://cr.openjdk.java.net/~__henryjen/jdk9/8047721/0/__webrev/
http://cr.openjdk.java.net/~henryjen/jdk9/8047721/0/webrev/
[1]
http://mail.openjdk.java.net/__pipermail/jdk9-dev/2014-June/__000806.html
http://mail.openjdk.java.net/pipermail/jdk9-dev/2014-June/000806.html
Appened is the diff as in the webrev.
Cheers,
Henry
diff --git a/src/share/classes/java/lang/__Package.java
b/src/share/classes/java/lang/__Package.java
--- a/src/share/classes/java/lang/__Package.java
+++ b/src/share/classes/java/lang/__Package.java
@@ -107,6 +107,7 @@
* loader to be found.
*
* @see ClassLoader#definePackage
+ * @since 1.2
*/
public class Package implements
java.lang.reflect.__AnnotatedElement {
/**
diff --git
a/src/share/classes/javax/__crypto/CipherInputStream.java
b/src/share/classes/javax/__crypto/CipherInputStream.java
--- a/src/share/classes/javax/__crypto/CipherInputStream.java
+++ b/src/share/classes/javax/__crypto/CipherInputStream.java
@@ -170,7 +170,7 @@
* @return the next byte of data, or code-1/code
if the end
of the
* stream is reached.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read() throws IOException {
if (ostart = ofinish) {
@@ -196,7 +196,7 @@
* the stream has been reached.
* @exception IOException if an I/O error occurs.
* @seejava.io.InputStream#read(byte[__],
int, int)
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read(byte b[]) throws IOException {
return read(b, 0, b.length);
@@ -217,7 +217,7 @@
* the stream has been reached.
* @exception IOException if an I/O error occurs.
* @seejava.io.InputStream#read()
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public int read(byte b[], int off, int len) throws
IOException {
if (ostart = ofinish) {
@@ -254,7 +254,7 @@
* @param n the number of bytes to be skipped.
* @return the actual number of bytes skipped.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
*/
public long skip(long n) throws IOException {
int available = ofinish - ostart;
@@ -277,7 +277,7 @@
* @return the number of bytes that can be read
from this
input stream
* without blocking.
* @exception IOException if an I/O error occurs.
- * @since JCE1.2
+ * @since 1.4, JCE1.2
Re: Code review request, 8044771, PKIXValidator indent cleanup
On 6/4/2014 2:38 AM, Xuelei Fan wrote: Webrev toolkit ignore space update so the webrev above looks strange. The major update is cleanup the indentation. For example, 4 leading space are removed at line 250 of PKIXValidator.java. Use the -b option in webrev to force whitespace changes to appear. Brad
Re: JDK 9 RFR for 8036841 : Reuse no-perms AccessControlContext object when performing isAuthorized check
Looks ok to me. Brad On 6/2/2014 11:15 AM, Sean Mullan wrote: This is a fix to avoid creating multiple no-perms ACC objects for access control checks. Since this is required in an uncommon scenario, I used the Initialization on demand holder pattern to only create it under those circumstances. webrev: http://cr.openjdk.java.net/~mullan/webrevs/8036841/webrev.01/ --Sean
Webrev for 8043342: StringBuffer/StringBuilder crypto changes.
No additional code review necessary, this is just an FYI. For internal reasons (i.e. we have to sign our JCE jar files), we have separated the JCE portion for: 8041679: Replace uses of StringBuffer with StringBuilder within the JDK into: 8043342: Replace uses of StringBuffer with StringBuilder within crypto code It's the exact same code, but only the four JCE files are included here. http://cr.openjdk.java.net/~wetmore/8043342/webrev.00/ I am sponsoring this for Jamil, from an original fix [1] sponsored by Paul Sandoz from: Otávio Gonçalves de Santana Thanks, Brad [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-May/026820.html
Re: RFR 8037745: Consider re-enabling PKCS11 mechanisms previously disabled due to Solaris bug 7050617
Whoo Hoo. :) Looks good. Brad On 5/15/2014 4:11 PM, Valerie (Yu-Ching) Peng wrote: Tony, Can you please help reviewing this PKCS11 provider configuration file change? This is for removing the digest related mechs from the disabledMechanism section now that relevant native bugs have been fixed. Webrev: http://cr.openjdk.java.net/~valeriep/8037745/webrev.00/ Thanks, Valerie
Re: Code review request [JDK 9] 8042449 Issue for negative byte major record version
I still need an official reviewer. Thanks for looking into this, I was going check into it today if you didn't. I figured it must be something in byte comparison. Sure enough. Good catches! :) That code's been in there a long time! Only nit is Copyright Dates if you choose to update. Rest looks good. Brad On 5/6/2014 6:43 AM, Xuelei Fan wrote: On 5/6/2014 9:39 PM, Florian Weimer wrote: On 05/06/2014 03:37 PM, Xuelei Fan wrote: On 5/6/2014 9:31 PM, Florian Weimer wrote: On 05/06/2014 02:00 PM, Xuelei Fan wrote: Storing both int version and major/minor byte versions is a little bit redundancy. The update is significant. I will focus on the signed byte issue in this fix. Yes, I get that. I've verified that you've covered all the version comparisons. Thanks for the code review. Do you have a OpenJDK author account? I'm not an official reviewer, I'm afraid. I will your name in a also reviewed by section. I still need an official reviewer. Thanks, Xuelei
Re: Security Dev [9] Review Request: 8028266 Tidy warnings cleanup for packages java.security/javax.security
We usually update the copyright dates:
* Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights
reserved.
javax/security/auth/kerberos/package-info.java
==
yes, or no, case-insensitive.
-
yes, or no, and values are case-insensitive.
I noticed in several places that while your changes are ok, it doesn't
update the ivalue/i instead of switching to {@code value}.
In a previous webrev, I mentioned that you'll need to work with someone
to update the built JCE binaries for the javax.crypto changes. Who is
going to help you with that?
Brad
On 4/21/2014 3:11 AM, alexander stepanov wrote:
Hello,
Could you please review the fix for the following bug:
https://bugs.openjdk.java.net/browse/JDK-8028266
Webrev corresponding:
http://cr.openjdk.java.net/~yan/JDK-8028266/webrev.00/
Just a minor cleanup of javadoc to avoid tidy warnings; no other code
affected.
Thanks.
Regards,
Alexander
Re: RFR 8039853: Provider.Service.newInstance() does not work with current JDK JGSS Mechanisms
Thanks Weijun,
The main class is fine.
In the test case, I would suggest adding a comment before the
InvalidAlgorithmParameterException that some engines require certain
parameters to be be present on creation, and a newInstance(null) will
trigger that exception.
HTH,
Brad
On 4/15/2014 8:01 AM, Sean Mullan wrote:
Looks fine to me.
--Sean
On 04/15/2014 04:03 AM, Wang Weijun wrote:
Please review the code changes at
http://cr.openjdk.java.net/~weijun/8039853/webrev.00/
If you find it confused, I have mistakenly pushed some code changes in
http://hg.openjdk.java.net/jdk9/dev/jdk/rev/ba6e2fcdfa15
and the current code change is trying to fix/enhance it. Altogether,
the actual change I want to make is:
diff --git
a/src/share/classes/sun/security/jgss/krb5/Krb5MechFactory.java
b/src/share/classes/sun/security/jgss/krb5/Krb5MechFactory.java
--- a/src/share/classes/sun/security/jgss/krb5/Krb5MechFactory.java
+++ b/src/share/classes/sun/security/jgss/krb5/Krb5MechFactory.java
@@ -86,6 +86,10 @@
return result;
}
+public Krb5MechFactory() {
+this(GSSCaller.CALLER_UNKNOWN);
+}
+
public Krb5MechFactory(GSSCaller caller) {
this.caller = caller;
}
diff --git
a/src/share/classes/sun/security/jgss/spnego/SpNegoMechFactory.java
b/src/share/classes/sun/security/jgss/spnego/SpNegoMechFactory.java
--- a/src/share/classes/sun/security/jgss/spnego/SpNegoMechFactory.java
+++ b/src/share/classes/sun/security/jgss/spnego/SpNegoMechFactory.java
@@ -96,6 +96,10 @@
return result;
}
+public SpNegoMechFactory() {
+this(GSSCaller.CALLER_UNKNOWN);
+}
+
public SpNegoMechFactory(GSSCaller caller) {
manager = new GSSManagerImpl(caller, false);
Oid[] mechs = manager.getMechs();
Please note that the previous change made in
src/share/classes/java/security/Provider.java is now backed out.
Thanks
Max
Re: Security Dev [9] Review Request: 8040260 Tidy warnings cleanup for javax.crypto, javax.net
We generally update the copyright dates to 2014. Since you are changing line numbers, please work with the JCE team to get the JCE provider rebuilt/signed/integrated. make/closed/tools/crypto/jce/jce.jar I wasn't aware of the trade;, nice to know! ;) Thanks, Brad On 4/15/2014 5:24 AM, alexander stepanov wrote: Hello, Could you please review the fix for the following bug: https://bugs.openjdk.java.net/browse/JDK-8040260 Webrev corresponding: http://cr.openjdk.java.net/~yan/8040260/webrev.00/ Just a very minor cleanup of javadoc to avoid tidy warnings; no other code affected. Thanks. Regards, Alexander
Re: Code review request, JDK-8040062 Need to add new methods in BaseSSLSocketImpl
There is a similar one for HttpsURLConnection, as we had problems in the past where the networking changes didn't update the SSL code. jdk/sun/net/www/protocol/https/HttpsURLConnection/CheckMethods.java Just a reminder to please include the JSSE reg tests when making Socket/HttpsURLConnection/etc. API changes or when changing code that is common to these methods. Thankfully we were running it anyway and was caught quickly. Thanks, Brad On 4/14/2014 6:45 AM, Chris Hegarty wrote: +1 . Thanks for updating this Xuelei. Nice test that caught this. -Chris. On 14/04/14 13:59, Sean Mullan wrote: Looks fine to me. Can you add a relates to link to JDK-8036979 and an appropriate noreg label? --Sean On 04/14/2014 06:04 AM, Xuelei Fan wrote: Hi, Please review the fix for JDK-8040062: http://cr.openjdk.java.net/~xuelei/8040062/webrev.00/ In JDK-8036979, there are news methods are added to java.net.Socket. These methods need to be overridden in SSL socket implementation, sun/security/ssl/BaseSSLSocketImpl.java. No new regression test. The existing test: sun/security/ssl/SSLSocketImpl/CheckMethods.java can be used to verify this fix. Thanks, Xuelei
Re: [9] review request for 6977937: The SunJCE PBKDF2KeyImpl is requiring the MAC instance also be from SunJCE.
On 4/5/2014 8:00 AM, Vincent Ryan wrote: I was concerned about the impact on applications of a different JCE provider being selected instead of SunJCE, on some platforms. I can change the fix to follow the standard JCE provider ordering. That would be my preference, but I can see both sides if someone has a strong case. Brad On 04/04/2014 22:23, Bradford Wetmore wrote: With the current and proposed code, you are effectively requiring the MAC come from JCE, as all the algorithms exist in SunJCE. IIRC, when we discussed the previous change in this area, the idea was that the MAC would follow the standard JCA provider priority ordering. Brad On 4/4/2014 8:45 AM, Vincent Ryan wrote: Hello, Please review the following fix to remove the requirement for the Mac algorithm used by a PBKDF2 algorithm to be supplied by the SunJCE provider. The SunJCE provider is still preferred (for compatibility with previous releases and for performance reasons) but it is no longer required. The com.sun.crypto.provider.PBKDF2KeyImpl class first searches SunJCE for the required Mac algorithm but fails over to searching the other installed JCE providers too. Bug: https://bugs.openjdk.java.net/browse/JDK-6977937 Webrev: http://cr.openjdk.java.net/~vinnie/6977937/webrev.00/ Thanks.
Re: [9] review request for 6977937: The SunJCE PBKDF2KeyImpl is requiring the MAC instance also be from SunJCE.
With the current and proposed code, you are effectively requiring the MAC come from JCE, as all the algorithms exist in SunJCE. IIRC, when we discussed the previous change in this area, the idea was that the MAC would follow the standard JCA provider priority ordering. Brad On 4/4/2014 8:45 AM, Vincent Ryan wrote: Hello, Please review the following fix to remove the requirement for the Mac algorithm used by a PBKDF2 algorithm to be supplied by the SunJCE provider. The SunJCE provider is still preferred (for compatibility with previous releases and for performance reasons) but it is no longer required. The com.sun.crypto.provider.PBKDF2KeyImpl class first searches SunJCE for the required Mac algorithm but fails over to searching the other installed JCE providers too. Bug: https://bugs.openjdk.java.net/browse/JDK-6977937 Webrev: http://cr.openjdk.java.net/~vinnie/6977937/webrev.00/ Thanks.
Re: Review Request of JDK Enhancement Proposal: DTLS
On 3/19/2014 5:50 PM, Xuelei Fan wrote: I was wondering to expose this application layer as a configurable parameter. Just to make sure we're talking about the same thing, you're pointing out: 1. The need for determining the PMTU for the various protocol types. (UDP/DCCP/TCP/SCTP/etc) 2. Communicating that to the JSSE layer. Since the current plan is for this DTLS impl to be transport protocol-independent, I think needs to be configurable primarily at the JSSE API. You could take a guess at a default PMTU, but the answer likely won't be right for all. Brad
Re: RFR 8033271: Manual security tests have @ignore rather than @run main/manual
Rajan, All of the tests I looked at have incorrectly modified copyright information. Please fix and then I can push. Copyright (c) 2005, 2006, Oracle and/or its affiliates. All rights reserved. should be: Copyright (c) 2005, 2014, Oracle and/or its affiliates. All rights reserved. Not: * Copyright (c) 2014, Oracle and/or its affiliates. All rights reserved. Brad On 3/17/2014 4:05 PM, Xuelei Fan wrote: Looks fine to me. Thank you, Rajan! Xuelei On 3/18/2014 4:17 AM, Rajan Halade wrote: Thanks again! Updated review with corrections - http://cr.openjdk.java.net/~wetmore/8033271/webrev.03/ - Rajan On 3/14/2014 18:36, Xuelei Fan wrote: Minimal comments: test/sun/security/smartcardio/TestAll.java == Looks like there is no actual update. test/sun/security/smartcardio/* === - 32 //This test requires special hardware. + 32 // This test requires special hardware. Looks nicer if there is leading space. test/sun/security/smartcardio/TestConnectAgain.java === - 29 * @run main/manual TestTransmit + 29 * @run main/manual TestConnectAgain Maybe a typo here. Would you please make the update? test/sun/security/ssl/X509TrustManagerImpl/ClientServer.java === - 35 * JSSE supports algorithm constraints with CR 6916074, - 36 * need to update this test case in JDK 7 soon + + 35 * JSSE supports algorithm constraints with CR 6916074, need to + 36 * update this test case in JDK 7 soon Would you mind add a blank line and join the two line accordingly? Otherwise, looks fine to me. Thanks, Xuelei
Re: Bug 8028627
Are you referring to 8014369: Intermittently only some components refresh This was submitted May 2013. For a general non-security bug, how can one add comments and updates? If you are a OpenJDK contributor with a userid, you should be able to add directly to it using the JIRA instance, but if not, then I'd suggest writing to the appropriate group. This is a AWT bug, so probably awt-dev? http://mail.openjdk.java.net/mailman/listinfo Brad On 2/23/2014 10:20 AM, Mickey Segal wrote: There used to be a way to add such comments directly to bug reports. For a general non-security bug, how can one add comments and updates? I have bugs submitted 4 years ago (e.g. http://bugs.java.com/view_bug.do?bug_id=8014369) that are still listed as In progress and I don't see any way to add a comment. Bradford Wetmore wrote: I've added the stack trace to the report.
Re: Bug 8028627
I've added the stack trace to the report. Thanks! Brad On 2/19/2014 9:59 PM, Stefan Liebig wrote: Hi, I would like to add a comment to an existing bug: - http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8028627 - https://bugs.openjdk.java.net/browse/JDK-8028627 The problem described in that bug seems that it has been discovered by statically code analysis. However, it seems that we have this problem in production code. A thread dump shows that two threads are looping: Java HotSpot(TM) Client VM (24.45-b08 mixed mode) pool-2-thread-2 prio=6 tid=0x40537c00 nid=0xb80 runnable [0x4298e000] java.lang.Thread.State: RUNNABLE at java.util.WeakHashMap.get(WeakHashMap.java:471) at javax.crypto.JceSecurity.getCodeBase(JceSecurity.java:222) at javax.crypto.JceSecurityManager.getCryptoPermission(JceSecurityManager.java:107) at javax.crypto.Cipher.getConfiguredPermission(Cipher.java:2503) at javax.crypto.Cipher.initCryptoPermission(Cipher.java:685) at javax.crypto.Cipher.chooseProvider(Cipher.java:848) - locked 0x16005f98 (a java.lang.Object) at javax.crypto.Cipher.init(Cipher.java:1213) at javax.crypto.Cipher.init(Cipher.java:1153) at org.hsqldb.persist.Crypto.init(Unknown Source) at org.hsqldb.persist.Logger.setVariables(Unknown Source) at org.hsqldb.persist.Logger.openPersistence(Unknown Source) at org.hsqldb.Database.reopen(Unknown Source) at org.hsqldb.Database.open(Unknown Source) - locked 0x15e51a60 (a org.hsqldb.Database) at org.hsqldb.DatabaseManager.getDatabase(Unknown Source) - locked 0x15e51a60 (a org.hsqldb.Database) at org.hsqldb.DatabaseManager.newSession(Unknown Source) at org.hsqldb.jdbc.JDBCConnection.init(Unknown Source) ... pool-2-thread-1 prio=6 tid=0x40537400 nid=0x18f4 runnable [0x412fe000] java.lang.Thread.State: RUNNABLE at java.util.WeakHashMap.get(WeakHashMap.java:471) at javax.crypto.JceSecurity.getCodeBase(JceSecurity.java:222) at javax.crypto.JceSecurityManager.getCryptoPermission(JceSecurityManager.java:107) at javax.crypto.Cipher.getConfiguredPermission(Cipher.java:2503) at javax.crypto.Cipher.initCryptoPermission(Cipher.java:685) at javax.crypto.Cipher.chooseProvider(Cipher.java:848) - locked 0x16006128 (a java.lang.Object) at javax.crypto.Cipher.init(Cipher.java:1213) at javax.crypto.Cipher.init(Cipher.java:1153) at org.hsqldb.persist.Crypto.init(Unknown Source) at org.hsqldb.persist.Logger.setVariables(Unknown Source) at org.hsqldb.persist.Logger.openPersistence(Unknown Source) at org.hsqldb.Database.reopen(Unknown Source) at org.hsqldb.Database.open(Unknown Source) - locked 0x15e5a718 (a org.hsqldb.Database) at org.hsqldb.DatabaseManager.getDatabase(Unknown Source) - locked 0x15e5a718 (a org.hsqldb.Database) at org.hsqldb.DatabaseManager.newSession(Unknown Source) at org.hsqldb.jdbc.JDBCConnection.init(Unknown Source) at org.hsqldb.jdbc.JDBCDriver.getConnection(Unknown Source) at org.hsqldb.jdbc.JDBCDriver.connect(Unknown Source) ... We have two database instances running parallel. Tschüß, Stefan - compeople AG Untermainanlage 8 60329 Frankfurt/Main fon: +49 (0) 69 / 27 22 18 0 fax: +49 (0) 69 / 27 22 18 22 web: www.compeople.de Vorstand: Jürgen Wiesmaier Aufsichtsratsvorsitzender: Christian Glanz Sitz der Gesellschaft: Frankfurt/Main Handelsregister Frankfurt HRB 56759 USt-IdNr. DE207665352 -
Re: Code review request, 8028518, Increase the priorities of GCM cipher suites
Looks ok to me, with the exception as you pointed out that this doesn't follow section 4 of RFC 6460. Why was this done, and how did you originally determine the original ciphersuite ordering for GCMs? Brad On 12/29/2013 7:56 PM, Xuelei Fan wrote: Hi, Please review this small update. webrev: http://cr.openjdk.java.net/~xuelei/8028518/webrev.00/ In TLS protocols, cipher suite specifies the crypto algorithms used in TLS connections. The priorities of cipher suites define the preference order that a cipher suite may be used in a TLS connection. When introducing the AEAD/GCM cipher suites in SunJSSE provider (JEP 115)[1], for better compatibility and interoperability, we decided to decrease the priority of cipher suites in GCM mode for a while before GCM technologies mature in the industry. It's time to consider to increase the priorities of GCM mode cipher suite in early stage of JDK 9. Thanks, Xuelei [1] http://openjdk.java.net/jeps/115
Re: Code review request, 8028518, Increase the priorities of GCM cipher suites
On 1/3/2014 6:19 PM, Xuelei Fan wrote: On 1/4/2014 6:41 AM, Bradford Wetmore wrote: Looks ok to me, with the exception as you pointed out that this doesn't follow section 4 of RFC 6460. Sorry, I did not get it. Would you mind point out the line number of the concern? This section in RFC 6460: A Suite B TLS client configured at a minimum level of security of 128 bits MUST offer the TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 or the TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 cipher suite in the ClientHello message. The TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 cipher suite is preferred; if offered, it MUST appear before the TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 cipher suite. You have: 993 add(TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, ... 995 add(TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, Why was this done, and how did you originally determine the original ciphersuite ordering for GCMs? Per RFC 6460, there are two profiles, Suite B Combination 1 and Suite B Combination 2. SunJSSE default cipher suite preference does not compliant to the profiles at present. That's why it is said, The preference order of the GCM cipher suites does not follow the spec of RFC 6460. About the ordering, please refer to line 964-977 of CipherSuite.java My question was, how did you choose the current order (currently lines 1080-1110: TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 TLS_RSA_WITH_AES_256_GCM_SHA384 TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384 TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384 TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 TLS_DHE_DSS_WITH_AES_256_GCM_SHA384 TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 TLS_RSA_WITH_AES_128_GCM_SHA256 TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256 TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256 TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 TLS_DHE_DSS_WITH_AES_128_GCM_SHA256 Brad Thanks, Xuelei Brad On 12/29/2013 7:56 PM, Xuelei Fan wrote: Hi, Please review this small update. webrev: http://cr.openjdk.java.net/~xuelei/8028518/webrev.00/ In TLS protocols, cipher suite specifies the crypto algorithms used in TLS connections. The priorities of cipher suites define the preference order that a cipher suite may be used in a TLS connection. When introducing the AEAD/GCM cipher suites in SunJSSE provider (JEP 115)[1], for better compatibility and interoperability, we decided to decrease the priority of cipher suites in GCM mode for a while before GCM technologies mature in the industry. It's time to consider to increase the priorities of GCM mode cipher suite in early stage of JDK 9. Thanks, Xuelei [1] http://openjdk.java.net/jeps/115
Re: code review request: 8030823 jdk9 version update
Looks good. As I mentioned in another thread, you'll need to wait for Joe's change (JDK-8000962) which I just also reviewed, otherwise when you run the regression tests for the closed, newly built signed jar files, one will fail due to baked-in spec/impl version being off. brad On 12/19/2013 3:52 PM, Anthony Scarpino wrote: This should, hopefully, be a quick and easy review. This is updated the version number for the providers to the new jdk9 gate. 8030823 Security Providers need to have their version numbers updated for JDK9 http://cr.openjdk.java.net/~ascarpino/8030823/webrev.00/ thanks Tony
Re: Code Review request: 8029550 javadoc updates
I really don't think formatting changes as part of other approved changes would be taboo. Joe Darcy has the same opinion. I would restore, but too late now. I knew I should have got to this email earlier. :) One question, I haven't tried this out, but will the simple @since 8 additions clobber all of the existing Description copied from... in methods like getOrDefault() ? If so, then we should probably wait until that bug is fixed. Brad On 12/4/2013 12:32 PM, Anthony Scarpino wrote: Ok.. I've removed the formatting changes and updated the webrev in-place. Tony On 12/04/2013 11:52 AM, Sean Mullan wrote: Looks ok to me. I'm not really sure if code formatting changes are considered a docs change though, I think this is stretching the definition, so I would probably just make the @since changes. --Sean On 12/04/2013 02:14 PM, Anthony Scarpino wrote: Hi, I need a quick review of the below. The changes reflect comments that Brad made about javadoc @since tags not being inherited from the new methods, additionally some formatting changes. No code changes were made 8029550 javadoc since tag and formatting updates for recent Hashtable updates http://cr.openjdk.java.net/~ascarpino/8029550/webrev.00/ Tony
Re: hg: code-tools/jtreg: 7900248: Enhance jtreg to copy/restore the Security Providers when running in agentvm or samevm mode.
Roger, Jon is correct, we had several options for test stabilization, including fixing several dozen tests that were written a decade before -agentvm came into being. See the bug and this external link for more details: http://mail.openjdk.java.net/pipermail/security-dev/2013-November/009701.html We could certainly request such a switch and clean those tests, but with this new JPRT switch for -agentvm, it no longer seems necessary esp. given limited cycles. Brad On 12/3/2013 8:44 AM, Jonathan Gibbons wrote: Roger, That conflicts with the wishes of the security team who requested that jtreg clean up the security providers -- i.e. they don't want their tests to have to worry about it. The security team filed the request for 7900248. So, I think you and the security team need to figure out the coding style and pattern you want and then figure out what jtreg support you need. -- Jon On 12/03/2013 06:47 AM, roger riggs wrote: Hi Jon, I'd like to see a log message flagging a test that does not restore the log providers. Otherwise, we're going to encourage poor hygiene in tests and won't know the extent of the use/misuse. Roger On 12/2/2013 9:09 PM, jonathan.gibb...@oracle.com wrote: Changeset: 8218bff65772 Author:jjg Date: 2013-12-02 18:07 -0800 URL: http://hg.openjdk.java.net/code-tools/jtreg/rev/8218bff65772 7900248: Enhance jtreg to copy/restore the Security Providers when running in agentvm or samevm mode. + make/tests/SecurityProviderTest.gmk ! src/share/classes/com/sun/javatest/regtest/Action.java + src/share/test/javatest/regtest/data/secprov/A.java + src/share/test/javatest/regtest/data/secprov/B.java + src/share/test/javatest/regtest/data/secprov/C.java + src/share/test/javatest/regtest/data/secprov/TEST.ROOT + src/share/test/javatest/regtest/data/secprov/Test.java
Re: Thoughts on possible options to JDK-8027598
jtreg team? I am not aware of one. Jon Gibbons works on this in his spare time. I would call him the JTREG team. Was there an ETA for this? Balancing recent test stabilization efforts with this, you may consider consider adding othervm and then back it out when the JTREG fix is made. Brad On 11/25/2013 2:29 AM, Chris Hegarty wrote: On 22/11/13 21:11, Rajan Halade wrote: On 11/22/2013 11:42, Sean Mullan wrote: On 11/21/2013 04:53 AM, Chris Hegarty wrote: If I am correct, JTREG has support provider cleanup already. But the question is even JTREG reset the providers, it still cannot ensure next test won't be impacted because in some circumstances JDK may need to cache something which depends on previous providers. Still need to analysis the test case by case. Right, any static or cached data may be invalid, and this will require careful changes to the JRE itself. Pardon my ignorance - if I gather correctly then ProvidersSnapshot library also doesn't sandbox effects completely. FYI. I am not part of the security team, and someone from the security group should ultimately have to agree/disagree, but we have similar issues in other parts of the platform. I don't know the specifics of the code here so it may, or may not, be an issue. If any part of the security framework stores values in static fields, that is dependent on the security providers, then when the providers change this static value may be incorrect. We encounter this from time to time with system properties. E.g. JDK HTTP Client code reads system property and stores in static field, JDK HTTP Client will never never re-read the property as it believes it will not change. Having jtreg reset/clean certain system properties will not help in this case. Yes, the example with system properties is a good one. However, providers are designed to be added and removed dynamically (see the java.security.Security API), so if there is some static information not being cleared when providers are reset, I would tend to think it may be a bug in the JDK. OK, if this is the case then that is fine. yes. thanks! My preference would be change jtreg to clear/restore providers, and more thoroughly analyze any subsequent test failures as it may be a bug in the JDK. Ok, I will follow up with jtreg team to find out when they can commit to timeline. jtreg team? I am not aware of one. Jon Gibbons works on this in his spare time. You may want to take a look at jtreg [1] in the code tools project. -Chris. [1] http://openjdk.java.net/projects/code-tools/jtreg/ - Rajan --Sean
Re: Code Review Request: 8025763
Sean wrote: That kind of seems like a javadoc bug to me. Shouldn't it add the @since tag as part of inheriting the javadoc? On the chance that this is a real bug, I filed this yesterday: https://bugs.openjdk.java.net/browse/JDK-8029241 @throws/@since are both missing (others?), it seems more efficient from a developer perspective to simply copy this info to the overridden classes, instead of making the developer drill down to get the rest of the information. Tony wrote: Let me see if setting @since doesn't cause the inheritance of the other tag to stop. If it doesn't I'll fix it for 8. I'm guessing that if you add anything, it's going to suppress the inherited javadoc, so you'll have to wait for JDK-8029241 to be fixed. But for the case of new stuff where you've provided new text, I feel it should have an @since added. And feel free to clean up the 80 char lines! ;) Brad On 11/27/2013 12:46 PM, Anthony Scarpino wrote: On Nov 27, 2013, at 10:12 AM, Sean Mullan sean.mul...@oracle.com wrote: On 11/26/2013 08:20 PM, Bradford Wetmore wrote: Tony, I note the @since's are missing for the new methods, both in the generated output in the overridden methods (i.e. no javadoc), and the methods in which you've changed the behavior (i.e. new javadoc). I'm not sure what you can do about the previous behavior (cc'ing Mike/Sowmya, maybe they know), but what about the new ones? That kind of seems like a javadoc bug to me. Shouldn't it add the @since tag as part of inheriting the javadoc? Anyway, adding @since tags would be considered a docs only change, so Tony you could still fix this for JDK 8 - can you file a bug? Thanks, Sean My first thought was it would be a javadoc issue too. Let me see if setting @since doesn't cause the inheritance of the other tag to stop. If it doesn't I'll fix it for 8. Tony Brad On 10/21/2013 2:48 PM, Sean Mullan wrote: On 10/18/2013 10:52 PM, Anthony Scarpino wrote: I've updated the webrev http://cr.openjdk.java.net/~ascarpino/8025763/webrev.01/ Update looks good. --Sean
Re: Code Review Request: 8021418
Where did this workaround suggestion come from? Please link these two issues (JDK-6978415) together. Have you talked to the network team about this? brad On 11/26/2013 6:09 PM, Xuelei Fan wrote: This change looks fine. JSSE regression tests use a lot of code as new ServerSocket(0), we may want a cleanup for test stabilization. Xuelei On 11/27/2013 9:13 AM, Rajan Halade wrote: May I request you to review this simple change - http://cr.openjdk.java.net/~juh/rajan/8021418/ http://cr.openjdk.java.net/%7Ejuh/rajan/8021418/ The test is modified to set SO_REUSEADDR on ServerSocket to false for stabilization. Thanks, Rajan
Re: Code Review Request: 8025763
Tony, I note the @since's are missing for the new methods, both in the generated output in the overridden methods (i.e. no javadoc), and the methods in which you've changed the behavior (i.e. new javadoc). I'm not sure what you can do about the previous behavior (cc'ing Mike/Sowmya, maybe they know), but what about the new ones? Brad On 10/21/2013 2:48 PM, Sean Mullan wrote: On 10/18/2013 10:52 PM, Anthony Scarpino wrote: I've updated the webrev http://cr.openjdk.java.net/~ascarpino/8025763/webrev.01/ Update looks good. --Sean
Re: Thoughts on possible options to JDK-8027598
On 11/19/2013 4:19 PM, Rajan Halade wrote: I am working towards fixing JDK-8027598 https://bugs.openjdk.java.net/browse/JDK-8027598, there are multiple options available here and would appreciate your thoughts on this. It was filed to address the issue at large reported inJDK-8027526 https://bugs.openjdk.java.net/browse/JDK-8027526. Problem - When a regression test is run in agentvm mode and alters security providers, it can cause adverse effects on next tests executed in the batch. We have been batteling with few intermittent failures which are caused by scenario like this so I think it is important to have this fix. Possible approaches - 1. Enhance JTREG - This option would require change in jtreg to store/restore security providers when run in agentvm mode. That is my preference. I'm also cc'ing Jonathan Gibbons who should also be involved. I believe you can just look to see if the provider list has been updated, then restore only if that is the case. 2. Update impacting tests to run in othervm - simple change but may slow down batch execution slightly. Correct. I am not suggesting the following, but one other thing that was proposed was to update the Makefiles to run the java_security* targets in othervm, but that doesn't work when calling jtreg directly, and impacts all tests in that target. 3. Update each test to use library java/security/KeyPairGenerator/Failover.java like done in java/security/Provider tests - another easy change and tests would continue to run agentvm but would have added overhead of restoring providers. We will continue to pursue option 1 but many not be possible. Option 2 3 above are equally good and are debatable so would like your thoughts on it. I would suggest pursing in order 1, 3, 2. Brad Thanks, Rajan
Re: Thoughts on possible options to JDK-8027598
On 11/19/2013 5:07 PM, Xuelei Fan wrote: On 11/20/2013 8:19 AM, Rajan Halade wrote: I am working towards fixing JDK-8027598 https://bugs.openjdk.java.net/browse/JDK-8027598, there are multiple options available here and would appreciate your thoughts on this. It was filed to address the issue at large reported inJDK-8027526 https://bugs.openjdk.java.net/browse/JDK-8027526. Problem - When a regression test is run in agentvm mode and alters security providers, it can cause adverse effects on next tests executed in the batch. We have been batteling with few intermittent failures which are caused by scenario like this so I think it is important to have this fix. Possible approaches - 1. Enhance JTREG - This option would require change in jtreg to store/restore security providers when run in agentvm mode. If I am correct, JTREG has support provider cleanup already. No, it does not currently. Brad But the question is even JTREG reset the providers, it still cannot ensure next test won't be impacted because in some circumstances JDK may need to cache something which depends on previous providers. Still need to analysis the test case by case. Xuelei 2. Update impacting tests to run in othervm - simple change but may slow down batch execution slightly. 3. Update each test to use library java/security/KeyPairGenerator/Failover.java like done in java/security/Provider tests - another easy change and tests would continue to run agentvm but would have added overhead of restoring providers. We will continue to pursue option 1 but many not be possible. Option 2 3 above are equally good and are debatable so would like your thoughts on it. Thanks, Rajan
Re: RFR: JDK-8027624 - com/sun/crypto/provider/KeyFactory/TestProviderLeak.java unstable again
Looks good, thanks for looking at this, Dan! Brad On 10/31/2013 3:05 PM, Dan Xu wrote: Hi All, Please help review a simple change towards the testcase, TestProviderLeak.java. In the call of SecretKeyFactory.getInstance(), it will try to access files under temporary directory by calling File.list() method. And this list() method may consume a lot of memory if there are large amount of files inside. In such case, it will cause OutOfMemory error and fail the test. As the contents in the tmp directory vary at different time and on different machines, it leads to the fragility of this test. In the change, it moves eatupMemory() method close to the testing iteration to avoid the memory consumption in non-testing codes. Bug: https://bugs.openjdk.java.net/browse/JDK-8027624 Webrev: http://cr.openjdk.java.net/~dxu/8027624/webrev/ http://cr.openjdk.java.net/%7Edxu/8027624/webrev/ Thanks, -Dan
Very simple webrev.
https://bugs.openjdk.java.net/browse/JDK-8027526 http://cr.openjdk.java.net/~wetmore/8027526/webrev/ Alan was getting some test failures, I'm 95% sure it's due to a provider being inserted one of two tests and being run with agentvm. According to Jon Gibbons, agentvm does not reset the Security Providers. I'm putting this one open change back, and will be filing a bug for the remainder. Brad
Re: Very simple webrev.
*facepalm* CR withdrawn...it must be the one in the closed repo then. Thanks Mandy. Brad On 10/30/2013 2:37 PM, Mandy Chung wrote: On 10/30/2013 2:25 PM, Bradford Wetmore wrote: https://bugs.openjdk.java.net/browse/JDK-8027526 http://cr.openjdk.java.net/~wetmore/8027526/webrev/ 32 * @run main/othervm SignatureGetAlgorithm 33 * @author youdwei 34 * @run main/othervm SignatureGetAlgorithm I think you intended to move line 32 rather than copy? Otherwise, looks okay. Mandy
Re: Very simple webrev.
I'm still going to change the name of the provider, the name test was resulting in failure mode: test failed: some condition which of course wasn't obvious where test was coming from. Brad On 10/30/2013 2:45 PM, Bradford Wetmore wrote: *facepalm* CR withdrawn...it must be the one in the closed repo then. Thanks Mandy. Brad On 10/30/2013 2:37 PM, Mandy Chung wrote: On 10/30/2013 2:25 PM, Bradford Wetmore wrote: https://bugs.openjdk.java.net/browse/JDK-8027526 http://cr.openjdk.java.net/~wetmore/8027526/webrev/ 32 * @run main/othervm SignatureGetAlgorithm 33 * @author youdwei 34 * @run main/othervm SignatureGetAlgorithm I think you intended to move line 32 rather than copy? Otherwise, looks okay. Mandy
hg: jdk8/tl/jdk: 8027526: CheckTipsAndVersions.java failing occasionally
Changeset: f731d096530f Author:wetmore Date: 2013-10-30 16:49 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f731d096530f 8027526: CheckTipsAndVersions.java failing occasionally Reviewed-by: mullan, mchung ! test/java/security/Signature/SignatureGetAlgorithm.java
hg: jdk8/tl/jdk: 8026762: jdk8-tl builds windows builds failing in corba - javac: no source files
Changeset: a45acc8de0f3 Author:wetmore Date: 2013-10-16 23:32 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a45acc8de0f3 8026762: jdk8-tl builds windows builds failing in corba - javac: no source files Reviewed-by: katleman, dholmes ! makefiles/GenerateClasses.gmk
hg: jdk8/tl/corba: 8026762: jdk8-tl builds windows builds failing in corba - javac: no source files
Changeset: 1a71d800b032 Author:wetmore Date: 2013-10-16 23:31 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/corba/rev/1a71d800b032 8026762: jdk8-tl builds windows builds failing in corba - javac: no source files Reviewed-by: katleman, dholmes ! makefiles/BuildCorba.gmk
Re: [8] 8026301: DomainKeyStore doesn't cleanup correctly when storing to keystore
Looks ok to me. Brad On 10/11/2013 12:10 PM, Sean Mullan wrote: Looks good, just add a noreg-trivial tag to the bug. --Sean On 10/11/2013 02:41 PM, Vincent Ryan wrote: Please review this fix to close output stream in DomainKeyStore: Bug: https://bugs.openjdk.java.net/browse/JDK-8026301 Webrev: http://cr.openjdk.java.net/~vinnie/8026301/webrev.00/ Thanks.
Re: Code Review Request for 8014374: Cannot initialize AES/GCM/NoPadding on wrap/unseal on solaris with OracleUcrypto
On 9/27/2013 4:49 PM, Valerie (Yu-Ching) Peng wrote: Xuelei, Since the source for OracleUcrypto provider isn't included in OpenJDK, it probably makes more sense to have its regression tests off OpenJDK as well. Please find the changes for the test relocation under 8014374: Webrev: http://cr.openjdk.java.net/~valeriep/8014374/webrev.00/ Sounds good. I see the deleted files, but don't see the new ones in the closed repo yet. Brad
RFR: JDK-8014838: getStrongSecureRandom() should require at least one implementation
This minor suggestion came up in May from our JCK team: https://bugs.openjdk.java.net/browse/JDK-8014838 http://cr.openjdk.java.net/~wetmore/8014838/webrev.00/ JCK suggested all implementations should have at least one strong random implementation. I've also changed the error case to throw NoSuchAlgorithmException instead of returning null. CCC review is also underway concurrently, but I'm not expecting any issues. Brad
Re: Code review request, JKD-8024068 sun/security/ssl/javax/net/ssl/ServerName/IllegalSNIName.java fails
Same.
Brad
On 9/1/2013 7:45 PM, Weijun Wang wrote:
Code change looks fine.
--Max
On 9/2/13 10:41 AM, Xuelei Fan wrote:
On 9/2/2013 10:38 AM, Xuelei Fan wrote:
Hi Weijun,
Please review this simple test fix. There is a typo in the test. The
issue is exposed after the fix of JDK-8023881.
webrev: http://cr.openjdk.java.net/~xuelei/8024068/webrev.00/
This bug may not have been published to bugs.sun.com. The typo and the
patch look like:
String[] illegalNames = {
-example\u3003\u3002com,
+example\u3002\u3002com,
example..com,
com\u3002,
com.,
.
};
Note that \u3002 is a kind of dot code point (ideographic full stop)
per RFC 3490.
Xuelei
\u3003 now is acceptable as a Unicode code point, and then result in
test failure.
Thanks,
Xuelei
Re: RFR JDK 8 doclint fixes in javax.net.ssl
Looks good to me, but Xuelei (owner) might want to have a quick look.
Brad
On 8/23/2013 2:34 PM, Joe Darcy wrote:
Hello,
Please review the patch below which resolves several doclint issues in
javax.net.ssl.
Thanks,
-Joe
diff -r 4ef82445ea20 src/share/classes/javax/net/ssl/SNIHostName.java
--- a/src/share/classes/javax/net/ssl/SNIHostName.javaFri Aug 23
20:59:34 2013 +0200
+++ b/src/share/classes/javax/net/ssl/SNIHostName.javaFri Aug 23
14:33:44 2013 -0700
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -293,6 +293,7 @@
* the a href={@docRoot}/java/util/regex/Pattern.html#sum
* regular expression pattern/a
* representing the hostname(s) to match
+ * @return a {@code SNIMatcher} object for {@code SNIHostName}s
* @throws NullPointerException if {@code regex} is
* {@code null}
* @throws java.util.regex.PatternSyntaxException if the regular
expression's
diff -r 4ef82445ea20 src/share/classes/javax/net/ssl/X509KeyManager.java
--- a/src/share/classes/javax/net/ssl/X509KeyManager.javaFri Aug 23
20:59:34 2013 +0200
+++ b/src/share/classes/javax/net/ssl/X509KeyManager.javaFri Aug 23
14:33:44 2013 -0700
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1999, 2004, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 1999, 2013, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -40,7 +40,7 @@
* UL
* LI determine the set of aliases that are available for negotiations
* based on the criteria presented,
- * LI select the ITALIC best alias /ITALIC based on
+ * LI select the i best alias/i based on
* the criteria presented, and
* LI obtain the corresponding key material for given aliases.
* /UL
Re: Bug in ProcessBuilder.
Martin,
Your fix looks good to me, just need a test case to putback. Should be
pretty straightforward to create a custom SecurityManager that throws a
ACE instead of a SE during a checkRead(), and then link together.
Brad
On 8/21/2013 3:51 PM, Martin Buchholz wrote:
Adding Alexey Utkin, who appears to be the author of the lines I am
proposing to modify. Alexey, you are invited to take ownership of this fix.
On Wed, Aug 21, 2013 at 3:43 PM, Martin Buchholz marti...@google.com
mailto:marti...@google.com wrote:
Hi security team,
There's some code in ProcessBuilder.java to avoid leaking data in
case ProcessBuilder.start fails.
It appears to have an obvious bug, with an obvious fix.
http://cr.openjdk.java.net/~martin/webrevs/openjdk8/ProcessBuilder-checkRead/
checkRead is spec'ed to throw SecurityException, not
AccessControlException. If checkRead does throw SecurityException,
then start will throw the wrong exception.
Untested.
@@ -1033,9 +1033,9 @@
// Can not disclose the fail reason for read-protected
files.
try {
security.checkRead(prog);
-} catch (AccessControlException ace) {
+} catch (SecurityException e) {
exceptionInfo = ;
-cause = ace;
+cause = e;
}
}
// It's much easier for us to create a high-quality error
Re: Code review request - 8022896: test/com/sun/crypto/provider/Cipher/RSA/TestOAEPPadding.java fails
Ditto. brad On 8/19/2013 12:08 PM, Sean Mullan wrote: Looks fine. --Sean On 08/19/2013 02:52 PM, Anthony Scarpino wrote: Hi, I need a very simple review on enabling a test http://cr.openjdk.java.net/~ascarpino/8022896/webrev.00/ thanks Tony
hg: jdk8/tl/jdk: 8019341: Update CookieHttpsClientTest to use the newer framework.
Changeset: 11c15607e43f Author:wetmore Date: 2013-07-05 18:22 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/11c15607e43f 8019341: Update CookieHttpsClientTest to use the newer framework. Reviewed-by: xuelei, smarks, michaelm ! test/sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/CookieHttpsClientTest.java ! test/sun/security/ssl/templates/SSLEngineTemplate.java ! test/sun/security/ssl/templates/SSLSocketSSLEngineTemplate.java ! test/sun/security/ssl/templates/SSLSocketTemplate.java
hg: jdk8/tl/jdk: 8014620: Signature.getAlgorithm return null in special case
Changeset: 116050227ee9 Author:youdwei Date: 2013-06-17 17:36 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/116050227ee9 8014620: Signature.getAlgorithm return null in special case Reviewed-by: wetmore ! src/share/classes/java/security/Signature.java + test/java/security/Signature/SignatureGetAlgorithm.java
hg: jdk8/tl/jdk: 8012530: test/sun/security/provider/SecureRandom/StrongSeedReader.java failing
Changeset: b600d637ef77 Author:wetmore Date: 2013-04-25 17:10 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b600d637ef77 8012530: test/sun/security/provider/SecureRandom/StrongSeedReader.java failing Reviewed-by: wetmore Contributed-by: alan.bate...@oracle.com ! test/sun/security/provider/SecureRandom/StrongSeedReader.java
hg: jdk8/tl/jdk: 8009925: Back out AEAD CipherSuites temporarily
Changeset: 6379415d8fca Author:wetmore Date: 2013-03-12 15:31 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6379415d8fca 8009925: Back out AEAD CipherSuites temporarily Reviewed-by: valeriep ! src/share/classes/com/sun/crypto/provider/TlsKeyMaterialGenerator.java ! src/share/classes/sun/security/internal/spec/TlsKeyMaterialParameterSpec.java ! src/share/classes/sun/security/internal/spec/TlsKeyMaterialSpec.java ! src/share/classes/sun/security/pkcs11/P11TlsKeyMaterialGenerator.java - src/share/classes/sun/security/ssl/Authenticator.java ! src/share/classes/sun/security/ssl/CipherBox.java ! src/share/classes/sun/security/ssl/CipherSuite.java ! src/share/classes/sun/security/ssl/EngineInputRecord.java ! src/share/classes/sun/security/ssl/EngineOutputRecord.java ! src/share/classes/sun/security/ssl/EngineWriter.java ! src/share/classes/sun/security/ssl/Handshaker.java ! src/share/classes/sun/security/ssl/InputRecord.java ! src/share/classes/sun/security/ssl/JsseJce.java ! src/share/classes/sun/security/ssl/MAC.java ! src/share/classes/sun/security/ssl/OutputRecord.java ! src/share/classes/sun/security/ssl/Record.java ! src/share/classes/sun/security/ssl/SSLEngineImpl.java ! src/share/classes/sun/security/ssl/SSLSocketImpl.java ! test/sun/security/ec/TestEC.java ! test/sun/security/pkcs11/fips/CipherTest.java ! test/sun/security/pkcs11/sslecc/CipherTest.java ! test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java - test/sun/security/ssl/javax/net/ssl/TLSv12/ShortRSAKeyGCM.java ! test/sun/security/ssl/sanity/ciphersuites/CipherSuitesInOrder.java ! test/sun/security/ssl/sanity/interop/CipherTest.java ! test/sun/security/ssl/templates/SSLSocketSSLEngineTemplate.java
Re: Code Review Request for 7030966, Support AEAD CipherSuites (JSSE part of JEP 115)
EngineOutputRecord.java
===
294/296: Another great comment. I might suggest reversing the
comments so that the comment about AEAD is in the AEAD arm, and CBC is
outside.
I'm not sure I catch your ideas. ;-) Would you please show me the code?
Just a simple reversal of the lines so that the code you're talking
about is contained in the block that handles it:
if (!writeCipher.isAEADMode()) {
// DON'T encrypt the nonce_explicit for AEAD mode
dstBB.position(dstPos + headerSize);
} // The explicit IV in TLS 1.1 and later can be encrypted.
Hope that's clearer.
Looks like my logic is correct. If the cipher is not AEAD mode, the
explicit IV can be encrypted; (otherwise) if the cipher is AEAD mode,
don't encrypt the nonce_explicit.
if (!writeCipher.isAEADMode()) {
// The explicit IV in TLS 1.1 and later can be encrypted.
dstBB.position(dstPos + headerSize);
} // Otherwise, DON'T encrypt the nonce_explicit for AEAD mode
Good grief. I obviously need more sleep. My apologies. :(
Brad
hg: jdk8/tl/jdk: 7197071: Makefiles for various security providers aren't including the default manifest
Changeset: 940c8cc5a5c4 Author:wetmore Date: 2012-10-23 12:36 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/940c8cc5a5c4 7197071: Makefiles for various security providers aren't including the default manifest Reviewed-by: valeriep, mullan, katleman ! make/com/oracle/security/ucrypto/Makefile ! make/javax/crypto/Defs-jce.gmk ! make/sun/security/ec/Makefile ! make/sun/security/mscapi/Makefile ! make/sun/security/pkcs11/Makefile + test/javax/crypto/sanity/CheckManifestForRelease.java
hg: jdk8/tl/jdk: 8001419: Build the JCE portion of JDK-8000970
Changeset: 13b46e8eda33 Author:ohrstrom Date: 2012-10-23 15:51 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/13b46e8eda33 8001419: Build the JCE portion of JDK-8000970 Summary: Original code done by Fredrik Ohrstrom, separated/pushed by wetmore Reviewed-by: wetmore ! src/share/classes/com/sun/crypto/provider/KeyProtector.java + src/share/classes/com/sun/crypto/provider/SealedObjectForKeyProtector.java
Re: Memory leak fix for: src/solaris/native/com/sun/security/auth/module/Unix.c
Yes, that's what I meant. Just that none of the other functions alloc'd memory internally that you had to manually free. Thanks for checking. Brad On 10/19/2012 5:54 PM, John Zavgren wrote: Brad: I'm not sure that I completely understand your question, because this is C code and there are no objects involved. Nevertheless, I read through the procedures that are defined in this particular file (Unix.c) and balanced their memory allocations (malloc(), calloc()) against their memory deallocations (free()) and they seem to balance out now. John - Original Message - From: bradford.wetm...@oracle.com To: john.zavg...@oracle.com Cc: security-dev@openjdk.java.net Sent: Friday, October 19, 2012 8:11:03 PM GMT -05:00 US/Canada Eastern Subject: Re: Memory leak fix for: src/solaris/native/com/sun/security/auth/module/Unix.c Looks good, taken in isolation. Just to be sure, are there any other methods that might return some object that has to be freed. Brad On 10/19/2012 1:28 PM, John Zavgren wrote: Greetings: The following webrev image contains a fix for a memory leak that occurs in the procedure: Java_com_sun_security_auth_module_UnixSystem_getUnixInfo (JNIEnv *env, jobject obj) in the file: jdk/src/solaris/native/com/sun/security/auth/module/Unix.c http://cr.openjdk.java.net/~khazra/john/8000204/webrev/ The leaked memory is associated with the pointer named groups: gid_t *groups = (gid_t *)calloc(numSuppGroups, sizeof(gid_t)); The procedure in question exits in many places and in every case it's necessary to deallocate this memory. The leak occurred because returns were being made without freeing it. I fixed the leak by modifying the code so that there is a common exit point, that is reached from these same places via goto statements, that performs this common function, immediately before the return statement. Thanks! John Zavgren john.zavg...@oracle.com
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/10/2012 7:54 PM, Xuelei Fan wrote:
No new webrev. I need a review of how to use SNIMatcher. See bellow
inline comments.
On 10/11/2012 7:38 AM, Brad Wetmore wrote:
On 10/10/2012 5:47 AM, Xuelei Fan wrote:
new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/
I guess you didn't need to have me as reviewer before going final with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
javax/net/ssl/SSLSocketFactory.java
===
Change look good.
216: I think you need a period at end of sentence here.
You got the others, but missed this one.
I think we discussed the style sometimes ago. If the sentence does not
start with a capital letter, then it does not need a period at the end
of the sentence. I can see both style in other specs.
Updated to add the period.
Sorry, I meant to say copyright change. Boy, did I goof on that one.
One of the copyright dates wasn't right, but the rest were.
I'll respond to the rest tomorrow. I have a sick PC to diagnose. :(
Brad
231: Might suggest some reason text for the
UnsupportedOperationException.
I think the exception name has say the exception clearly. I think it
does not make sense to add additional message for it. I had a search in
the packages of java.lang and java.uitl, most of the codes (39 of 42)
use the default non-parameter constructor of
UnsupportedOperationException. I will prefer to use the current style.
No problem.
sun/security/ssl/Utilities.java
===
46: Minor comment on method name, you might want to use
addToSNIServerNameList since you are adding.
84: Just wondering why you added this method here? This added a bit of
overhead in extra methods calls (well, maybe not with hotspot unrolling)
and added a few chars to the source. So given that you have added this,
why not update the remainder also?
EngineOutputRecord/InputRecord/OutputRecord.
Good point!
Or do it the way you did. This just adds a little extra source overhead.
See the below comment in SSLSocketImpl.java. If you decide to accept
it, you will want to remove the unmodifiable collection.
See the reply in SSLSocketImpl.java
Ok.
sun/security/ssl/Handshaker.java
291: The change to allow for setting of SNI information has opened up a
race condition. It's probably not too bad for SSLSocket, but might be
more for SSLEngine. When we create ClientHandshaker/ServerHandshakers,
we normally grab all the parameters necessary for the handshaker, and
any future parameter modifications will apply to the next handshake. In
the past, our SNI information would never change, so it wasn't necessary
to pass it in on creation. That assumption no longer holds.
So you could see something like this:
sslEngine.beginHandshake();
SSLParameters sslp = new SSLParameters();
sslp.setHostnames(example.com);
sslEngine.setSSLParameters(sslp);
// do handshake...
and you'll get the new value instead of old.
Good catch!
Updated to store the local server name indication and matchers in
Handshaker.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
Good catch!
sun/security/ssl/HelloExtensions.java
=
423: This behavior is currently underspecified.
I think the behavior is specified in RFC 6066:
... If the server understood the ClientHello extension but
does not recognize the server name, the server SHOULD take one of two
actions: either abort the handshake by sending a fatal-level
unrecognized_name(112) alert or continue the handshake.
As means that the server will try to recognize every type of server
name.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
I may have different options on this point. I think, it must be a
specified
Re: (2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension
On 8/14/2012 7:45 PM, Xuelei Fan wrote: 197: You're not planning to process (e.g. ServerHandshaker/ClientHandshaker.process_message) the consumed handshaking bytes immediately during the createSocket call, are you? You still need to allow the user time to set the socket options/SSLParameters/etc. I was expecting in this method you'd just suck in the consumed bytes into temporary storage, then create/return the socket, and then when the handshaking is started, you then read out from the temporary storage until you run out, then you switch to the Socket's InputStream. You're right. It is allowed to set more options in the returned socket before kick off handshake. 197: This needs some wordsmithing here. This method will produce the SSLSocket that will be consuming this data, so of course it has to be called first. I'm not sure I understand your point. Please comment it again with the revised APIs if you still have concerns. I just didn't understand much of this paragraph. 1. You have to call this method, then set up your parameters, then start your handshaking. So the first half of this sentence doesn't apply. Oh, I know your concerns. What I want to express is that before the calling to method, the caller should not do real handshaking. The logic I concerned looks like: // 1. accept a socket // 2. read ClientHello and reply ServerHello to output stream. // 3. call this method SSLSocket sslSocket = (SSLSocket)sslSocketFactory.createSocket( socke, inputStream, true); because the handshaking has started in step 2, then in step 3, we cannot get a proper SSLSocket. How would the ServerHello be generated until you call this method and then start the handshaking on the returned SSLSocket? You said in a previous internal conversation that SSLExplore would not generate any ServerHello, so this can't be what you mean. Are you talking about layering another SSLSocket over a already layered SSLSocket? 2. consumed network data is resumable wasn't clear either. To me this should mean that you can obtain the data which has already been read from s. Yes, need wordsmithing here. 3. Otherwise, the behavior of the return socket is not defined lost me. Does this mean that that SSLParameters and assorted settings are not otherwise defined? See above example. I think I need the above answered before I can comment further here. I think you could delete this paragraph. From your second email: Thought more about the design, I would have to say that we cannot return the default value in sslParameters.getServerNames(). Otherwise, the following two block of codes look very weird to me: // case one: 1 SSLparameters sslParameters = sslSocket.getSSLParameters(); 2 sslParameters.clearServerName(host_name); 3 MapString, String names = sslParameters.getServerNames(); 4 sslSocket.setSSLParameters(sslParameters); 5 sslParameters = sslSocket.getSSLParameters(); 6 names = sslParameters.getServerNames(); In line 3, the returned map does not contain host_name entry. But in line 6, it may be expected that no host_name in the returned map. But if we want to return default values, line 6 do need to return a map containing host_name. The behavior is pretty confusing. We may want to try avoid the confusion. I'm not following your confusion, it seemed pretty straightforward to me, it works much like CipherSuites. We have a set of ciphersuites which are enabled by default. We can turn some off by using SSLParameters. Expanding a bit on your example here, I'll describe what I think would happen internally/externally: 1SSLSocket sslSocket = mySSLSocketFactory.createSocket( www.example.com, 443); mySSLSocketFactory sets any initial parameters as usual. SSLSocketImpl knows it's connecting to www.example.com and automatically stores host_name - www.example.com in its local host data (map or separate variables). 2 SSLparameters sslParameters = sslSocket.getSSLParameters(); SSLSocketImpl.getSSLParameters() creates a SSLParameters, and sets the hostmap to the one value host_name - www.example.com If the application want to get the default values, they just pull them out of the SSLParameters here 3 sslParameters.clearServerName(host_name); Or sslParameters.setServerName(host_name, null)? User just decided to clear it. Ok, that's what we do. It becomes an empty map in SSLParameters. 4 MapString, String names = sslParameters.getServerNames(); Returns empty Map. As far as good. 5 sslSocket.setSSLParameters(sslParameters); SSLSocketImpl.setSSLParameters is empty, so SSLSocketImpl takes this SSLParameters and as a result, clears it's internal host_name map to null, and thus won't send anything out since it's empty. We have problems here. We need to support that if an application does not specified host_name value, we should use default values. I. SSLParameters sslParameters = new SSLParameters(); II.
Re: (2nd round) Proposed API Changes for JEP 114: TLS Server Name Indication (SNI) Extension
How would the ServerHello be generated until you call this method and then start the handshaking on the returned SSLSocket? One can use SSLEngine.wrap()/unwrap() to generate handshaking messages from socket I/O stream. // accept a socket // read/write the I/O with SSLEngine // call this method Switching between SSLSocket/SSLEngine like that would require intentional misuse of the API. You could make the same claim for regular handshaking. Brad Xuelei You said in a previous internal conversation that SSLExplore would not generate any ServerHello, so this can't be what you mean. Are you talking about layering another SSLSocket over a already layered SSLSocket? 2. consumed network data is resumable wasn't clear either. To me this should mean that you can obtain the data which has already been read from s. Yes, need wordsmithing here. 3. Otherwise, the behavior of the return socket is not defined lost me. Does this mean that that SSLParameters and assorted settings are not otherwise defined? See above example. I think I need the above answered before I can comment further here. I think you could delete this paragraph. From your second email: Thought more about the design, I would have to say that we cannot return the default value in sslParameters.getServerNames(). Otherwise, the following two block of codes look very weird to me: // case one: 1 SSLparameters sslParameters = sslSocket.getSSLParameters(); 2 sslParameters.clearServerName(host_name); 3 MapString, String names = sslParameters.getServerNames(); 4 sslSocket.setSSLParameters(sslParameters); 5 sslParameters = sslSocket.getSSLParameters(); 6 names = sslParameters.getServerNames(); In line 3, the returned map does not contain host_name entry. But in line 6, it may be expected that no host_name in the returned map. But if we want to return default values, line 6 do need to return a map containing host_name. The behavior is pretty confusing. We may want to try avoid the confusion. I'm not following your confusion, it seemed pretty straightforward to me, it works much like CipherSuites. We have a set of ciphersuites which are enabled by default. We can turn some off by using SSLParameters. Expanding a bit on your example here, I'll describe what I think would happen internally/externally: 1SSLSocket sslSocket = mySSLSocketFactory.createSocket( www.example.com, 443); mySSLSocketFactory sets any initial parameters as usual. SSLSocketImpl knows it's connecting to www.example.com and automatically stores host_name - www.example.com in its local host data (map or separate variables). 2 SSLparameters sslParameters = sslSocket.getSSLParameters(); SSLSocketImpl.getSSLParameters() creates a SSLParameters, and sets the hostmap to the one value host_name - www.example.com If the application want to get the default values, they just pull them out of the SSLParameters here 3 sslParameters.clearServerName(host_name); Or sslParameters.setServerName(host_name, null)? User just decided to clear it. Ok, that's what we do. It becomes an empty map in SSLParameters. 4 MapString, String names = sslParameters.getServerNames(); Returns empty Map. As far as good. 5 sslSocket.setSSLParameters(sslParameters); SSLSocketImpl.setSSLParameters is empty, so SSLSocketImpl takes this SSLParameters and as a result, clears it's internal host_name map to null, and thus won't send anything out since it's empty. We have problems here. We need to support that if an application does not specified host_name value, we should use default values. I. SSLParameters sslParameters = new SSLParameters(); II. sslParameters.setCipherSuites(...); III. SSLSocket sslSocket = sslSocketFactory.createSocket(www.example.com, 443) IV. sslSocket.setSSLParameters(sslParameters); Before line IV and after line II, the sslParameters.getServerNames() are empty. In line IV, we need to make sure the internal host_name, www.example.com is used as default value, and send it to server in SNI. That's the default behaviors in JDK 7. We cannot break it without strong desires. I think it means that we cannot clear the internal host_name when the sslParameters.getServerNames() return empty. Does it make sense to you? Ok, that's an issue, I was assuming people would generally get a SSLParameters from the SSLSocket/SSLEngine, which would have prepopulated values such as CipherSuites/Protocols and SNI values. Now I hear what you're saying. So in SSLSocket/SSLEngine, we have a very similar situation with the CipherSuites/Protocols. The way we did it there was if SSLParameters.getCipherSuites() is null (that is, has not been set in this instance), we let the default values stand (that is we did not call SSLSocket.setEnabledCipherSuites()). You could do something like that with the SNI info. I've always felt the setServerName/getServerName should take/return the same
Re: Code review request, 7145960 sun/security/mscapi/ShortRSAKey1024.sh failing on windows
Looks good to me also. Brad On 5/17/2012 6:24 AM, Vincent Ryan wrote: Your fix looks fine. However you don't need to modify the file-separator on Windows - both forms are supported in pathnames. On 05/17/12 01:54 PM, Xuelei Fan wrote: Hi, Please review the fix of test cases in Windows. webrev: http://cr.openjdk.java.net./~xuelei/7145960/webrev.00/ Cause of the issue: when keytool cannot generate key pairs in Windows system, we should exit the testing. Thanks, Xuelei
hg: jdk8/tl/jdk: 7167362: SecureRandom.init should be converted, amendment to 7084245
Changeset: 6438f1277df6 Author:wetmore Date: 2012-05-09 16:33 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6438f1277df6 7167362: SecureRandom.init should be converted, amendment to 7084245 Reviewed-by: sherman ! src/share/classes/sun/security/provider/SecureRandom.java
Re: code review request, CR 7166570: JSSE certificate validation has started to fail for certificate chains
Hi Xuelei, Can you put these CR requests on cr.openjdk.java.net? I can't see them from home. Brad On 5/5/2012 8:06 AM, Xuelei Fan wrote: The webrev URL should be: http://javaweb.us.oracle.com/~xufan/bugbios/7166570/webrev.00/ Xuelei On 5/5/2012 9:38 PM, Xuelei Fan wrote: Hi, Please review the fix for JSSE certification path validation issue. webrev: http://javaweb.us.oracle.com/~xufan/bugbios/7166570/webrev/ Cause of the issue: in SimpleValiadtor, the count pathLenConstraint was not calculated properly, the non-intermediate certificate (end-entity certificate) was count in. Thanks, Xuelei
hg: jdk8/tl/jdk: 7157903: JSSE client sockets are very slow
Changeset: 10480cf00dcd Author:wetmore Date: 2012-04-11 17:12 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/10480cf00dcd 7157903: JSSE client sockets are very slow Reviewed-by: xuelei ! src/share/classes/sun/security/ssl/AppOutputStream.java ! src/share/classes/sun/security/ssl/EngineOutputRecord.java ! src/share/classes/sun/security/ssl/OutputRecord.java ! src/share/classes/sun/security/ssl/SSLSocketImpl.java
hg: jdk8/tl/jdk: 7142172: Custom TrustManagers that return null for getAcceptedIssuers will NPE
Changeset: 7a7dcbbd610f Author:wetmore Date: 2012-03-30 15:43 -0700 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7a7dcbbd610f 7142172: Custom TrustManagers that return null for getAcceptedIssuers will NPE Reviewed-by: xuelei ! src/share/classes/sun/security/ssl/SSLContextImpl.java + test/sun/security/ssl/com/sun/net/ssl/internal/ssl/SSLContextImpl/NullGetAcceptedIssuers.java
hg: jdk8/tl/jdk: 7142509: Cipher.doFinal(ByteBuffer, ByteBuffer) fails to process when in.remaining() == 0
Changeset: b16cbeb0d213 Author:wetmore Date: 2012-02-10 19:07 -0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b16cbeb0d213 7142509: Cipher.doFinal(ByteBuffer,ByteBuffer) fails to process when in.remaining() == 0 Reviewed-by: valeriep ! src/share/classes/javax/crypto/CipherSpi.java + test/javax/crypto/CipherSpi/DirectBBRemaining.java
