Re: TLS ALPN Proposal v5

[Bradford Wetmore]

On 9/23/2015 2:33 AM, Simone Bordet wrote:

Hi,

On Wed, Sep 23, 2015 at 7:04 AM, Bradford Wetmore
 wrote:



This new proposal still requires that ciphers are sorted in a way that
matches the ApplicationProtocol order.
Would be nice if, along with the HTTP/2 blacklist, there is a HTTP/2
comparator that sorts ciphers putting the blacklisted ones at the end.


Hm...is the sample code at the end of the initial class description
insufficient?  Adding a comparator seems a little heavyweight in that it
could require access to the ciphersuite internals and would add a lot of
complexity for this one known use case.  When TLSv1.3 is done, the blacklist
stuff in HTTP/2 goes away.


Sure, but until TLS 1.3 widely deployed, applications will have to
sort the ciphers to put HTTP/2 ones before the blacklisted ones.
Providing this comparator is as trivial as providing
ApplicationProtocol.HTTP2BLACKLIST, so I thought to mention it.


I learned something today: Collections/Arrays.sort()/others are stable. 
 (i.e. equal elements will not be reordered as a result of the sort) 
My expectation of "equals" was .equals(), not return value == 0.


I was concerned that such a Comparator might reorder valid H2 suites 
from what was passed in.  Thankfully, that's not the case, so I've added 
this Comparator.  There is a warning now about "consistency with 
equals()", as the Strings obvioulsy won't be equals() and thus sorted 
sets/maps just won't work.  (See the Comparator pages for further 
discussion.)



I also don't understand why there are 2 methods for the protocol name
? What value does it bring to have 2 methods for the same thing ?


Please see the IANA registry:

http://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids

for RFC 7301:

 http://www.rfc-editor.org/info/rfc7301

getProtocolName() is the IANA/IETF textual representation of the protocol
name (i.e. "Protocol" column), for example "HTTP/1.1", "SPDY/3", and "HTTP/2
over TLS".  I suppose toString() could be used instead, but thought it might
eventually output additional ALPN value state.  I don't have any concrete
plans at this point.

getNetworkSequence() is the identification sequence for the protocol (i.e.
"Identification Sequence" column), and represents the actual byte
identifiers that will travel the network in an ALPN extension.

 0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31 ("http/1.1")
 0x73 0x70 0x64 0x79 0x2f 0x33 ("spdy/3")
 0x68 0x32 ("h2")

When client wants to send the extension over the network, it grabs the
ApplicationProtocols values from the SSLParameters, then calls
getNetworkSequence() on each ApplicationProtocol to obtain the actual opaque
ProtocolName(1..2^8-1) to send.  Likewise on the server side, we match the
incoming active ALPN opaque values with the list of mutually agreeable ALPN
values.  And of course, send back the final selected value.


Sure, but application will have to implement two methods instead of
one, and AFAIU the JDK implementation is never calling
getProtocolName() since it's just a description for humans.


I think that a textual name will be better than:

// Output:  javax.net.ssl.ApplicationProtocol$1@1b9e1916

System.out.println(ApplicationProtocol.H2);

and there's no UTF-8 ambiguity.


I've updated the webrev to include an SSLSocket test variant, and added a
few more comments.

 http://cr.openjdk.java.net/~wetmore/8051498/webrev.14/

Hopefully things are more clear now.  Thanks for your review/comments.


I see now, thanks for the pointers !

Indulge me a bit more below on the Map passed as parameter to
ApplicationProtocol :)

IIUC, by the time we are executing the code that calls
ApplicationProtocol.match(), the TLS protocol is already chosen and
it's available in SSLSession.


Not necessarily.  This could also be called before the connection has 
even been attempted, say if the client wants to determine if the 
proposed protocols or protocol/ciphersuites it wants to use are even 
allowed by an ApplicationProtocol.



When remains is the transient value of cipher that is being chosen.
Because we already have modified the API to support the application
protocol transient value (by adding
SSLEngine.getHandshakeApplicationProtocol()) to be used by
KeyManagers, I was wondering if we cannot either:


CipherSuite is more of a Session value, so it should probably be part of 
the handshakeSSLSession.  We could set 
handshakeSSLSession.getCipherSuite() before calling the ALPN selector, 
or pass it in as part of the Map.


> A) add: String SSLEngine.getHandshakeCipherSuite(), to be used by
> ApplicationProtocol

That's kind of what we are doing already, but just in the 
ApplicationProtocol matcher instead so that it doesn't add extra methods 
to SSLSocket/SSLEngine.


And this doesn

Re: TLS ALPN Proposal v5

[Bradford Wetmore]

> This new proposal still requires that ciphers are sorted in a way that
> matches the ApplicationProtocol order.
> Would be nice if, along with the HTTP/2 blacklist, there is a HTTP/2
> comparator that sorts ciphers putting the blacklisted ones at the end.

Hm...is the sample code at the end of the initial class description 
insufficient?  Adding a comparator seems a little heavyweight in that it 
could require access to the ciphersuite internals and would add a lot of 
complexity for this one known use case.  When TLSv1.3 is done, the 
blacklist stuff in HTTP/2 goes away.


> I don't like the first parameter of ApplicationProtocol.match() to be
> a Map;

I personally don't like the Map either, but for 
situations where we don't have a connection yet (e.g. trying to reduce 
the set of protocols and/or ciphersuites for setEnabledCiphersuites()), 
this worked.


Xuelei suggested the String->String map for future expansion, making it 
easier to add new parameters in between major releases if new ALPN 
protocols are introduced.  (For those who aren't aware, adding 
classes/methods/variables/etc. is pretty much forbidden in update 
(minor) releases, but simply adding a new string to a Map is generally OK.)


> I would prefer a full blown class at this
> point, that contains all the parameters, for example:
>
> inner class ApplicationProtocol.Info
> {
>  tlsProtocol
>  cipher
>  sslEngine
>  sessionIsResuming
>  etc.
> }

The other problem here is having a way to get at all the parameters of a 
connection.  Enumerating them all in such a class is essentially 
duplicating methods that are already currently available in the 
SSLSession (handshakeSession) + SSLSocket/SSLEngines classes.  Any 
future enhancements to SSLSocket/SSLEngine would have to be added here 
again in such a class.


> I also don't understand why there are 2 methods for the protocol name
> ? What value does it bring to have 2 methods for the same thing ?

Please see the IANA registry:


http://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids

for RFC 7301:

http://www.rfc-editor.org/info/rfc7301

getProtocolName() is the IANA/IETF textual representation of the 
protocol name (i.e. "Protocol" column), for example "HTTP/1.1", 
"SPDY/3", and "HTTP/2 over TLS".  I suppose toString() could be used 
instead, but thought it might eventually output additional ALPN value 
state.  I don't have any concrete plans at this point.


getNetworkSequence() is the identification sequence for the protocol 
(i.e. "Identification Sequence" column), and represents the actual byte 
identifiers that will travel the network in an ALPN extension.


0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31 ("http/1.1")
0x73 0x70 0x64 0x79 0x2f 0x33 ("spdy/3")
0x68 0x32 ("h2")

When client wants to send the extension over the network, it grabs the 
ApplicationProtocols values from the SSLParameters, then calls 
getNetworkSequence() on each ApplicationProtocol to obtain the actual 
opaque ProtocolName(1..2^8-1) to send.  Likewise on the server side, we 
match the incoming active ALPN opaque values with the list of mutually 
agreeable ALPN values.  And of course, send back the final selected value.


> RFC 7301 hints that the bytes to send over the network are the UTF-8
> bytes from that string.

Intentional, this ApplicationProtocol class and these two contained 
values eliminate that problem.  There is no UTF-8 to network byte 
conversion needed.  The network sequence is hard-coded into the 
ApplicationProtocol and is used directly as the network bytes.  The 
protocolName string is for user clarity.


> There are still a lot of details missing, such as where is the chosen
> ALPN value stored

In SSLSocket/SSLEngine.

If the connection is in the middle of a handshake and you need the 
handshake value:


getHandshakeApplicationProtocol()

Once the connection has finished handshaking, you can get the 
final/active value:


getApplicationProtocol()

The ALPN value is no longer in SSLSession, since it needs to be 
per-connection, not per session.


>  (and how it can be retrieved by the KeyManager, for
> example),

JSSE makes calls to X509KeyManagers (for SSLSockets) and 
X509ExtendedKeyManager (for SSLEngines).  The chooseServerAlias methods 
are passed SSLSocket/SSLEngines for the connection being negotiated.


As above, SSLSocket/SSLEngine now have:

getHandshakeApplicationProtocol();  // if handshaking
getApplicationProtocol();   // if not handshaking

So if chooseServerAlias is called, we're in the middle of a handshake, 
and can get the ALPN value thusly:


chooseClientAlias(String[] keyType,
Principal[] issuers, Socket socket) {

...deleted...

ApplicationProtocol alpn =
socket.getHandshakeApplicationProtocol();

...deleted...

> as well as the webrev not showing any real implementation,

Yes, I'm using pseudo-code at this po

TLS ALPN Proposal v5

[Bradford Wetmore]

Hi all,

On 9/7/2015 7:18 AM, Simone Bordet wrote:

On Mon, Sep 7, 2015 at 5:54 AM, Bradford Wetmore
 wrote:



In general, if the ciphersuites aren't ordered properly, that should be
considered a configuration error on the part of the application(s). However,
this dynamic ALPN selection approach still allows for appropriate ALPN
values to be selected for each possible ciphersuite. That is, if the suites
were ordered H1/H2, the ALPN value should be H1.

For an example of such a selector, please see the new H2/H2+H1/H1
ApplicationProtocolSelector implementions in
ApplicationProtocolSelector.java, and how they are called in
ServerHandshaker.java.


Here is the latest:

http://cr.openjdk.java.net/~wetmore/8051498/webrev.09


Are you sure this is the latest code ?
The relevant changes in ServerHandshaker are commented out.


Yes, sorry for not making this clear.  We've had a lot of internal 
discussions about the API and how to implement it:  the comments in the 
implementation classes (sun.security.ssl) are primarily for Vinnie who 
has been working on the implementation.  But I thought they provide some 
explanation as to how things might be implemented internally so I left 
them in the webrev.



I have the feeling that ApplicationProtocolSelector (APS) is not
really an application protocol selector, but a cipher suite selector.


It was *(note past tense)* primarily an application protocol selector. 
The fact that it could also influence ciphersuite selection was an 
optimization to avoid using suites known not to work with a particular 
ALPN value, but were going to be attempted anyway (say due to a server 
misconfiguration).


However, that has changed as the API has taken a different direction.

You mentioned "session resumption" in your email, which reminded me that 
ALPN values should be attached to connections (i.e. 
SSLSocket/SSLEngine), not SSLSessions.  That simplified things in 
several areas, especially in the Protocol Selector.


We no longer have an ApplicationProtocolSelector, but rather an 
interface called ApplicationProtocol.  These are concrete 
implementations of each ALPN/NPN value, and are responsible for 
examining the state of the connection so far and to return true if the 
conditions are right.   These instances are passed as 
List to SSLParameters and then to 
SSLSocket/SSLEngines.  We have a similar but somewhat simpler server 
selection algorithm than what was described earlier:


Select TLS version

for each ciphersuite

load ALPN query with:
proposed TLS version + proposed ciphersuite
SSLSocket/SSLEngine + handshaking state

for each ALPN value
if (ALPN.matches())
temporarily assign ALPN to the
handshaking SSLSocket/SSLEngine

approve ciphersuite
calls KeyManager for alias
(can use ALPN value above)
checks cipher/MAC restrictions, etc
break;

if no errors
assign ciphersuite to handshakeSession
assign ALPN to SSLSocket/SSLEngine
assign handshakeSesion as the actual Session
to SSLSocket/SSLEngine


APS seem to conflate cipher suite selection with application protocol
selection: the return value of select() actually is a 2-tuple that
means (ciphers[0], protocol) so that returning the empty string or
null does not have any effect on protocol selection, but only on
cipher selection.


To restate the point I think you raising, "Should the ALPN selection 
mechanism influence the TLS protocol version selection mechanism?"


We had a similar discussion internally.

Why would we want to encourage/enable the selection of downrev'd TLS 
protocol versions based on ALPN values?  If we actually support the 
higher/stronger TLS version, IMHO we should be using them.


1.  I think the stronger security benefits from the later protocols 
should always take priority over ALPN values and that we shouldn't be 
encouraging downrev'ing at all. For example, if there is a 
TLSv1.2/1.1/1.0 server, and we negotiated down to 1.0 just to support 
some ALPN value (or due to a bug in the selector), we lose GCM (v1.2) 
and the explicit IV (v1.1). ALPN values are application values, which I 
would expect to not be as security sensitive as the protocol values.


2.  The server hello's protocol version is supposed to be the lower of 
that suggested by the client in the client hello and the highest 
supported by the server.  Returning a lower value just to satisfy an 
ALPN selection would indicate to anyone listening (proxy, other 
connection from same VM?) that the higher TLS values are not supported 
on this server, which I think is incorrect. Future connections to this 
server might cache this lower protocol value and lose the stronger 
protocols.


3. I would expect that "older" ALPN values will likely still be 
supported in later versions of the TLS protoco

Re: [9] request for review: 8136534: Loading JKS keystore using non-null InputStream results in closed stream

[Bradford Wetmore]

Line 257 looks long.  Not sure why you changed it.

Otherwise, looks good.

Brad


On 9/16/2015 12:12 PM, Vincent Ryan wrote:

Please review this JDK 9 fix to stop KeyStore.load from closing the input 
stream passed to it.
Thanks.

Bug: https://bugs.openjdk.java.net/browse/JDK-8136534
Webrev: http://cr.openjdk.java.net/~vinnie/8136534/webrev.00/

Re: RFR: JDK-8015388 : Required algorithms for JDK 9

[Bradford Wetmore]

I just reviewed the new TrustManagerFactory, looks ok to me too.

Brad


On 9/15/2015 4:43 AM, Sean Mullan wrote:

On 09/04/2015 08:32 PM, Xuelei Fan wrote:

Looks fine to me.


One additional change. I have added PKIX as a required algorithm for
TrustManagerFactory. Please review the updated webrev:

http://cr.openjdk.java.net/~mullan/webrevs/8015388/webrev.01/

Thanks,
Sean



Xuelei

On 9/4/2015 11:51 PM, Sean Mullan wrote:

The JDK includes a list of required security algorithms that all
implementations must support:
http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#impl



This list is reviewed before each major release to check if new
algorithms should be added (or existing algorithms removed).

For JDK 9, we are proposing to add several new algorithms and keysizes
that are recommended by standards bodies for cryptographic operations
and security protocols. Adding these as requirements ensures that Java
applications can depend on them to be available on all Java 9
implementations. The new requirements are:

1. Signature: SHA256withDSA
2. KeyPairGenerator: DSA (2048), DiffieHellman (2048, 4096), RSA (4096)
3. AlgorithmParameterGenerator: DSA (2048), DiffieHellman (2048, 4096)
4. Cipher: AES/GCM/NoPadding (128), AES/GCM/PKCS5Padding (128)
5. SSLContext: TLSv1.1, TLSv1.2

webrev: http://cr.openjdk.java.net/~mullan/webrevs/8015388/webrev.00/

Thanks,
Sean

Re: [9] RFR: 8048356: SecureRandom default provider tests

[Bradford Wetmore]

I might also suggest grabbing a byte from the various implementations to 
ensure they return working implementations.


You should also include a comment which mentions that this is an 
implementation-specific test for the Oracle & stock OpenJDK 
implementations, and that other implementations might use other 
SecureRandom implementations in which case this test would need to be 
adjusted.


Does that make sense?

Brad


On 9/4/2015 1:43 PM, Rajan Halade wrote:

Please help with your review of this new test to check default provider
used with SecureRandom.

Bug: https://bugs.openjdk.java.net/browse/JDK-8048356
Webrev: http://cr.openjdk.java.net/~rhalade/8048356/webrev.00/

Thanks,
Rajan

TLS ALPN Proposal v4

[Bradford Wetmore]

Hi all,

Simone/Xuelei/I 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.

What are the behaviors of other vendors?  Can we ask for a clarification
from both HTTP/2 and TLS WG?


and then Simone wrote:

I support Xuelei in that you should ask confirmation to the HTTP/2
editor.


Thanks for the encouragement, Simone.  As you've probably heard, I did 
contact the IETF HTTP/2 working group and posed several questions about 
expected usage[1].  I'm combining those responses plus several other 
points that came up since the last discussion.  This may be repeating 
some things already said, but serves as useful background info for those 
here that might not be following closely:


1.  HTTP/2 (aka H2) and TLSv1.3 were developed in parallel.  The H2 
designers wanted the ciphersuite restrictions in the proposed TLSv1.3. 
But since TLSv1.3 wasn't ready, they compromised by allowing TLSv1.2 to 
be used but with the restriction that only those ciphersuites that were 
expected/allowed for TLSv1.3 could be used.  That is, there is a 
blacklist of ciphersuites for TLSv1.2:  if a suite is present, it can't 
be used in TLSv1.2.  (e.g. no RSA KeyExchange, no CBC-based block 
ciphers, no stream ciphers, etc.)


2. RFC 7301 says in a couple places that it would be advantageous to 
have the chosen ALPN value used by the certificate selection mechanism 
(See sections 1 & 4).  Without a radical rewrite of the current 
selection mechanism (n-tuple), that means ALPN selection should be done 
before the ciphersuite is selected.  We could also check after, but I 
have a different approach (see below).


3. From the H2 working group discussion, a server instance will very 
likely support both H2 and legacy HTTP/1.1. This means for servers that 
prefer H2, any iterative cipher selection mechanisms needs to try the 
H2-specific ciphersuites first, then legacy non-H2 suites.  That is, the 
suites must be ordered appropriately so that the ciphersuite selection 
mechanism won't attempt a blacklisted suite before exhausting all 
H2-acceptable suites.  This ordering can be requested today in JSSE by 
the server calling sslParameters.setUseCipherSuitesOrder(true).  This 
particular point won't matter when TLSv1.3 is in play, as we wouldn't 
try those suites at all.


4. Clients may not know whether a server will be H2 or HTTP/1.1, so they 
should also appropriately sort ciphersuites based on their ALPN 
preferences.  (H2 first, H1 second.)


5. For our SunJSSE, while I think our current enabled list order is 
generally ok, we should probably reorder the ciphersuite priorities so 
that the TLSv1.3 acceptable suites are up front, with the others 
following. This prefers forward secrecy ciphersuites to our current 
ordering. I am thinking we should probably do this for JDK 9, and maybe 
backport as well. The current webrev (link below) doesn't have this yet.


6.  To avoid downgrade attacks, applications should not provide for a 
fallback mechanism.  This includes ALPN selection.


Connection#1:  {"h2", "http/1.1"} // Don't make two connections.
Connection#2:  {"http/1.1"}

POODLE was a good example where allowing fallbacks bit hard.

Of course, we can't control this at the JSSE layer, it's the application 
layer responsibility.



Tradeoff between A) change radically the OpenJDK implementation to
support an iterative processing of TLS protocols, extensions (all of
them), ciphers, aliases, etc. that would be future proof (if that is
even possible) and B) hack in just enough of what is needed to support
H2 today (for which we already have working examples) ?


Given where we are now schedule wise (integration currently due at the 
end of September), and that SunJSSE is such an iterative implementation, 
coming up with a multi-selector API is likely beyond what we can do at 
this point.


(webrev link below).

IMHO, the following works well.  I've added a new method that contains 
the ordered list of ciphersuites still to be tried which is a hint for 
ALPN selection, but we delay the actual ciphersuite selection until 
after the ALPN value is chosen.  So the algorithm is now:


0.  Applications (especially server) might order suites with h2
first for TLSv1.2. sslParameters.setUseCipherSuitesOrder(true)
should be called on the server to ensure those suites are
tried first.

1.  Start Handshake.

2.  Internal server handshaker chooses the TLS version.

3.  The internal server handshaker finds the client/server/protocol
version intersection of the suites, loads the initial ordered
list into a new method on a SSLSession (obtained by the
getHandshakeSSLSession()), then iterates through the
ordered list of ciph

Re: RFR(XS): 8132551: Initialize local variables before returning them in p11_convert.c

[Bradford Wetmore]

Looks good.  Thanks.

Brad


On 8/6/2015 7:53 AM, Volker Simonis wrote:

Hi,

can somebody please review these trivial change which initializes
local variables before they are returned as return values:

http://cr.openjdk.java.net/~simonis/webrevs/2015/8132551
https://bugs.openjdk.java.net/browse/JDK-8132551

Funny enough, this was objected by the Visual Studio compiler in the
slowdebug build only but not in the product or debug builds.

Thank you and best regards,
Volker

Re: RFR: (s) 8130696: Security Providers need to have their version numbers updated for JDK 9

[Bradford Wetmore]

I had done a preliminary review earlier, and the current changes look good.

Brad


On 7/7/2015 10:48 AM, Iris Clark wrote:

Hi.

Please review changes to resolve the following bug:

8130696: Security Providers need to have their version numbers updated
for JDK 9 (Verona)

Bug: https://bugs.openjdk.java.net/browse/JDK-8130696

Changeset:  http://cr.openjdk.java.net/~iris/verona/8130696/webrev

The constructor invocation for every Provider implementation contains a
hard-coded value for the version, "1.9d", which it compares to
java.specification.version.  Verona [0,1] changes the system property
from "1.9" to "9".  The provider’s version should be changed to “9.0d”.

Many regression tests in the jdk repository fail due to this bug.  Test
groups with failing tests detecting this problem include jdk_security,
jdk_management, jdk_jmx, and jdk_net.  These tests now pass.  I have
verified that all providers modified by 8030823 [2] have been updated.

After review, the changeset will be pushed to verona/stage [3]. The
changeset will go to jdk9/dev when Verona is complete later this summer.

Thanks,

Iris

[0] http://openjdk.java.net/projects/verona

[1] http://openjdk.java.net/jeps/223

[2] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/f1066af06fa0

[3] http://hg.openjdk.java.net/verona/stage

Re: TLS ALPN Proposal v3

[Bradford Wetmore]

Ok, I'll check with the HTTP/2 group tomorrow.  It appears the proper 
list is:


ietf-http...@w3.org

Is that correct?

Brad



On 7/9/2015 8:29 AM, Simone Bordet wrote:

Hi,

On Thu, Jul 9, 2015 at 1:42 AM, Bradford Wetmore
 wrote:

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.


I suggest to rename

ExtendedSSLSession.getRequestedApplicationProtocols()

to

ExtendedSSLSession.getPeerApplicationProtocols()

The protocols are not really "requested", they are more "offered", but
IIUC the reason to add "Requested" to this method is to distinguish it
from SSLParameters.getApplicationProtocols() which returns the local
protocols, and in that spirit I think "Peer" is better for
ExtendedSSLSession.


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.


Well, to be precise, either the application protocol is negotiated
after the cipher (and you need cipher sorting to influence the cipher
selection towards the application protocol you would like to choose),
or it must happen at the same time - that is, cipher and application
protocol must be chosen at the same time - but this implies that the
action of choosing that tuple may be invoked multiple times with the
current OpenJDK implementation.

Note that I don't know if the fact that cipher selection is an
iterative process is an OpenJDK implementation detail.
If other implementations are not iterative, then perhaps they have a
single moment where the tuple is chosen.

I support Xuelei in that you should ask confirmation to the HTTP/2 editor.
Also, remember that Firefox, Chrome, OpenSSL, nghttp2, etc. are all
open source and their code is available to verify the behavior.


IIUC, the HTTP/2 blacklist is just strongly recommended ("...SHOULD NOT use
any of the cipher suites...black list"), but not required.  Such potential
peers must also support such a configuration, but in general, it will not.
See section 9.2.2.  I think it's still considered compliant to the spec tho.


 From experience, if a server breaks this SHOULD NOT, it won't work
with any browser.
We had our share of pain trying to figure out interoperability with
browsers for Jetty :)
Sure, it's a SHOULD NOT, but it's like a MUST NOT if you want a
browser to talk to a Java server (or a Java client to talk to a
current HTTP/2 server).


Simone wrote two different ways to do selection:


1) ... so that TLS protocol, cipher (possibly the alias too) and
application protocol are chosen together, or
2) we separate the TLS protocol and
cipher negotiation (and alias) in one step, and we perform application
protocol selection afterwards.


Rather than completely redo the JSSE selection mechanism with the
(version/ciphersuite/ALPN)-tuple idea (which would be a much more involved
API and behavior change), I think the more straightforward solution (2) is
better.


That's what we do in Jetty's ALPN implementation too.
Be aware that it rules out some possibility such as those mentioned by
Michael from RFC 7301.


Also, it is critical to detail how the mechanism work.

Example implementations for SSLFunction would be great to have: the
typical HTTP/2 case is to select the application protocol based on the
TLS protocol and the already negotiated cipher.


I have a "working" test example which shows how the ALPN APIs can be used
for HTTP/2 clients and servers. It is a minor configuration tweak to the
jdk/test/javax/net/ssl/templates/SSLEngineTemplate.java test that we use as
the basis for JSSE SSLEngine testing.


Do you have a Java 9 server that can negotiate h2 with current browsers ?


http://cr.openjdk.java.net/~wetmore/8051498/webrev.04/test/javax/net/ssl/ALPN/SSLEngineAlpnHttp2.java.html

See the configuration in createSSLEngines() around line 265 and 280.


http://cr.openjdk.java.net/~wetmore/8051498/webrev.04/test/javax/net/ssl/ALPN/Http2ApplicationSelector.java.html

Note the HTTP/2 blacklist and reordering code.

The code is not actually "working" yet (haven't merged API/impl repos yet),
but shows how to configure/use this API.


Just a reminder that the cipher blacklist is only valid for TLS 1.2.
For example, if the negotiated TLS protocol is 1.3, there is no need
to look at the ciphers (nor to sort them).

Thanks !

Re: Code Review Request: JDK-8130460 Increase the stability of DTLS test CipherSuite.java

[Bradford Wetmore]

Looks good.

Brad


On 7/8/2015 4:24 PM, Xuelei Fan wrote:

ping ...

Xuelei

On 7/6/2015 9:11 AM, Xuelei Fan wrote:

Hi,

Please review this simple fix.

http://cr.openjdk.java.net/~xuelei/8130460/webrev.00/

The DTLS test, javax/net/ssl/DTLS/CipherSuite.java, failed in nightly
regression intermittent.  In the test, it only allows 10 times app
packet loss and 60 times handshake packet loss.  This value is
reasonable in general, but may be not big enough if the system
networking is heavy loaded.  With this update, I plan to increase the
packet loss limitation in order to increase the stability of the related
tests.

Thanks,
Xuelei

TLS ALPN Proposal v3

[Bradford Wetmore]

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 List 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 c

Re: TLS ALPN Proposal v2

[Bradford Wetmore]

y use
Oracle standard names documentation.


This is not really 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 List 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

[Bradford Wetmore]

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()}).
   *   
   *   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

[Bradford Wetmore]

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 List 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

[Bradford Wetmore]

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
 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();
 List 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();
 List 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

Re: TLS ALPN Proposal

[Bradford Wetmore]

> 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();
List 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();
List 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
 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 List 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 

Re: TLS ALPN Proposal

[Bradford Wetmore]

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 List 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 List 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=ixgf5SlvOB4&feature=youtu.be&t=27

Re: TLS ALPN Proposal

[Bradford Wetmore]

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 List 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

[Bradford Wetmore]

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 List, then also select() should take a List, rather
than a String[].
Since it's the server picking the protocols, would be handy to have
the client protocols in a List 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 
probably should 

TLS Handshake Message Proposal (Was: Re: JEP 244: TLS Application-Layer Protocol Negotiation Extension)

[Bradford Wetmore]

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 handshak

TLS ALPN Proposal

[Bradford Wetmore]

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 List 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(List 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 was also some

Re: JEP 244: TLS Application-Layer Protocol Negotiation Extension

[Bradford Wetmore]

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 messag

Re: RFR (xs) 8059588: deadlock in java/io/PrintStream when verbose javax.net.debug flags are set

[Bradford Wetmore]

Sean,

Is this a java.security.debug or a javax.net.debug issue as mentioned in 
the synopsis?  It imports from sun.security.util.Debug, which is 
java.security.debug, not javax.net.debug.


In the bug, both variables were set.

...deleted...
-Djavax.net.debug=ssl,handshake,record,keygen,session,defaultctx,sslctx,ses
sioncache,keymanager,trustmanager -Dcom.oracle.security.ucrypto.debug=true
-Djava.security.debug=all -Djsse.S

Probably the synopsis should be changed.

Brad



On 3/27/2015 12:05 PM, Seán Coffey wrote:

Thanks Sean. Yes - I'll add those braces before pushing.

regards,
Sean.

On 27/03/2015 18:58, Sean Mullan wrote:

This looks fine, just one comment. Can you put braces around the if
debug statement on lines 117-118 (just to be safe).

--Sean

On 03/25/2015 09:52 AM, Seán Coffey wrote:

This issue was reported while debugging a pkcs11 issue. While I can't
reproduce it, the captured stack trace does show a deadlock issue. Fix
is to simply stop using the SessionManager class lock to record and
print the maxActiveSessions in use and replace it with a local lock.

bug: https://bugs.openjdk.java.net/browse/JDK-8059588
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8059588/webrev/

Regards,
Sean.

JEP Review Request: TLS Application-Layer Protocol Negotiation Extension

[Bradford Wetmore]

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

[Bradford Wetmore]

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

Re: disabled SSL3 not reflected in "supported protocols"

[Bradford Wetmore]

On 1/27/2015 9:50 AM, Bernd wrote:

> with the Java 7u76 update the default security setting is, that SSL3 is
> banned.

As you saw (and for those that haven't yet), SSLv3 has been disabled 
(deactivated) by default, but it can be reenabled by removing "SSLv3" 
from the list of disabled algorithms in the Security Property 
"jdk.tls.disabledAlgorithms".  (See /lib/security/java.security)


jdk.tls.disabledAlgorithms=SSLv3

SSLv3 is still available, but you have to really jump through hoops to 
get it back.


> At first I thought, this would reflect in enabled and supported
> protocols, however the list of supported protocols still contain SSL3
> and I can also enable SSL3 and this is reflected on the
> getEnabledProtocols():
>
> 1.7.0_76 Oracle Corporation jdk.tls.disabledAlgorithms=SSLv3
> Default Protocols, enabled: [TLSv1] supported: [SSLv2Hello, SSLv3,
> TLSv1, TLSv1.1, TLSv1.2]
> Set SSL3+TLSv1, enabled: [SSLv3, TLSv1]
> Set SSL3, enabled: [SSLv3]
> Now handshaking...
> Exception in thread "main" javax.net.ssl.SSLHandshakeException: No
> appropriate protocol (protocol is disabled or cipher suites are
> inappropriate)
>
> Only at handshake time it looks, like the disabled check is done.

Correct.  It uses the new AlgorithmConstraints class internally, and 
there's a lookup of the Security Property for older releases.


> I wonder would it be cleaner to remove it from the supported set and not
> keep it in the enabled set (but accept the setEnabled for backward
> compatibility).

We went back and forth on this trying to balance the complete removal of 
the SSLv3 code vs. your suggestion to something in between, and it had 
to be something that a System Admin could configure.


We decided for those applications that had hard-coded SSLv3, we should 
not introduce an unexpected IllegalArgumentException Runtime exception 
on set().  We also had to balance in JCK assumptions that a set() 
followed by a get() were expected to return the same values.  (e.g. no 
new JCK failures.)


All of the SSLContexts leave SSLv3 out of the default protocol list 
(including the "SSLv3" SSLContext which leaves only "TLSv1" enabled), so 
the only people who would be potentially impacted are those who are 
intentionally trying to set SSLv3, and they should be asking themselves 
different questions.


This approach is a little confusing/surprising, but gave us the best 
compromise between the competing goals.


Brad

Re: RFR: 8069038: javax/net/ssl/TLS/TLSClientPropertyTest.java needs to be updated for JDK-8061210

[Bradford Wetmore]

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

RFR: 8069038: javax/net/ssl/TLS/TLSClientPropertyTest.java needs to be updated for JDK-8061210

[Bradford Wetmore]

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

[Bradford Wetmore]

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 8044860: Vectors and fixed length fields should be verified for allowed sizes

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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?


WeakReference 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 encapsulation, u

Re: RFR: JDK-8047769 SecureRandom should be more frugal with file descriptors

[Bradford Wetmore]

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

[Bradford Wetmore]

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.

[Bradford Wetmore]

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?

[Bradford Wetmore]

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

[Bradford Wetmore]

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 a

Re: A fully fledged TLS Extensions API ?

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

javax/crypto/JceSecurity.java
line 79: this could be (PrivilegedExceptionAction) 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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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 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 -1
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.

Re: RFR: 8047721: @since should have JDK version

[Bradford Wetmore]

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 -1 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 @@
   *  mark and reset 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 byte.
   * @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 b 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,

Re: ThreadLocalRandom clinit troubles

[Bradford Wetmore]

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 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

[Bradford Wetmore]

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 -1 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 @@
   *  mark and reset 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 byte.
   * @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 b 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 @@
   * stream.
   *
   * @excepti

Re: Code review request, 8044771, PKIXValidator indent cleanup

[Bradford Wetmore]

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

[Bradford Wetmore]

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.

[Bradford Wetmore]

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: Signing operation on client side during SSL Handshake

[Bradford Wetmore]

You should continue following the code, but IIRC, internally 
"MD5andSHA1withRSA" does a Signature.getInstance("NONEwithRSA"), and 
then MessageDigest.getInstance("MD5") and ("SHA").


As long as your provider provides those algorithms and is prioritized 
ahead of other providers which do provide them, you should get them.


Note this is an implementation detail which could change, but AFAIK 
Oracle isn't doing any development in the Open 6 tree.


brad






On 5/20/2014 7:53 AM, Marcin Kaszubski wrote:

Hi,
I want to use private key stored in client TPM to establish MTLS (so
both client and server will be verified) connection with server. So
during ssl handshake this key will be used to sign some data. I wanted
to write my own provider and implement required services to achieve it.
Unfortunately during code review of jdk i found a problem. During sign
operation on client side provider seems to be hardcoded.

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/security/ssl/RSASignature.java#82


How can I use my own providers and its implementation of Signature to
achieve it? Is there a different implementation of SSLSocket which my be
used to achieve it?

This is calling stack:

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/security/ssl/ClientHandshaker.java#734

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/security/ssl/HandshakeMessage.java#1262

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/sun/security/ssl/RSASignature.java#82




Best Regards,
Marcin

Re: RFR 8037745: Consider re-enabling PKCS11 mechanisms previously disabled due to Solaris bug 7050617

[Bradford Wetmore]

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

[Bradford Wetmore]

> 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: [9] Review Request: 8028266 Tidy warnings cleanup for packages java.security/javax.security

[Bradford Wetmore]

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 value 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: [9] Review Request: 8040260 Tidy warnings cleanup for javax.crypto, javax.net

[Bradford Wetmore]

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 ™, 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: RFR 8039853: Provider.Service.newInstance() does not work with current JDK JGSS Mechanisms

[Bradford Wetmore]

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: Code review request, JDK-8040062 Need to add new methods in BaseSSLSocketImpl

[Bradford Wetmore]

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.

[Bradford Wetmore]

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.

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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.(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.(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.(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.(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: CFV: New Security Group Member: Jason Uh

[Bradford Wetmore]

Vote:  Yes

Brad



On 1/28/2014 5:27 AM, Sean Mullan wrote:

I hereby nominate Jason Uh to Membership in the Security Group.

Jason Uh is a member of the Java security libraries team at Oracle and
has been an active contributor to the OpenJDK security group for almost
2 years. Jason was voted in as a JDK 8 committer in February, 2013.
Jason has integrated many significant bug fixes, helped reduce the
number of compiler warnings and improved overall test stability in the
security libraries area.

Votes are due by February 11, 2014, 06:00 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: Code review request, 8028518, Increase the priorities of GCM cipher suites

[Bradford Wetmore]

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, 8028518, Increase the priorities of GCM cipher suites

[Bradford Wetmore]

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: 8030823 jdk9 version update

[Bradford Wetmore]

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, 7093640, Enable TLS 1.2 for client-side default contexts

[Bradford Wetmore]

Hi Xuelei,

I looked as several of the new tests but not all, and looked at the 
existing tests plus new code.


SSLContextImpl.java
===

227:  As I mentioned in Instant Message tonight, I'm not sure this code 
is needed.  I think these values are being set as part of the 
super.engineGetDefaultSSLParameters, so unless your change somehow 
tweaks them, this can probably go.  Thanks for checking this out.


631:  Minor nit, you could tighten up the exception message a little:

PROPERTY_NAME + ": " + protocols[i] +
" is not a standard TLS protocol name";

IllegalProtocolProperty.java and others

Minor nit, you won't be able to run this from the command line in case 
someone wants to do so, I generally do a System.getProperty() on the 
first line instead of a @run option.


Brad



On 12/17/2013 2:08 AM, Xuelei Fan wrote:

Hi,

This is a request to enabled TLS 1.2 for client-side default contexts.
Please review this update.

webrev: http://cr.openjdk.java.net/~xuelei/7093640/webrev.00/

We are still concern about the version intolerance issue with some older
SSL/TLS server implementation.  As a workaround, a new system property,
"jdk.tls.client.protocols", is defined to configure the protocols in
default contexts.

By default, TLS 1.1 and TLS 1.2 (plus other supported and safe
protocols) are enabled unless the system property is explicit configured
and does not contain "TLSv1.1" or "TLSv1.2".

The property string is a list of comma separated standard SSL protocol
names. The syntax of the property string can be described as this Java
BNF-style:
  ClientProtocols:
 ('"' SSLProtocolNames '"') | SSLProtocolNames
  SSLProtocolNames:
 SSLProtocolName { , SSLProtocolName }
  SSLProtocolName:
 (see below)

The "SSLProtocolName" is the standard SSL protocol name as described in
the "Java Cryptography Architecture Standard Algorithm Name
Documentation". If the property value does not comply to the above
syntax, or the specified value of SSLProtocolName is not a supported SSL
protocol name, the instantiation of the SSLContext provider service (via
SSLContext.getInstance() methods) may generate a
java.security.NoSuchAlgorithmException. Please note that the protocol
name is case-sensitive.

If the system property is not set or is empty, the default enabled
protocol setting in both client and server looks like:

Protocol Enabled   Enabled
  for Clientfor Server
 ----
SSLv3Yes   Yes
TLSv1Yes   Yes
TLSv1.1  Yes   Yes
TLSv1.2  Yes   Yes
SSLv2Hello   NoYes


If the system property is set to "TLSv1,TLSv1.1", the default enabled
protocol setting in both client and server looks like:

Protocol Enabled   Enabled
  for Clientfor Server
 ----
SSLv3NoYes
TLSv1Yes   Yes
TLSv1.1  Yes   Yes
TLSv1.2  NoYes
SSLv2Hello   NoYes

This update does not impact the API specification of JSSE, JSSE server
side and third party's provider.

Thanks,
Xuelei

Re: Code Review request: 8029550 javadoc updates

[Bradford Wetmore]

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.

[Bradford Wetmore]

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: Code Review Request: 8021418

[Bradford Wetmore]

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/


The test is modified to set SO_REUSEADDR on ServerSocket to false for
stabilization.

Thanks,
Rajan

Re: Code Review Request: 8025763

[Bradford Wetmore]

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  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: Thoughts on possible options to JDK-8027598

[Bradford Wetmore]

> 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

[Bradford Wetmore]

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

[Bradford Wetmore]

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
, 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
.

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: Thoughts on possible options to JDK-8027598

[Bradford Wetmore]

On 11/19/2013 4:19 PM, Rajan Halade wrote:

I am working towards fixing 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
.

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: RFR: JDK-8027624 - com/sun/crypto/provider/KeyFactory/TestProviderLeak.java unstable again

[Bradford Wetmore]

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/


Thanks,

-Dan

hg: jdk8/tl/jdk: 8027526: CheckTipsAndVersions.java failing occasionally

[bradford . wetmore]

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

Re: Very simple webrev.

[Bradford Wetmore]

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

Re: Very simple webrev.

[Bradford Wetmore]

*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

Very simple webrev.

[Bradford Wetmore]

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: [8] Request for Review: 8026233: test/sun/security/tools/keytool/StorePasswords.java needs to clean up files

[Bradford Wetmore]

Looks good.

brad


On 10/17/2013 11:35 AM, Vincent Ryan wrote:

Looks fine.
Thanks.


On 17 Oct 2013, at 19:04, Jason Uh wrote:


Hi Vinnie, I'd like to add to this changeset to ensure that resources are 
closed. Could you please review the revision?

http://cr.openjdk.java.net/~juh/8026233/webrev.01/

Thanks,
Jason

On 10/10/2013 12:15 PM, Vincent Ryan wrote:

That fix looks fine Jason.
Thanks.

On 10 Oct 2013, at 01:57, Jason Uh wrote:


Hi Vinnie,

Could you please review this fix? The test 
sun/security/tools/keytool/StorePasswords.java can terminate with an error on 
Windows because of files not getting cleaned up, so this fix deletes the 
keystore file at the end of the test.

webrev: http://cr.openjdk.java.net/~juh/8026233/webrev.00/

Thanks,
Jason

hg: jdk8/tl/corba: 8026762: jdk8-tl builds windows builds failing in corba - javac: no source files

[bradford . wetmore]

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

hg: jdk8/tl/jdk: 8026762: jdk8-tl builds windows builds failing in corba - javac: no source files

[bradford . wetmore]

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

Re: [8] 8026301: DomainKeyStore doesn't cleanup correctly when storing to keystore

[Bradford Wetmore]

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.

hg: jdk8/tl/jdk: 8025694: Rename getStrongSecureRandom based on feedback; ...

[bradford . wetmore]

Changeset: a6ac824b463d
Author:wetmore
Date:  2013-10-02 09:38 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a6ac824b463d

8025694: Rename getStrongSecureRandom based on feedback
8014838: getStrongSecureRandom() should require at least one implementation
Reviewed-by: mullan, darcy

! src/share/classes/java/security/SecureRandom.java
! src/share/lib/security/java.security-windows
! test/sun/security/provider/SecureRandom/StrongSecureRandom.java

Re: RFR: JDK-8014838: getStrongSecureRandom() should require at least one implementation

[Bradford Wetmore]

During the internal CCC review, the CCC lead suggested a change to the 
method name to more closely follow the existing getInstance() methods, 
so I've changed the name from:


java.security.SecureRandom.getStrongSecureRandom()

to

java.security.SecureRandom.getInstanceStrong()

This particular change will be tracked via:

8025694: Rename getStrongSecureRandom based on feedback

This will collate better (i.e. javadoc/NetBeans) than getStrongInstance().

It was also pointed out that the current strong Window MSCAPI-based 
implementation may not be available under the various profiles, so I've 
added a SHA1PRNG:SUN fallback in case the MSCAPI is not available.


https://bugs.openjdk.java.net/browse/JDK-8014838
https://bugs.openjdk.java.net/browse/JDK-8025694
http://cr.openjdk.java.net/~wetmore/8025694/webrev.00
(contains both 8025694/8014838)

Even though we are not currently building profiles on Windows currently, 
we could in the future, so the profiles team agrees this is a good 
compromise.


Brad




On 9/26/2013 4:04 PM, Bradford Wetmore wrote:

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 for 8014374: Cannot initialize "AES/GCM/NoPadding" on wrap/unseal on solaris with OracleUcrypto

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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: Bug in ProcessBuilder.

[Bradford Wetmore]

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 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