Re: Draft design for Key Derivation API

[Michael StJohns]

On 11/7/2017 8:17 PM, Jamil Nimeh wrote:


Hi Mike, thank you for your comments and feedback.  I have a few 
comments and questions inline:




(I have a little bit more time before my flight than i thought so 
see inline).



On 11/06/2017 05:25 PM, Michael StJohns wrote:

On 11/3/2017 4:59 PM, Jamil Nimeh wrote:

Hello all,

This is a review request for the draft of a new Key Derivation API.  
The goal of this API will be to provide a framework for KDF 
algorithms like HKDF, TLS-PRF, PBKDF2 and so forth to be publicly 
accessible.  We also plan to provide an SPI that let 3rd parties 
create their own implementations of KDFs in their providers, rather 
than trying to force them into KeyGenerators, SecretKeyFactories and 
the like.


Rather than stuff this email full of the specification text (since 
it is likely to get quite a few iterations of comments and 
comments-to-comments), I have placed the API both in simple text 
form and as a Javadoc at the following locations:


spec: http://cr.openjdk.java.net/~jnimeh/reviews/kdfspec/kdfspec.01.txt

javadoc: http://cr.openjdk.java.net/~jnimeh/reviews/kdfspec/javadoc.01/

They're both the same content, just use whichever is friendlier for 
your eyes.


In addition, I have opened up the JEP as well:

https://bugs.openjdk.java.net/browse/JDK-8189808

Thanks to those who have contributed to very early internal drafts 
of this so far, and thanks in advance to those who will be 
contributing comments going forward.


--Jamil


Most of the following suggestions (and please take them as such 
regardless of any directive language) represent things I've had to do 
manually that I'd really prefer to do in a real key derivation API.  
A few are related to how to keep things securely stored in an HSM.


Add a .reset() method to KeyDerivation.  Call this to clear the state 
of the KDF.


Add an .initialize(List, SecretKey 
masterSecret) method.  Remove the argument to deriveKey and 
deriveKeys.  This plays with the stuff to follow, but basically, a 
KDF may need all of the per-key derivation input to calculate the 
total length of the output key stream as an internal input to the KDF 
before ever emitting a single key.   Also - how exactly were you 
planning on keying the KDF? I guess you could pass that in in the 
KeyDerivation.getInstance() call or as part of the algorithmParameter 
but probably makes more sense to keep the KDF instance key-free 
to allow for reuse.
Well, let's get the easy one out of the way.  As you suspected I 
planned to pass the SecretKey in via AlgorithmParameterSpec.  The 
three classes unfortunately didn't show that.  Maybe on the next 
iteration I can put an HkdfParameterSpec in there just as a sample so 
folks can see that where the key comes in.  The reason I went that way 
was because the goal was to provide all algorithm paramters at 
instantiation time, and the SecretKey was just another input.  I don't 
know if just making the KDF key-free would be enough for reuse, at 
least not for all cases.  Thinking about HKDF and TLS 1.3 for 
instance, the key is the same for a collection of keys (like the 
client and server app traffic master keys that come from the master 
secret, for instance) - what changes are the other inputs to HKDF.


Yup - but that's easily handled through the new initialization call - 
which again matches the way Cipher, Signature and KeyAgreement do 
things.   Simplifying (??) the interface just to make one use case 
easier is probably not a great tradeoff.





One issue that came up on an early internal rev of the API was that we 
didn't want to separate instantiation and initialization, so all the 
inputs to the KDF now come in at getInstance time through 
AlgorithmParameterSpecs, rather than doing getInstance/init/... like 
KeyAgreement does.  I wonder if it would be OK to still have an init 
(and a reset as you wanted) method so we can provide new inputs 
top-to-bottom into the KDF object.  All the getInstance forms would 
stay more or less the same, so there's no way to make a KDF object 
without it being in an initialized state.  But when you need new 
inputs you don't have to make a new object.  I like being able to 
reuse the object and reset it to its starting state.  I don't know if 
the folks that brought up the instance/init issue would have a problem 
with that.  I think we're still adhering to the spirit of what they 
wanted to see since getInstance still gives you a fully initialized 
object.


As I noted in my other email, that's not the general form of a contract 
in the JCA.




That's a bit different than what you're talking about with your 
initialize method, I kinda birdwalked a bit.  Let me ask a couple 
questions: When you proposed initialize(), were you envisioning that 
applications would always need to call it before derive*?


Yes, init would always need to be called before you begin derives. A KDF 
call would require an instantiation (where you pass the parameters of 
the mechanism - 

Re: RFR [10]: JDK-8182484: Remove 1024-bit default requirement from javadoc of java.security.interfaces.DSAKeyPairGenerator

[Valerie Peng]

Hi, Sean,

I updated the webrev in place - now this change contains only javadoc 
update of DSAKeyPairGenerator interface.

CSR has also been updated accordingly. Could you please take a look?

Thanks,
Valerie

On 11/2/2017 6:24 PM, Valerie Peng wrote:

Sean,

Could you help review this RFE below? It's mainly the javadoc update 
of java.security.interfaces.DSAKeyPairGenerator which replaces the 
1024-bit default value with provider-specific one and removal of the 
earlier changes for working around this javadoc limitation. I reused 
the wordings from existing security classes.


RFE: https://bugs.openjdk.java.net/browse/JDK-8182484
Webrev: http://cr.openjdk.java.net/~valeriep/8182484/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8190569

Thanks,
Valerie

Re: [10] RFR : 8186628 : SSL session cache can cause a scalability bottleneck

[Roger Riggs]

Hi Ivan,

One idea to consider is an indirection that spreads the work over 
multiple Cache implementations.
Similar to what ConcurrentHashMap does, doing an early fan out to 
multiple Caches based on the key.
If it was keyed to the same key as the cache, it would be able to take 
advantage of re-using the contexts.

Though I'm not sure how to size or re-size the index based on load or ...

I would think that using a 'small' cache size would bound the expunge 
time and still allow some-reuse.


$.02, Roger


On 11/8/2017 2:09 PM, Ivan Gerasimov wrote:


Thank you Bernd for looking into this!


On 11/7/17 11:42 PM, Bernd Eckenfels wrote:

Hello,

There is already a property to set the cache size, would it be enough 
to re-purpose a cache size of 0 to turn it off?


Currently, setting the cache size to 0 means that it is unbounded, so 
that the entries are removed from the cache only when they get expired.


Are there numbers to show when this is actually a problem? Is this 
only for 100% Cache misses?
We've seen dumps with lots of threads blocked waiting on the 
Cache.get()/put()/remove().
This is primarily due to the time spent in the cache cleaning routines 
(see 
sun.security.util.MemoryCache.emptyQueue()/expungeExpiredEntries()), 
which are executed inside the synchronization block.
This time is linear on the size of cache, but limiting the cache size 
doesn't always help either, as the amount of cleanup work also 
increases with a bounded cache.


Allowing to avoid to use the cache removed this bottleneck and under 
certain conditions the throughput increased from 35 to 120 sessions 
per second.


Please note that the proposed option javax.net.ssl.needCacheSessions 
will be true by default, so the default behavior will not change.
Only in specific situation, if it is proved that turning off the cache 
will improve performance, this option will be recommended to be set to 
false.



Maybe the cache itself needs some optimizations?

Certainly, it would be very good to optimize the cache implementation!
I've made a few attempts, but failed to achieve a significant 
improvement in different scenarios.
The complication is due to the two requirements: maintaining fixed 
cache capacity and maintaining FIFO order when removing the entries.  
This makes it hard to use the concurrent data structures as is.


Still, I'm totally for the cache optimization in JDK 10, if it is 
possible.
However, if it is done, it would not be probably backported to the 
earlier releases.


And I'm going to propose to backport the proposed fix with the option 
to turn off the cache, as it will be useful for some currently running 
applications.


With kind regards,
Ivan

(It is hard to imagine that a saved handshake does not compensate for 
hundreds of gets - especially if the current version still would 
generate a cache key)


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net

*From:* security-dev  on 
behalf of Ivan Gerasimov 

*Sent:* Wednesday, November 8, 2017 3:24:54 AM
*To:* security-dev@openjdk.java.net
*Subject:* [10] RFR : 8186628 : SSL session cache can cause a 
scalability bottleneck

Hello everybody!

The class sun.security.ssl.SSLSessionContextImpl maintains caches for
the sessions reuse.
Access to the cache from threads is serialized.
It was reported that under heavy load the time of waiting for the turn
to access the synchronized methods outweighs the time of creating a new
session.

It is proposed to introduce a flag that will allow to avoid using the
cache altogether.
Would you please help review the proposed fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8186628
WEBREV: http://cr.openjdk.java.net/~igerasim/8186628/00/webrev/ 



--
With kind regards,
Ivan Gerasimov



--
With kind regards,
Ivan Gerasimov

Re: [10] RFR : 8186628 : SSL session cache can cause a scalability bottleneck

[Ivan Gerasimov]

Thank you Bernd for looking into this!


On 11/7/17 11:42 PM, Bernd Eckenfels wrote:

Hello,

There is already a property to set the cache size, would it be enough 
to re-purpose a cache size of 0 to turn it off?


Currently, setting the cache size to 0 means that it is unbounded, so 
that the entries are removed from the cache only when they get expired.


Are there numbers to show when this is actually a problem? Is this 
only for 100% Cache misses?
We've seen dumps with lots of threads blocked waiting on the 
Cache.get()/put()/remove().
This is primarily due to the time spent in the cache cleaning routines 
(see 
sun.security.util.MemoryCache.emptyQueue()/expungeExpiredEntries()), 
which are executed inside the synchronization block.
This time is linear on the size of cache, but limiting the cache size 
doesn't always help either, as the amount of cleanup work also increases 
with a bounded cache.


Allowing to avoid to use the cache removed this bottleneck and under 
certain conditions the throughput increased from 35 to 120 sessions per 
second.


Please note that the proposed option javax.net.ssl.needCacheSessions 
will be true by default, so the default behavior will not change.
Only in specific situation, if it is proved that turning off the cache 
will improve performance, this option will be recommended to be set to 
false.



Maybe the cache itself needs some optimizations?

Certainly, it would be very good to optimize the cache implementation!
I've made a few attempts, but failed to achieve a significant 
improvement in different scenarios.
The complication is due to the two requirements: maintaining fixed cache 
capacity and maintaining FIFO order when removing the entries.  This 
makes it hard to use the concurrent data structures as is.


Still, I'm totally for the cache optimization in JDK 10, if it is possible.
However, if it is done, it would not be probably backported to the 
earlier releases.


And I'm going to propose to backport the proposed fix with the option to 
turn off the cache, as it will be useful for some currently running 
applications.


With kind regards,
Ivan

(It is hard to imagine that a saved handshake does not compensate for 
hundreds of gets - especially if the current version still would 
generate a cache key)


Gruss
Bernd

Gruss
Bernd
--
http://bernd.eckenfels.net

*From:* security-dev  on behalf 
of Ivan Gerasimov 

*Sent:* Wednesday, November 8, 2017 3:24:54 AM
*To:* security-dev@openjdk.java.net
*Subject:* [10] RFR : 8186628 : SSL session cache can cause a 
scalability bottleneck

Hello everybody!

The class sun.security.ssl.SSLSessionContextImpl maintains caches for
the sessions reuse.
Access to the cache from threads is serialized.
It was reported that under heavy load the time of waiting for the turn
to access the synchronized methods outweighs the time of creating a new
session.

It is proposed to introduce a flag that will allow to avoid using the
cache altogether.
Would you please help review the proposed fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8186628
WEBREV: http://cr.openjdk.java.net/~igerasim/8186628/00/webrev/ 



--
With kind regards,
Ivan Gerasimov



--
With kind regards,
Ivan Gerasimov