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

[Seán Coffey]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20181106/5c78617f/attachment.htm>