Hi Sean,
Some of the changes in the webrev belongs to JDK-8203629 and should be
removed for clarity.
Some initial comments:
default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The purpose of
the control attribute is so to provide parameterized configuration of
events for JMC. As it is now, the security events will be enabled when
a user turns on the exception events.
X509CertEvent:
You should use milliseconds since epoch to represent a date instead of a
string value, i.e.
@Label("Valid From")
@Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
public long validFrom;
@Label("Valid Until")
@Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
public long validUntil;
This following annotation adds little value
@Description("Details of Security Property")
I would either remove the annotation, or provide information that helps
a user understand the event. For instance, what is X509, and what kind
of certificates are we talking about?
@Category("Java Application")
I'm a bit worried that we will pollute the "Java Application" namespace
if we add lots of JDK events in that category. We may want to add a new
top level category "Java Development Kit", analogous to the "Java
Virtual Machine" category, and put all security related events in
subcategory, perhaps called "Security".
@Label("X509Cert")
The label should be human readable name, with spaces, title cased etc. I
would recommend "X.509 Certificate". In general, avoid abbreviations
like "certs" and instead use labels such as "Certificate Chain". The
label should be short and not a full sentence.
For details see,
https://docs.oracle.com/javase/10/docs/api/jdk/jfr/Label.html
I think it would be good to separate testing of JFR and logging into
different files / tests. I would prefer that the test name is the same
as the event prefixed with "Test", i.e TestX509CertificateEvent, as this
is the pattern used by other JFR tests.
I reworked one of the tests to how I like to see it:
/*
* @test
* @key jfr
* @library /test/lib
* @run main/othervm jdk.jfr.event.security.TestX509CertificateEvent
*/
public class TestX509CertificateEvent {
private static final String CERTIFICATE_1 = "...";
private static final String CERTIFICATE_2 = "...";
public static void main(String... args) throws CertificateException {
Recording r = new Recording();
r.enable(EventNames.X509Certificate).withoutStackTrace();
r.start();
CertificateFactory cf = CertificateFactory.getInstance("X.509");
cf.generateCertificate(new
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
cf.generateCertificate(new
ByteArrayInputStream(CERTIFICATE_2.getBytes()));
// Make sure only one event per certificate
cf.generateCertificate(new
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
cf.generateCertificate(new
ByteArrayInputStream(CERTIFICATE_2.getBytes()));
r.stop();
List<RecordedEvent> events = Events.fromRecording(r);
Asserts.assertEquals(events.size(), 2, "Expected two X.509
Certificate events");
assertEvent(events, "1000", "SHA256withRSA",
"CN=SSLCertificate, O=SomeCompany",
"CN=Intermediate CA Cert, O=SomeCompany",
"RSA", 2048);
assertEvent(events, "1000", "SHA256withRSA",
"CN=SSLCertificate, O=SomeCompany",
"CN=Intermediate CA Cert, O=SomeCompany",
"RSA", 2048);
}
private static void assertEvent(List<RecordedEvent> events, String
certID, String algId, String subject,
String issuer, String keyType, int length) throws Exception {
for (RecordedEvent e : events) {
if (e.getString("serialNumber").equals(certID)) {
Events.assertField(e, "algId").equal(algId);
...
return;
}
}
System.out.println(events);
throw new Exception("Could not find event with Cert ID: " +
certID);
}
}
The reworked example uses the Events.assertField method, which will give
context if the assertion fails. Keeping the test simple, means it can be
analyzed quickly if it fails in testing. There is no new test framework
to learn, or methods to search for, and it is usually not hard to
determine if the failure is product, test or infrastructure related, and
what component (team) should be assigned the issue. The use of
EventNames.X509Certificate means all occurrences of the event can be
tracked done in an IDE using find by reference.
Thanks
Erik
Following on from the recent JDK-8203629 code review, I'd like to
propose enhancements on how we can record events in security libs. The
introduction of the JFR libraries can give us much better ways of
examining JDK actions. For the initial phase, I'm looking to enhance
some key security library events in JDK 11 so that they can be either
recorded to JFR, logged to a traditional logger, or both.
Examples of how useful JFR recordings could be can be seen here :
http://cr.openjdk.java.net/~coffeys/event_snaps/X509Event_1.png
http://cr.openjdk.java.net/~coffeys/event_snaps/securityProp_1.png
http://cr.openjdk.java.net/~coffeys/event_snaps/securityProp_2.png
http://cr.openjdk.java.net/~coffeys/event_snaps/TLSEvent_1.png
securityProp_2.png gives an example of how the JFR recording can be
queried to quickly locate events of interest (in this case, code
setting the jdk.tls.* Security properties). I still need to clean up
the TLSEvents testcase to improve test coverage and hope to do that in
coming days.
JBS record :
* https://bugs.openjdk.java.net/browse/JDK-8148188
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v1/webrev/
