Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[dean . long]

Thanks Mandy.  I also appreciate you noticing (off-list) that I can 
remove the extra space in "Class " in several places.  I have updated 
webrev.4 in place.


dl

On 11/2/18 1:55 PM, Mandy Chung wrote:

Hi Dean,

I reviewed webrev.4 version.  It looks good.  Happy to see moving the 
doPrivileged support to Java and the performance improvement.


On 10/31/18 3:23 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need 
to guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method 
returns.
Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I kept 
the old native privileged stack and compared the results of the old 
and new implementations for each getContext call, which did catch 
some early bugs.  The changes were also examined by internal security 
experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].




FYI.  Sean and I also did some experiment to replace 
JVM_GetStackAccessControlContext with StackWalker some time ago. 
Another potential area to move the code from VM to Java for the future 
as David explored and probably involves  performance work in the stack 
walker.


Mandy

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[Mandy Chung]

Hi Dean,

I reviewed webrev.4 version.  It looks good.  Happy to see moving the 
doPrivileged support to Java and the performance improvement.


On 10/31/18 3:23 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need to 
guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method 
returns.
Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I kept 
the old native privileged stack and compared the results of the old 
and new implementations for each getContext call, which did catch some 
early bugs.  The changes were also examined by internal security 
experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is approximate 
50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here [2].




FYI.  Sean and I also did some experiment to replace 
JVM_GetStackAccessControlContext with StackWalker some time ago. Another 
potential area to move the code from VM to Java for the future as David 
explored and probably involves  performance work in the stack walker.


Mandy

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession

[Xuelei Fan]

Thanks for the review and suggestions, Chris and Sean.

I just finalized the CSR.

Thanks,
Xuelei

On 11/2/2018 5:58 AM, Chris Hegarty wrote:

Thanks for the updates Xuelei, some minor comments inline..

On 1 Nov 2018, at 23:42, Xuelei Fan > wrote:


On 11/1/2018 11:24 AM, Sean Mullan wrote:

On 10/31/18 11:52 AM, Chris Hegarty wrote:

Xuelei,

On 30/10/18 20:55, Xuelei Fan wrote:

Hi,

For the current HttpsURLConnection, there is not much security 
parameters exposed in the public APIs.  An application may need 
richer information for the underlying TLS connections, for example 
the negotiated TLS protocol version.


Please let me know if you have concerns to add a new method 
HttpsURLConnection.getSSLSession() and deprecate the duplicated 
methods, by the end of Nov. 2, 2018.


Here is the proposal:
https://bugs.openjdk.java.net/browse/JDK-8213161
Are there any security issues associated with returning the 
SSLSession, since it is mutable?
It should be fine.  The update APIs of the session (invalidating, bind 
values) does not impact the connection.


Alternatively, as is done in the new HTTP Client, an immutable
SSLSession instance can be returned.

+ *   SHOULD override this method with appropriate 
implementation.

s/appropriate/an appropriate/
I would probably not capitalize "SHOULD" and just say "should". 
"SHOULD" is more common in RFCs. I don't see that much in javadocs.
+ * @implNote The JDK Reference Implementation supports this 
operation.
+ *   As an application may have to use this operation 
for more

+ *   security parameters, it is recommended to support this
+ *   operation in all implementations.
I think it should be obvious that the JDK implementation would 
override this method so not sure that first sentence is necessary. 
The other sentence seems like it could be combined with the previous 
sentence, ex:
"Subclasses should override this method with an appropriate 
implementation since an application may need to access additional 
parameters associated with the SSL session."

Updated accordingly, in the CSR and webrev:
https://bugs.openjdk.java.net/browse/JDK-8213161


The CSR looks good. I made a few minor edits to the verbiage
and added myself as reviewer.

The title will need to be updated to reflect the addition of the
new method in SecureCacheResponse. Maybe:

"Add SSLSession accessors to HttpsURLConnection and
SecureCacheResponse"

-Chris.

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession

[Chris Hegarty]

Thanks for the updates Xuelei, some minor comments inline..

> On 1 Nov 2018, at 23:42, Xuelei Fan  wrote:
> 
> On 11/1/2018 11:24 AM, Sean Mullan wrote:
>> On 10/31/18 11:52 AM, Chris Hegarty wrote:
>>> Xuelei,
>>> 
>>> On 30/10/18 20:55, Xuelei Fan wrote:
 Hi,
 
 For the current HttpsURLConnection, there is not much security parameters 
 exposed in the public APIs.  An application may need richer information 
 for the underlying TLS connections, for example the negotiated TLS 
 protocol version.
 
 Please let me know if you have concerns to add a new method 
 HttpsURLConnection.getSSLSession() and deprecate the duplicated methods, 
 by the end of Nov. 2, 2018.
 
 Here is the proposal:
  https://bugs.openjdk.java.net/browse/JDK-8213161
>> Are there any security issues associated with returning the SSLSession, 
>> since it is mutable?
> It should be fine.  The update APIs of the session (invalidating, bind 
> values) does not impact the connection.

Alternatively, as is done in the new HTTP Client, an immutable
SSLSession instance can be returned.

>> + *   SHOULD override this method with appropriate 
>> implementation.
>> s/appropriate/an appropriate/
>> I would probably not capitalize "SHOULD" and just say "should". "SHOULD" is 
>> more common in RFCs. I don't see that much in javadocs.
>> + * @implNote The JDK Reference Implementation supports this operation.
>> + *   As an application may have to use this operation for more
>> + *   security parameters, it is recommended to support this
>> + *   operation in all implementations.
>> I think it should be obvious that the JDK implementation would override this 
>> method so not sure that first sentence is necessary. The other sentence 
>> seems like it could be combined with the previous sentence, ex:
>> "Subclasses should override this method with an appropriate implementation 
>> since an application may need to access additional parameters associated 
>> with the SSL session."
> Updated accordingly, in the CSR and webrev:
>https://bugs.openjdk.java.net/browse/JDK-8213161 
> 
The CSR looks good. I made a few minor edits to the verbiage
and added myself as reviewer.

The title will need to be updated to reflect the addition of the
new method in SecureCacheResponse. Maybe:

"Add SSLSession accessors to HttpsURLConnection and
SecureCacheResponse"

-Chris.