Re: RFR: 8193892: Impact of noncloneable MessageDigest implementation

[Seán Coffey]

Thanks for the review Brad. Yes - I'll make those changes before pushing.

regards,
Sean.


On 23/02/2018 20:14, Brad Wetmore wrote:

Minor comments.  I'm ok with leaving as is, but this seems cleaner.

MyProvider.java
---
Would prefer to use the non-deprecated super call.

*Digest.java

Would you consider making these 3 duplicate classes into a single 
class, with the three public derived classes within?  And then update 
the MyProvider entries with DigestBase${MD5,SHA,SHA256}. i.e.


import java.security.*;

class DigestBase extends MessageDigestSpi {

    private MessageDigest digest = null;

    public DigestBase(String alg, String provider) throws Exception {
    digest = MessageDigest.getInstance(alg, provider);
    }

    @Override
    protected void engineUpdate(byte input) {
    digest.update(input);
    }

    @Override
    protected void engineUpdate(byte[] input, int offset, int len) {
    digest.update(input, offset, len);
    }

    @Override
    protected byte[] engineDigest() {
    return digest.digest();
    }

    @Override
    protected void engineReset() {
    digest.reset();
    }

    public static final class MD5 extends DigestBase {
    public MD5() throws Exception {
    super("MD5", "SUN");
    }
    }

    public static final class SHA extends DigestBase {
    public SHA() throws Exception {
    super("SHA", "SUN");
    }
    }

    public static final class SHA256 extends DigestBase {
    public SHA256() throws Exception {
    super("SHA-256", "SUN");
    }
    }
}

Thanks, sorry for the delay.

Brad



On 2/15/2018 7:40 AM, Xuelei Fan wrote:

Looks fine to me.  Thanks!

Xuelei

On 2/15/2018 1:04 AM, Seán Coffey wrote:

A reminder for this review..

regards,
Sean.


On 09/02/2018 16:25, Seán Coffey wrote:
Looking to push a new test which helps test CloneableDigest code. 
It's something that arose during JDK-8193683 fix.


The test was contributed by Brad Wetmore. I've converted it to use 
the SSLSocketTemplate.


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

Re: RFR 8197518: Kerberos krb5 authentication: AuthList's put method leads to performance issue

[Weijun Wang]

> On Feb 24, 2018, at 12:38 AM, Daniel Fuchs  wrote:
> 
> Hi Max,
> 
>  85 // has larger timestamp.
>  86 entries.addFirst(t);
> 
> shouldn't you set oldestTime here as well?

The order might be a little misleading, you addFirst() the latest entry and the 
oldest one is at getLast(). oldestTime is set on line 78 because it's the only 
entry. But for line 86, there are already existing entries.

--Max

> 
> best regards,
> 
> -- daniel
> 
> On 23/02/2018 15:21, Xuelei Fan wrote:
>> Looks fine to me.
>> Xuelei
>> On 2/23/2018 6:13 AM, Weijun Wang wrote:
>>> Updated at http://cr.openjdk.java.net/~weijun/8197518/webrev.02/.

Re: [PATCH] JDK-8190917 : SSL session resumption, through handshake, in SSLEngine is broken for any protocols lesser than TLSv1.2

[Jaikiran Pai]

Sounds fine, thank you Xuelei. Would this later be backported to Java 9 too?

-Jaikiran


On 24/02/18 12:21 AM, Xuelei Fan wrote:

Hi Jaikiran,

Thanks a lot for the update.  Your code looks fine to me.

As we are working on the re-org of the implementation[1] now, I may 
integrate your contribution shortly after the re-org changes.


Thanks,
Xuelei

[1]: 
http://mail.openjdk.java.net/pipermail/security-dev/2018-February/016815.html


On 2/22/2018 9:58 PM, Jaikiran Pai wrote:
Given some recent changes to the class involved in this patch, in the 
jdk repo (http://hg.openjdk.java.net/jdk/jdk), I noticed some merge 
conflicts to this patch today. So I've now attached an updated patch 
which resolves those merge issues. This has been tested with latest 
jtreg (tip).


-Jaikiran


On 06/12/17 9:35 PM, Jaikiran Pai wrote:

Thank you Xuelei. Please take your time.

-Jaikiran



On Wednesday, December 6, 2017, Xuelei Fan > wrote:


    Hi Jaikiran,

    I will sponsor this contribution.  I need more time for the review
    and testing.

    Thanks,
    Xuelei

    On 11/23/2017 9:22 PM, Jaikiran Pai wrote:

    As noted in [1], there's a regression in Java 9, where SSL
    session resumption no longer works for SSL protocols other
    than TLSv1.2. The code which is responsible for session
    resumption resides in the ServerHandshaker[2], in the
    clientHello method. This method, in its logic to decide
    whether or not to resume a (previously) cached session, checks
    if the client hello message has a session id associated. If it
    does, it then fetches a session from the cache. If it does
    find a previously cached session for that id, it then goes
    ahead to check if the SSL protocol associated with the cached
    session matches with the protocol version that's
    "applicable/selected" of the incoming client hello message.
    However, a relatively recent change[3] has, IMO,
    unintentionally caused the protocol version check logic to
    fail for protocols other than TLSv1.2. The protocol version
    check looks like:


    // cannot resume session with different

    if (oldVersion != protocolVersion) {
    resumingSession = false;
    }

    The `protocolVersion` variable in this case happens to be a
    _default initialized value_ (TLSv1.2) instead of the one
    that's "selected" based on the incoming client hello message.
    As a result the `protocolVersion` value is always TLSv1.2 and
    thus for any other protocols, this comparison fails, even when
    the client hello message uses the right session id and the
    right protocol that matches the previous session.

    The attached patch, includes a potential fix to this issue.
    The change makes sure that the protocol selection, based on
    the client hello message, is done before this session
    resumption check and also makes sure it uses that "selected"
    protocol in the version comparison check instead of the class
    member `protocolVersion` (which gets set when the server hello
    message is finally being sent).

    The attached patch also contains a (jtreg) test case which
    reproduces this bug and verifies this fix. The test case tests
    the session resumption against TLSv1, TLSv1.1 and TLSv1.2. The
    test code itself is a minor modification of the SSL demo
    that's available here [4].

    This is my first OpenJDK patch and my OCA got approved a few
    days back. Could someone please help with the review of this
    patch?

    P.S: I noticed that the JIRA got assigned a few days back. I
    am not sure if that means the fix is already being worked upon
    by someone else from the team. If that's the case, then let me
    know and I'll let it be handled by them.

    [1] https://bugs.openjdk.java.net/browse/JDK-8190917


    [2]
http://hg.openjdk.java.net/jdk/jdk/file/b7ae1437111b/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java



    [3] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/42268eb6e04e


    [4]
https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/samples/sslengine/SSLEngineSimpleDemo.java



    -Jaikiran

Re: RFR 8197518: Kerberos krb5 authentication: AuthList's put method leads to performance issue

[Weijun Wang]

Updated at http://cr.openjdk.java.net/~weijun/8197518/webrev.02/.

1. ConcurrentHashMap used in MemoryCache. No more "content.remove(key)" but 
it's actually useless because rc.isEmpty() will not be true normally. I'll 
think about how to clean up unused AuthList objects in another RFE.

2. AuthList now removes entries from tail backwards until one is within 
lifespan. I still kept an oldestTime field. In my experiment it is useful but a 
bigger value does not help much, so I just hardcoded it to 5 seconds.

Thanks
Max

> On Feb 23, 2018, at 4:57 PM, Weijun Wang  wrote:
> 
> Brilliant idea. I will do more experiment.
> 
> Thanks
> Max
> 
>> On Feb 23, 2018, at 11:25 AM, Xuelei Fan  wrote:
>> 
>> Hi Weijun,
>> 
>> I thought more about the performance impact.  The impact may mainly come 
>> from the big size of the cached entries.
>> 
>> The current implementation needs to go over the full list: find the 1st 
>> expired item and then remove the rest.  The previous one have an additional 
>> round with entries.indexOf().  Could we just start from the end of the list?
>> 
>>   while (true) {
>>  E e = entries.removeLast();
>>  if e not expired {
>> add it back and break;
>>  }
>>   };
>> 
>> If the list is really big, and the lifetime is significant big as well (>> 1 
>> minute), iterate from the oldest item (backward from the end of the list) 
>> may be much more effective.  LinkedList itself is not synchronized, so as if 
>> there is not too much items to go over, the performance should be fine.  I'm 
>> hesitate to hard code a cleanup every 1 minute strategy.  If we clean often, 
>> there may be not too much items to go over in the list.  So we might be able 
>> to remove the "at most every minute" strategy.
>> 
>> Xuelei
>> 
>> On 2/22/2018 5:55 PM, Weijun Wang wrote:
>>> Updated webrev at http://cr.openjdk.java.net/~weijun/8197518/webrev.01/.
 On Feb 23, 2018, at 9:02 AM, Weijun Wang  wrote:
 
 You mean I can save it somewhere and only update it when a cleanup is 
 performed?
 
 This should be better. Now there will be only isEmpty(), getFirst() and 
 addFirst(), and one less getLast().
 
 Thanks
 Max
 
> On Feb 23, 2018, at 1:45 AM, Xuelei Fan  wrote:
> 
> Looks like list synchronization is a factor of the performance impact. 
> Maybe, you can have a private time for the oldest entry so don't 
> access/iterate/cleanup entries list until necessary.  The "at most every 
> minute" may be not a good strategy in some situations.
>>> In fact, it's now almost "exactly every minute". What situations do you 
>>> think it's not good? I cannot use size() because I have to remember all 
>>> entries with lifespan to be correct.
>>> Thanks
>>> Max
> 
> Xuelei
> 
> On 2/22/2018 12:36 AM, Weijun Wang wrote:
>> Please take a review at
>>  http://cr.openjdk.java.net/~weijun/8197518/webrev.00/
>> Two notes:
>> 1. I tried list.subList(here, end).clear() but it's not faster.
>> 2. I have looked at ConcurrentHashMap + ConcurrentSkipListMap but will 
>> need more time to verify its correctness and measure the performance 
>> gain. Since the bug is reported on 8u, a safer fix looks better.
>> Noreg-perf.
>> Thanks
>> Max
 
> 

Re: RFR 8181594: Efficient and constant-time modular arithmetic

[Xuelei Fan]

ArrayUtil.java:
===
I'm not very sure how widely this utilities will be used in the future. 
Looks like only BigIntegerModuloP uses this classes.  I may prefer to 
define private methods for byte array swap in BigIntegerModuloP.



BigIntegerModuloP.java:
===
As this is a class for testing or ptototype purpose,  it might not be a 
part of JDK products, like JRE.  Would you mind move it to a test 
package if you want to keep it?



IntegerModuloP, IntegerModuloP_Base and MutableIntegerModuloP
=
In the security package/context, it may make sense to use 
"IntegerModulo" for the referring to "integers modulo a prime value". 
The class name of "IntegerModuloP_Base" is not a general Java coding 
style.  I may prefer a little bit name changes like:

 IntegerModuloP_Base   -> IntegerModulo
 IntegerModuloP-> ImmutableIntegerModulo
 MutableIntegerModuloP -> MutableIntegerModulo

 IntegerFieldModuloP   -> IntegerModuloField (?)


MutableIntegerModuloP.java
==
void conditionalSwapWith(MutableIntegerModuloP b, int swap);
As the 'swap' parameter can only be 0 or 1, could it be a boolean parameter?

Except the conditionalSwapWith() method, I did not get the points why we 
need a mutable version.  Would you please have more description of this 
requirement?



IntegerModuloP_Base.java

default byte[] addModPowerTwo(IntegerModuloP_Base b, int len)
void addModPowerTwo(IntegerModuloP_Base b, byte[] result);

For the first sign of the method names, I thought it is to calculate as 
"(this + b) ^ 2 mod m".  Besides, what's the benefits of the two 
methods?  Could we just use:

  this.add(b).asByteArray()

I guess, but not very sure, it is for constant time calculation.  If the 
function is required, could it be renamed as:


  // the result is inside of the size range
  IntegerModuloP addModSize(IntegerModuloP_Base b, int size)
Or
  // the result is wrapped if outside of the size range
  IntegerModuloP addOnWrap(IntegerModuloP_Base b, int size)

and the use may look like:
  this.addModSize(b, size).asByteArray()


Will review the rest when I understand more about the interfaces design.

Thanks,
Xuelei

On 1/30/2018 8:52 AM, Adam Petcher wrote:

+core-libs-dev


On 1/26/2018 4:06 PM, Adam Petcher wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8181594
Webrev: http://cr.openjdk.java.net/~apetcher/8181594/webrev.00/

This is a code review for the field arithmetic that will be used in 
implementations of X25519/X448 key agreement, the Poly1305 
authenticator, and EdDSA signatures. I believe that the library has 
all the features necessary for X25519/X448 and Poly1305, and I expect 
at most a couple of minor enhancements will be required to support 
EdDSA. There is no public API for this library, so we can change it in 
the future to suit the needs of new algorithms without breaking 
compatibility with external code. Still, I made an attempt to clearly 
structure and document the (internal) API, and I want to make sure it 
is understandable and easy to use.


This is not a general-purpose modular arithmetic library. It will only 
work well in circumstances where the sequence of operations is 
restricted, and where the prime that defines the field has some useful 
structure. Moreover, each new field will require some field-specific 
code that takes into account the structure of the prime and the way 
the field is used in the application. The initial implementation 
includes a field for Poly1305 and the fields for X25519/X448 which 
should also work for EdDSA.


The benefits of using this library are that it is much more efficient 
than using similar operations in BigInteger. Also, many operations are 
branch-free, making them suitable for use in a side-channel resistant 
implementation that does not branch on secrets.


To provide some context, I have attached a code snippet describing how 
this library can be used. The snippet is the constant-time Montgomery 
ladder from my X25519/X448 implementation, which I expect to be out 
for review soon. X25519/X448 only uses standard arithmetic operations, 
and the more unusual features (e.g. add modulo a power of 2) are 
needed by Poly1305.


The field arithmetic (for all fields) is implemented using a 32-bit 
representation similar to the one described in the Ed448 paper[1] (in 
the "Implementation on 32-bit platforms" section). Though my 
implementation uses signed limbs, and grade-school multiplication 
instead of Karatsuba. The argument for correctness is essentially the 
same for all three fields: the magnitude of each 64-bit limb is at 
most 2^(k-1) after reduction, except for the last limb which may have 
a magnitude of up to 2^k. The values of k are between 26 to 28 
(depending on the field), and we can calculate that the maximum 
magnitude for any limb during an add-multiply-carry-reduce 

Re: Update mechanism for the upcoming trust store

[Fotis Loukos]

Hello Sean,

On 29/01/2018 04:30 μμ, Fotis Loukos wrote:
> On 26/01/2018 11:31 μμ, Sean Mullan wrote:
>> On 1/24/18 5:39 AM, Fotis Loukos wrote:
 You may not be aware, but the JDK does currently support a mechanism for
 blacklisting certificates. The lib/security/blacklisted.certs file
 contains a list of the fingerprints of certificates that are explicitly
 distrusted. If a root was compromised and added to this file it would no
 longer be trusted.
>>> My biggest concern is what you describe below, the dynamic update.
>>>
 However, currently there is no mechanism in OpenJDK to dynamically
 update that file. While I think there is merit in implementing something
 like that, many challenges would need to be addressed as part of that,
 for example, where and how does this file get updated, how is it
 verified, etc.
>>> The verification step can be implemented as described above. I think
>>> that the update step requires some design, but I don't think it is that
>>> difficult. For example, a naive algorithm such as every Monday plus a
>>> random number of days/hours/minutes in order to avoid heavy traffic
>>> during the update period would be good for starters. As a first step to
>>> try a new format, you could even fetch it once during installation and
>>> provide a means for the user to update it manually.
>>
>> One thing we could potentially do initially is to provide a stable https
>> URL where an updated blacklist could be downloaded, something like what
>> Mozilla does for OneCRL [1]. This would be an initial step, not the
>> whole solution of course, but it at least would allow someone to quickly
>> update their JDK should a certificate need to be distrusted ASAP.
>>
>> Let me look into that a bit.
>>
>> I think implementing a dynamic solution is more challenging and would
>> require more design/review, etc but feel free to provide more details on
>> any additional thoughts or design sketches you might have.
> 
> I find it pretty important to ensure that the file is also signed and
> not just served over https. I was wondering if the community runs a
> private CA, or has access to an HSM that can be used to generate and
> store a private key that will sign these files.
> 

Do you have any info on this? Is there a way to sign these files
securely, or should we find a different method of packaging them?

Regards,
Fotis

> Regards,
> Fotis
> 
>>
>> --Sean
>>
>> [1]
>> https://firefox.settings.services.mozilla.com/v1/buckets/blocklists/collections/certificates/records
>>
>>
>>
> 
> 


-- 
Fotis Loukos, PhD
Director of Security Architecture
SSL Corp
e: fot...@ssl.com
w: https://www.ssl.com

Re: RFR 8197518: Kerberos krb5 authentication: AuthList's put method leads to performance issue

[Xuelei Fan]

Looks fine to me.

Xuelei

On 2/23/2018 6:13 AM, Weijun Wang wrote:

Updated at http://cr.openjdk.java.net/~weijun/8197518/webrev.02/.

1. ConcurrentHashMap used in MemoryCache. No more "content.remove(key)" but 
it's actually useless because rc.isEmpty() will not be true normally. I'll think about 
how to clean up unused AuthList objects in another RFE.

2. AuthList now removes entries from tail backwards until one is within 
lifespan. I still kept an oldestTime field. In my experiment it is useful but a 
bigger value does not help much, so I just hardcoded it to 5 seconds.

Thanks
Max


On Feb 23, 2018, at 4:57 PM, Weijun Wang  wrote:

Brilliant idea. I will do more experiment.

Thanks
Max


On Feb 23, 2018, at 11:25 AM, Xuelei Fan  wrote:

Hi Weijun,

I thought more about the performance impact.  The impact may mainly come from 
the big size of the cached entries.

The current implementation needs to go over the full list: find the 1st expired 
item and then remove the rest.  The previous one have an additional round with 
entries.indexOf().  Could we just start from the end of the list?

   while (true) {
  E e = entries.removeLast();
  if e not expired {
 add it back and break;
  }
   };

If the list is really big, and the lifetime is significant big as well (>> 1 minute), 
iterate from the oldest item (backward from the end of the list) may be much more effective.  
LinkedList itself is not synchronized, so as if there is not too much items to go over, the 
performance should be fine.  I'm hesitate to hard code a cleanup every 1 minute strategy.  If 
we clean often, there may be not too much items to go over in the list.  So we might be able to 
remove the "at most every minute" strategy.

Xuelei

On 2/22/2018 5:55 PM, Weijun Wang wrote:

Updated webrev at http://cr.openjdk.java.net/~weijun/8197518/webrev.01/.

On Feb 23, 2018, at 9:02 AM, Weijun Wang  wrote:

You mean I can save it somewhere and only update it when a cleanup is performed?

This should be better. Now there will be only isEmpty(), getFirst() and 
addFirst(), and one less getLast().

Thanks
Max


On Feb 23, 2018, at 1:45 AM, Xuelei Fan  wrote:

Looks like list synchronization is a factor of the performance impact. Maybe, you can 
have a private time for the oldest entry so don't access/iterate/cleanup entries list 
until necessary.  The "at most every minute" may be not a good strategy in some 
situations.

In fact, it's now almost "exactly every minute". What situations do you think 
it's not good? I cannot use size() because I have to remember all entries with lifespan 
to be correct.
Thanks
Max


Xuelei

On 2/22/2018 12:36 AM, Weijun Wang wrote:

Please take a review at
  http://cr.openjdk.java.net/~weijun/8197518/webrev.00/
Two notes:
1. I tried list.subList(here, end).clear() but it's not faster.
2. I have looked at ConcurrentHashMap + ConcurrentSkipListMap but will need 
more time to verify its correctness and measure the performance gain. Since the 
bug is reported on 8u, a safer fix looks better.
Noreg-perf.
Thanks
Max

Re: RFR 8197518: Kerberos krb5 authentication: AuthList's put method leads to performance issue

[Daniel Fuchs]

Hi Max,

  85 // has larger timestamp.
  86 entries.addFirst(t);

shouldn't you set oldestTime here as well?

best regards,

-- daniel

On 23/02/2018 15:21, Xuelei Fan wrote:

Looks fine to me.

Xuelei

On 2/23/2018 6:13 AM, Weijun Wang wrote:

Updated at http://cr.openjdk.java.net/~weijun/8197518/webrev.02/.

1. ConcurrentHashMap used in MemoryCache. No more 
"content.remove(key)" but it's actually useless because rc.isEmpty() 
will not be true normally. I'll think about how to clean up unused 
AuthList objects in another RFE.


2. AuthList now removes entries from tail backwards until one is 
within lifespan. I still kept an oldestTime field. In my experiment it 
is useful but a bigger value does not help much, so I just hardcoded 
it to 5 seconds.


Thanks
Max


On Feb 23, 2018, at 4:57 PM, Weijun Wang  wrote:

Brilliant idea. I will do more experiment.

Thanks
Max


On Feb 23, 2018, at 11:25 AM, Xuelei Fan  wrote:

Hi Weijun,

I thought more about the performance impact.  The impact may mainly 
come from the big size of the cached entries.


The current implementation needs to go over the full list: find the 
1st expired item and then remove the rest.  The previous one have an 
additional round with entries.indexOf().  Could we just start from 
the end of the list?


   while (true) {
  E e = entries.removeLast();
  if e not expired {
 add it back and break;
  }
   };

If the list is really big, and the lifetime is significant big as 
well (>> 1 minute), iterate from the oldest item (backward from the 
end of the list) may be much more effective.  LinkedList itself is 
not synchronized, so as if there is not too much items to go over, 
the performance should be fine.  I'm hesitate to hard code a cleanup 
every 1 minute strategy.  If we clean often, there may be not too 
much items to go over in the list.  So we might be able to remove 
the "at most every minute" strategy.


Xuelei

On 2/22/2018 5:55 PM, Weijun Wang wrote:
Updated webrev at 
http://cr.openjdk.java.net/~weijun/8197518/webrev.01/.
On Feb 23, 2018, at 9:02 AM, Weijun Wang  
wrote:


You mean I can save it somewhere and only update it when a cleanup 
is performed?


This should be better. Now there will be only isEmpty(), 
getFirst() and addFirst(), and one less getLast().


Thanks
Max

On Feb 23, 2018, at 1:45 AM, Xuelei Fan  
wrote:


Looks like list synchronization is a factor of the performance 
impact. Maybe, you can have a private time for the oldest entry 
so don't access/iterate/cleanup entries list until necessary.  
The "at most every minute" may be not a good strategy in some 
situations.
In fact, it's now almost "exactly every minute". What situations do 
you think it's not good? I cannot use size() because I have to 
remember all entries with lifespan to be correct.

Thanks
Max


Xuelei

On 2/22/2018 12:36 AM, Weijun Wang wrote:

Please take a review at
  http://cr.openjdk.java.net/~weijun/8197518/webrev.00/
Two notes:
1. I tried list.subList(here, end).clear() but it's not faster.
2. I have looked at ConcurrentHashMap + ConcurrentSkipListMap 
but will need more time to verify its correctness and measure 
the performance gain. Since the bug is reported on 8u, a safer 
fix looks better.

Noreg-perf.
Thanks
Max

Re: [PATCH] JDK-8190917 : SSL session resumption, through handshake, in SSLEngine is broken for any protocols lesser than TLSv1.2

[Xuelei Fan]

Hi Jaikiran,

Thanks a lot for the update.  Your code looks fine to me.

As we are working on the re-org of the implementation[1] now, I may 
integrate your contribution shortly after the re-org changes.


Thanks,
Xuelei

[1]: 
http://mail.openjdk.java.net/pipermail/security-dev/2018-February/016815.html


On 2/22/2018 9:58 PM, Jaikiran Pai wrote:
Given some recent changes to the class involved in this patch, in the 
jdk repo (http://hg.openjdk.java.net/jdk/jdk), I noticed some merge 
conflicts to this patch today. So I've now attached an updated patch 
which resolves those merge issues. This has been tested with latest 
jtreg (tip).


-Jaikiran


On 06/12/17 9:35 PM, Jaikiran Pai wrote:

Thank you Xuelei. Please take your time.

-Jaikiran



On Wednesday, December 6, 2017, Xuelei Fan > wrote:


Hi Jaikiran,

I will sponsor this contribution.  I need more time for the review
and testing.

Thanks,
Xuelei

On 11/23/2017 9:22 PM, Jaikiran Pai wrote:

As noted in [1], there's a regression in Java 9, where SSL
session resumption no longer works for SSL protocols other
than TLSv1.2. The code which is responsible for session
resumption resides in the ServerHandshaker[2], in the
clientHello method. This method, in its logic to decide
whether or not to resume a (previously) cached session, checks
if the client hello message has a session id associated. If it
does, it then fetches a session from the cache. If it does
find a previously cached session for that id, it then goes
ahead to check if the SSL protocol associated with the cached
session matches with the protocol version that's
"applicable/selected" of the incoming client hello message.
However, a relatively recent change[3] has, IMO,
unintentionally caused the protocol version check logic to
fail for protocols other than TLSv1.2. The protocol version
check looks like:


// cannot resume session with different

if (oldVersion != protocolVersion) {
resumingSession = false;
}

The `protocolVersion` variable in this case happens to be a
_default initialized value_ (TLSv1.2) instead of the one
that's "selected" based on the incoming client hello message.
As a result the `protocolVersion` value is always TLSv1.2 and
thus for any other protocols, this comparison fails, even when
the client hello message uses the right session id and the
right protocol that matches the previous session.

The attached patch, includes a potential fix to this issue.
The change makes sure that the protocol selection, based on
the client hello message, is done before this session
resumption check and also makes sure it uses that "selected"
protocol in the version comparison check instead of the class
member `protocolVersion` (which gets set when the server hello
message is finally being sent).

The attached patch also contains a (jtreg) test case which
reproduces this bug and verifies this fix. The test case tests
the session resumption against TLSv1, TLSv1.1 and TLSv1.2. The
test code itself is a minor modification of the SSL demo
that's available here [4].

This is my first OpenJDK patch and my OCA got approved a few
days back. Could someone please help with the review of this
patch?

P.S: I noticed that the JIRA got assigned a few days back. I
am not sure if that means the fix is already being worked upon
by someone else from the team. If that's the case, then let me
know and I'll let it be handled by them.

[1] https://bugs.openjdk.java.net/browse/JDK-8190917


[2]

http://hg.openjdk.java.net/jdk/jdk/file/b7ae1437111b/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java




[3] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/42268eb6e04e


[4]

https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/samples/sslengine/SSLEngineSimpleDemo.java




-Jaikiran

RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Roger Riggs]

Please review cleanup replacements of 
System.getProperty("line.separator") with System.lineSeparator().
It uses the line separator from System instead of looking it up in the 
properties each time.

Also fixed one javadoc @see reference.

The affected files are in several packages:

   com/sun/crypto/provider/
   java/util/regex/
   jdk/internal/util/xml/impl/

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/

Thanks, Roger

Re: RFR 8198645 Use System.lineSeparator() instead of getProperty("line.separator")

[Lance Andersen]

Looks good Roger


> On Feb 23, 2018, at 2:39 PM, Roger Riggs  wrote:
> 
> Please review cleanup replacements of System.getProperty("line.separator") 
> with System.lineSeparator().
> It uses the line separator from System instead of looking it up in the 
> properties each time.
> Also fixed one javadoc @see reference.
> 
> The affected files are in several packages:
> 
>   com/sun/crypto/provider/
>   java/util/regex/
>   jdk/internal/util/xml/impl/
> 
> Webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-line-separator-8198645/
> 
> Thanks, Roger
> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: 8193892: Impact of noncloneable MessageDigest implementation

[Brad Wetmore]

Minor comments.  I'm ok with leaving as is, but this seems cleaner.

MyProvider.java
---
Would prefer to use the non-deprecated super call.

*Digest.java

Would you consider making these 3 duplicate classes into a single class, 
with the three public derived classes within?  And then update the 
MyProvider entries with DigestBase${MD5,SHA,SHA256}.  i.e.


import java.security.*;

class DigestBase extends MessageDigestSpi {

private MessageDigest digest = null;

public DigestBase(String alg, String provider) throws Exception {
digest = MessageDigest.getInstance(alg, provider);
}

@Override
protected void engineUpdate(byte input) {
digest.update(input);
}

@Override
protected void engineUpdate(byte[] input, int offset, int len) {
digest.update(input, offset, len);
}

@Override
protected byte[] engineDigest() {
return digest.digest();
}

@Override
protected void engineReset() {
digest.reset();
}

public static final class MD5 extends DigestBase {
public MD5() throws Exception {
super("MD5", "SUN");
}
}

public static final class SHA extends DigestBase {
public SHA() throws Exception {
super("SHA", "SUN");
}
}

public static final class SHA256 extends DigestBase {
public SHA256() throws Exception {
super("SHA-256", "SUN");
}
}
}

Thanks, sorry for the delay.

Brad



On 2/15/2018 7:40 AM, Xuelei Fan wrote:

Looks fine to me.  Thanks!

Xuelei

On 2/15/2018 1:04 AM, Seán Coffey wrote:

A reminder for this review..

regards,
Sean.


On 09/02/2018 16:25, Seán Coffey wrote:
Looking to push a new test which helps test CloneableDigest code. 
It's something that arose during JDK-8193683 fix.


The test was contributed by Brad Wetmore. I've converted it to use 
the SSLSocketTemplate.


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