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.