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.