<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Sean, <br>
<br>
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.<br>
<br>
Perhaps we could find a better name for <span class="new">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.<br>
<br>
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. <br>
<br>
I could not find the file: test/lib/jdk/test/lib/security/</span>TestJdkSecurityPropertyModification.java<span
class="new"><br>
<br>
Thanks<br>
Erik<br>
<br>
</span></div>
<blockquote
cite="mid:43073e02-7e92-9e93-398d-d37a778a56d3@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<p>With JDK-8203629 now pushed, I've re-based my patch on latest
jdk/jdk code.</p>
<p>Some modifications also made based on off-thread suggestions :
<br>
</p>
<p>src/java.base/share/classes/java/security/Security.java<br>
</p>
<p>* Only record JDK security properties for now (i.e. those in
java.security conf file) <br>
- we can consider a new event to record non-JDK security
events if demand comes about<br>
- new event renamed to <b>Jdk</b>SecurityPropertyModificationEvent</p>
<p>src/java.base/share/classes/sun/security/x509/X509CertImpl.java<br>
</p>
<p>* Use hashCode() equality test for storing certs in List. <br>
</p>
<p>Tests updated to capture these changes</p>
<p>src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java</p>
<p>* Verify that self signed certs exercise the modified code
paths</p>
<p>I've added new test functionality to test use of self signed
cert.</p>
<p>webrev : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoffeys/webrev.8148188.v7/webrev/">http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/</a><br>
</p>
<pre class="moz-signature" cols="72">Regards,
Sean.</pre>
<div class="moz-cite-prefix">On 17/10/18 11:25, Seán Coffey wrote:<br>
</div>
<blockquote
cite="mid:99c29269-1332-1a2d-c2a2-4b7479177488@oracle.com"
type="cite">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. <br>
<br>
Some of the main changes worthy of highlighting : <br>
<br>
* Separate out the tests for JFR and Logger events <br>
* Rename some events <br>
* Normalize the data view for X509ValidationEvent (old event
name : CertChainEvent) <br>
* Introduce CertificateSerialNumber type to make use of the
@Relational JFR annotation. <br>
* Keep calls for JFR event operations local to call site to
optimize jvm compilation <br>
<br>
webrev : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoffeys/webrev.8148188.v6/webrev/">http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/</a>
<br>
<br>
This code is dependent on the JDK-8203629 enhancement being
integrated. <br>
<br>
Regards, <br>
Sean. <br>
<br>
On 10/07/18 13:50, Seán Coffey wrote: <br>
<blockquote type="cite">Erik, <br>
<br>
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. <br>
<br>
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. <br>
<br>
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. <br>
<br>
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. <br>
<br>
I've removed the variable names using underscore. Also
optimized some variable assignments in
X509Impl.commitEvent(..) <br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecoffeys/webrev.8148188.v5/webrev/">http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/</a>
<br>
<br>
regards, <br>
Sean. <br>
<br>
On 09/07/2018 18:01, Seán Coffey wrote: <br>
<blockquote type="cite">Erik, <br>
<br>
Thanks for reviewing. Comments inline.. <br>
<br>
On 09/07/18 17:21, Erik Gahlin wrote: <br>
<blockquote type="cite">Thanks Sean. <br>
<br>
Some feedback on the code in the security libraries. <br>
<br>
- We should use camel case naming convention for variables
(not underscore). <br>
</blockquote>
Sure. I see two offending variable names which I'll fix up.
<br>
<blockquote type="cite"> <br>
- Looking at sun/security/ssl/Finished.java, <br>
<br>
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. <br>
</blockquote>
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. <br>
<br>
regards, <br>
Sean. <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>