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

[Erik Gahlin]

Hi Sean,

I think we could still call the event 
"jdk.SecurityPropertyModification", but add a @Description that explains 
that events are only emitted for the JDK due to security concerns. If we 
at a later stage want to include user events, we could add those and 
remove the @Description, possibly with a setting where you can specify 
scope, or perhaps a new event all together.

Perhaps we could find a better name for the field validationEventNumber? 
It sounds like we have an event number, which is not really the case. We 
have a counter for the validation id.

I noticed that you use hashCode as an id. I assume this is to simplify 
the implementation? That is probably OK, the risk of a collision is 
perhaps low, but could you make the field a long, so we in the future 
could add something more unique (Flight Recorder uses compressed 
integers, so a long uses the same amount of disk space as an int). Could 
you also rename the field and the annotation, perhaps certificationId 
could work, so we don't leak how the id was implemented.

I could not find the file: 
test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java

Thanks
Erik

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