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. * src/java.base/share/classes/sun/security/x509/X509CertImpl.java 158 // Event recording cache list 159 private List<Integer> recordedCerts; Shouldn't this be static? Otherwise each certificate would have it's own instance and duplicates would not be detected. 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 45 l.addExpected("SunJSSE Test Serivce"); Is that a typo? "Serivce" * 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. --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.