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

Sean Mullan sean.mullan at oracle.com
Fri Nov 16 18:12:17 UTC 2018 

Updated webrev looks good.

--Sean

On 11/16/18 10:31 AM, Seán Coffey wrote:
> 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.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>

More information about the core-libs-dev mailing list