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

[Seán Coffey]

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.
>>>>>
>>>>
>>>
>>