On 2018-06-27 21:14, Seán Coffey wrote:
On 27/06/2018 19:57, Xuelei Fan wrote:
Hi Sean,
I may reply in several replies.
PKIXMasterCertPathValidator.java
--------------------------------
+ CertChainEvent cce = new CertChainEvent();
+ if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+ String c = reversedCertList.stream()
+ .map(x -> x.getSerialNumber().toString(16))
+ .collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+ }
No matter the event or the JFR mechanism is enabled or not, the above
code will create a new instance. Is the return value of
cce.isEnabled() dynamically changed or static?
This is a requirement from the JFR framework. All their
event.isEnabled calls are instance methods and follow a similar
pattern. The JFR team assure me that the JIT can optimize away such
calls.
The JIT will most likely not be able to optimize if the event class
escapes.
From a JFR perspective, this would be the preferred layout:
EventA eventA= new EventA();
eventA.value = this.value;
eventA.commit();
and then do logging separately:
System.Logger.log(DEBUG, this.value)
With this layout, the JIT will remove the allocation and dead store.
If it is expensive to gather the data for the event, like the
CertChainEvent above, the following pattern should be used.
EventB eventB= new EventB ();
if (eventB.shouldCommit()) {
eventB.value = calculateValue();
eventB .commit();
}
If JFR is not enabled, shouldCommit returns false and the JIT should be
able to remove the allocation. This will be best from a performance
point of view, and also in my opinion from a maintenance and readability
perspective. Others may disagree.
Erik
Is there a need to support both logging and JFR? I'm new to record
events. I did not get the point to use both.
I was initially hoping to concentrate on just JFR events but I got
strong feedback to also consider use of Logger (esp. in cases where
the jdk.jfr module is not available)
regards,
Sean.
Thanks,
Xuelei
On 6/26/2018 3:18 PM, Seán Coffey wrote:
Erik,
I rebased the patch with TLS v1.3 integration today. I hadn't
realized how much the handshaker code had changed. Hopefully, I'll
get a review from security dev team on that front.
Regards the JFR semantics, I believe the edits should match majority
of requests . I still have a preference for the test infra design
for these new logger/JFR tests used in version 1 of webrev. I think
it makes sense to keep the test functionality together - no sense in
separating them out into different components IMO. Also, some of the
edits to the JFR testing were made to test for the new dual
log/record functionality. I might catch up with you tomorrow to see
what the best arrangement would be.
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/
regards,
Sean.
On 25/06/2018 21:22, Seán Coffey wrote:
Many thanks for the review comments Erik. Replies inline.
On 24/06/2018 14:21, Erik Gahlin wrote:
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.
Makes sense. I'll remove that parameter.
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;
The CertificateValidity class operates on Date Object values. I'll
work with the Date.getTime() method then (and update the Logger
formatting)
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?
Yes - that looks like the wrong Description. I'll review all of
these fields.
@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".
Yes - Open to suggestions. "Security" sounds like a good suggestion.
@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'll take a look at that pattern again. I've separated out the
current tests into an (a) outer test to analyze the logger output
and (b) the inner test which checks for JFR correctness. I did
include extra logic to make sure that the EventHelper configuration
was working correctly. "Events.assertField" looks very helpful.
Thanks for the pointer.
Let me take on board the suggestions below and get a new webrev out
on Tuesday.
regards,
Sean.
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/
