RFR: 8148188: Enhance the security libraries to record events of interest

Tue Nov 13 18:53:09 UTC 2018

Looking good, a couple of comments/questions:

* src/java.base/share/classes/java/security/Security.java

The isJdkSecurityProperty method could return false positives, for 
example there may be a non-JDK property starting with "security.". I was 
thinking it would be better to put all the JDK property names in a 
HashSet which is populated by the static initialize() method, and only 
if event logging is enabled. Then setProperty can just check if the 
property name is in this set.

* src/java.base/share/classes/sun/security/x509/X509CertImpl.java

  158     // Event recording cache list
  159     private List<Integer> recordedCerts;

Shouldn't this be static? Otherwise each certificate would have it's own 
instance and duplicates would not be detected.

The rest of my comments are mostly minor stuff, and nits:

* 
src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java

245         if(xve.shouldCommit() || EventHelper.isLoggingSecurity()) {

add space before "(xve"

* src/java.base/share/classes/sun/security/ssl/Finished.java

1097 }

indent

1098             if(event.shouldCommit()) {

add space before "(event"

* src/java.base/share/classes/sun/security/x509/X509CertImpl.java

update copyright

* src/java.base/share/classes/jdk/internal/event/EventHelper.java

35  * A helper class to have events logged to a JDK Event Logger

Add '.' at end of sentence.

* src/java.base/share/classes/jdk/internal/event/TLSHandshakeEvent.java

38: remove blank line

* src/java.base/share/classes/jdk/internal/event/X509ValidationEvent.java

30  * used in X509 cert path validation

Add '.' at end of sentence.

* src/jdk.jfr/share/classes/jdk/jfr/events/X509ValidationEvent.java

47: remove blank line

* test/jdk/jdk/security/logging/TestTLSHandshakeLog.java

45         l.addExpected("SunJSSE Test Serivce");

Is that a typo? "Serivce"

* test/jdk/jdk/security/logging/TestX509ValidationLog.java

54: remove blank line

* test/lib/jdk/test/lib/security/SSLSocketTest.java

Why is this file included? It looks like a duplicate of SSLSocketTemplate.

--Sean

On 11/6/18 3:46 AM, Seán Coffey wrote:
> With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk code.
> 
> Some modifications also made based on off-thread suggestions :
> 
> src/java.base/share/classes/java/security/Security.java
> 
> * Only record JDK security properties for now (i.e. those in 
> java.security conf file)
>    - we can consider a new event to record non-JDK security events if 
> demand comes about
>    - new event renamed to *Jdk*SecurityPropertyModificationEvent
> 
> src/java.base/share/classes/sun/security/x509/X509CertImpl.java
> 
> * Use hashCode() equality test for storing certs in List.
> 
> Tests updated to capture these changes
> 
> src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java 
> 
> 
> * Verify that self signed certs exercise the modified code paths
> 
> I've added new test functionality to test use of self signed cert.
> 
> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/
> 
> Regards,
> Sean.
> 
> On 17/10/18 11:25, Seán Coffey wrote:
>> I'd like to re-boot this review. I've been working with Erik off 
>> thread who's been helping with code and test design.
>>
>> Some of the main changes worthy of highlighting :
>>
>> * Separate out the tests for JFR and Logger events
>> * Rename some events
>> * Normalize the data view for X509ValidationEvent (old event name : 
>> CertChainEvent)
>> * Introduce CertificateSerialNumber type to make use of the 
>> @Relational JFR annotation.
>> * Keep calls for JFR event operations local to call site to optimize 
>> jvm compilation
>>
>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/
>>
>> This code is dependent on the JDK-8203629 enhancement being integrated.
>>
>> Regards,
>> Sean.
>>
>> On 10/07/18 13:50, Seán Coffey wrote:
>>> Erik,
>>>
>>> After some trial edits, I'm not so sure if moving the event & logger 
>>> commit code into the class where it's used works too well after all.
>>>
>>> In the code you suggested, there's an assumption that calls such as 
>>> EventHelper.certificateChain(..) are low cost. While that might be 
>>> the case here, it's something we can't always assume. It's called 
>>> once for the JFR operation and once for the Logger operation. That 
>>> pattern multiplies itself further in other Events. i.e. set up and 
>>> assign the variables for a JFR event without knowing whether they'll 
>>> be needed again for the Logger call. We could work around it by 
>>> setting up some local variables and re-using them in the Logger code 
>>> but then, we're back to where we were. The current EventHelper design 
>>> eliminates this problem and also helps to abstract the 
>>> recording/logging functionality away from the functionality of the 
>>> class itself.
>>>
>>> It also becomes a problem where we record events in multiple 
>>> different classes (e.g. SecurityPropertyEvent). While we could leave 
>>> the code in EventHelper for this case, we then have a mixed design 
>>> pattern.
>>>
>>> Are you ok with leaving as is for now? A future wish might be one 
>>> where JFR would handle Logger via its own framework/API in a future 
>>> JDK release.
>>>
>>> I've removed the variable names using underscore. Also optimized some 
>>> variable assignments in X509Impl.commitEvent(..)
>>>
>>> http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/
>>>
>>> regards,
>>> Sean.
>>>
>>> On 09/07/2018 18:01, Seán Coffey wrote:
>>>> Erik,
>>>>
>>>> Thanks for reviewing. Comments inline..
>>>>
>>>> On 09/07/18 17:21, Erik Gahlin wrote:
>>>>> Thanks Sean.
>>>>>
>>>>> Some feedback on the code in the security libraries.
>>>>>
>>>>> - We should use camel case naming convention for variables (not 
>>>>> underscore).
>>>> Sure. I see two offending variable names which I'll fix up.
>>>>>
>>>>> - Looking at sun/security/ssl/Finished.java,
>>>>>
>>>>> I wonder if it wouldn't be less code and more easy to read, if we 
>>>>> would commit the event in a local private function and then use the 
>>>>> EventHelper class for common functionality.
>>>> I'm fine with your suggested approach. I figured that tucking the 
>>>> recording/logging logic away in a helper class also had benefits but 
>>>> I'll re-factor to your suggested style unless I hear objections.
>>>>
>>>> regards,
>>>> Sean.
>>>>
>>>
>>
> 