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

Fri Nov 16 15:31:54 UTC 2018

I've renamed the 'peerCertificateId' variable and label in 
TLSHandshakeEvent to 'certificateId'. This allows the event data to be 
co-displayed in JMC views which share the same type of data 
(@CertificateId). I've uploaded an example screenshot [1]

I've also made an adjustment to the TestModuleEvents test to disregard 
the jdk.proxy1 module for test purposes.

http://cr.openjdk.java.net/~coffeys/webrev.8148188.v10/webrev/

[1] 
http://cr.openjdk.java.net/~coffeys/jmc_screenshots/shared_selection_certificateId.png/Screenshot%20from%202018-11-16%2015%3a28%3a59.png

Regards,
Sean.

On 14/11/18 21:09, Seán Coffey wrote:
> Hi Sean,
>
> comments inline..
>
> On 13/11/2018 18:53, Sean Mullan wrote:
>> 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.
> after further discussion, we've decided to log all properties operated 
> on via Security.setProperty(String, String). It think it makes sense. 
> The contents of a JFR recording should always be treated as sensitive. 
> Keep in mind also that these new JFR Events will be off by default.
>>
>> * src/java.base/share/classes/sun/security/x509/X509CertImpl.java
> Good points here. I've taken on board your suggestion to move this 
> code logic to the sun/security/provider/X509Factory.java class as per 
> your later email suggestion.
>
>> 45         l.addExpected("SunJSSE Test Serivce");
>>
>> Is that a typo? "Serivce"
>>
> That's a typo in the test certificate details. We should fix it up via 
> another bug record.
>
>> * 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.
> Yes - it's almost identical to SSLSocketTemplate except that 
> SSLSocketTemplate doesn't belong to a package. I had a hard time 
> incorporating its use into the jtreg tests via the @lib syntax. I 
> think it's best if we open another JBS issue to migrate other uses of 
> SSLSocketTemplate to jdk.test.lib.security.SSLSocketTemplate
>
> I've cleaned up the various typo/syntax issues that you've highlighted 
> also.
> http://cr.openjdk.java.net/~coffeys/webrev.8148188.v9/webrev/index.html
>
> regards,
> Sean.
>
>> 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
>>
>>
>> --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.
>>>>>>
>>>>>
>>>>
>>>
>