RFR: 8148188: Enhance the security libraries to record events of interest
sean.coffey at oracle.com
Tue Nov 6 08:46:27 UTC 2018
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 :
* 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
* Use hashCode() equality test for storing certs in List.
Tests updated to capture these changes
* 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/
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 :
> * 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.
> On 10/07/18 13:50, Seán Coffey wrote:
>> 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
>> 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(..)
>> On 09/07/2018 18:01, Seán Coffey wrote:
>>> 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
>>> 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.
More information about the core-libs-dev