RFR: 8148188: Enhance the security libraries to record events of interest
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/ -- Regards, Sean.
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/
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/
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/
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? 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. 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/
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.
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/
KeyUtil.java ------------ 167 public static String getKeyAlgorithm(Key key) { I may suggest remove this method and use Key.getAlgorithm() directly. The KeyUtil.getKeyAlgorithm() is incomplete, and may need additional maintenance if new key algorithms are added in the future. For example, in JDK 11, "RSASSA-PSS" and "XDH" are added, but we really forgot that we may need to update this file as well. On 6/27/2018 12:14 PM, 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.
Got it.
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)
There are three logging mechanisms: JFR, record event log and the component debugging log. It is a little bit overloaded to me. If jdk.jfr is not available, the existing component debugging log may be used instead. I think you must have weight this decision in the past, so I will accept your decision if you want to keep it. Thanks, Xuelei
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/
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/
Thanks for the update Erik. By default I'm proposing that the new JFR Events and Logger be disabled. As a result the event class shouldn't escape. If performance metrics highlight an issue, we should revisit. regards, Sean. On 27/06/2018 20:57, Erik Gahlin wrote:
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/ >
It's sufficient if an event object escapes to another method (regardless if JFR is enabled or not). Some more feedback: Rename event jdk.CertChain to jdk.CertificateChain Rename event jdk.X509Cert to jdk.X509Certificate Rename field certChain to certificateChain. Rename field serialNum to serialNumber Rename field algId to AlgorithmicId or AlgorithmicID Rename @Label("Ciphersuite") to @Label("Cipher Suite") Rename @Label("Cert Chain") to @Label("Certificate Chain") Rename @Label("Property Name") to "Name" or "Key" if that makes sense in the context? Rename @Label("Property Value") to "Value". Put events in a subcategory, i.e @Category({"Java Development Kit", "Security"}) Make classes final. What is the unit of the key in X509Certificate event? Bits? If that is the case, use @DataAmount(DataAmount.BITS) @Label("Serial numbers forming chain of trust") should not be a sentence. How about "Serial Numbers"? I think the tests are hard to read when two things are tested at the same time. It is also likely that if a test fail due to logging issues, it will be assigned to JFR because of the test name, even thought the issues is not JFR related. If you want to reuse code between tests, I would put it in testlibrary. Erik
Thanks for the update Erik. By default I'm proposing that the new JFR Events and Logger be disabled. As a result the event class shouldn't escape. If performance metrics highlight an issue, we should revisit.
regards, Sean.
On 27/06/2018 20:57, Erik Gahlin wrote:
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/ >> >
Comments inline. On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method (regardless if JFR is enabled or not).
Some more feedback:
Rename event jdk.CertChain to jdk.CertificateChain Rename event jdk.X509Cert to jdk.X509Certificate Rename field certChain to certificateChain. Rename field serialNum to serialNumber all above done. Rename field algId to AlgorithmicId or AlgorithmicID maybe "algorithm" works here also ? Rename @Label("Ciphersuite") to @Label("Cipher Suite") Rename @Label("Cert Chain") to @Label("Certificate Chain") Rename @Label("Property Name") to "Name" or "Key" if that makes sense in the context? Rename @Label("Property Value") to "Value". Put events in a subcategory, i.e @Category({"Java Development Kit", "Security"}) done. Make classes final. done. I had thought that the JFR mechanism required non-final classes. What is the unit of the key in X509Certificate event? Bits? If that is the case, use @DataAmount(DataAmount.BITS) Yes - it's essentially the bit length of the keys used. Let me look into that annotation usage. @Label("Serial numbers forming chain of trust") should not be a sentence. How about "Serial Numbers"?
I think the tests are hard to read when two things are tested at the same time. It is also likely that if a test fail due to logging issues, it will be assigned to JFR because of the test name, even thought the issues is not JFR related. I think we're always going to have some ownership issues when tests serve a dual purpose. I still think it makes sense to keep the test logic in one place. Any failures in these tests will most likely be owned by security team. (moving the tests to security directory is also an option)
If you want to reuse code between tests, I would put it in testlibrary. Let me check if there's any common patterns that could be added to the testlibary.
Thanks, Sean.
Erik
Thanks for the update Erik. By default I'm proposing that the new JFR Events and Logger be disabled. As a result the event class shouldn't escape. If performance metrics highlight an issue, we should revisit.
regards, Sean.
On 27/06/2018 20:57, Erik Gahlin wrote:
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/ >>> >> >
I've introduced a new test helper class in the jdk/test/lib/jfr directory to help with the dual test nature of the new tests. It's helped alot with test code duplication. Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits. The output is displayed in units like "KiB" - not the normal when examining key lengths used in X509Certificates. i.e a 2048 bit key gets displayed as "2 KiB" - I'd prefer to keep the 2048 display version. new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/ Regards, Sean. On 28/06/18 17:59, Seán Coffey wrote:
Comments inline.
On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method (regardless if JFR is enabled or not).
Some more feedback:
Rename event jdk.CertChain to jdk.CertificateChain Rename event jdk.X509Cert to jdk.X509Certificate Rename field certChain to certificateChain. Rename field serialNum to serialNumber all above done. Rename field algId to AlgorithmicId or AlgorithmicID maybe "algorithm" works here also ? Rename @Label("Ciphersuite") to @Label("Cipher Suite") Rename @Label("Cert Chain") to @Label("Certificate Chain") Rename @Label("Property Name") to "Name" or "Key" if that makes sense in the context? Rename @Label("Property Value") to "Value". Put events in a subcategory, i.e @Category({"Java Development Kit", "Security"}) done. Make classes final. done. I had thought that the JFR mechanism required non-final classes. What is the unit of the key in X509Certificate event? Bits? If that is the case, use @DataAmount(DataAmount.BITS) Yes - it's essentially the bit length of the keys used. Let me look into that annotation usage. @Label("Serial numbers forming chain of trust") should not be a sentence. How about "Serial Numbers"?
I think the tests are hard to read when two things are tested at the same time. It is also likely that if a test fail due to logging issues, it will be assigned to JFR because of the test name, even thought the issues is not JFR related. I think we're always going to have some ownership issues when tests serve a dual purpose. I still think it makes sense to keep the test logic in one place. Any failures in these tests will most likely be owned by security team. (moving the tests to security directory is also an option)
If you want to reuse code between tests, I would put it in testlibrary. Let me check if there's any common patterns that could be added to the testlibary.
Thanks, Sean.
Erik
Thanks for the update Erik. By default I'm proposing that the new JFR Events and Logger be disabled. As a result the event class shouldn't escape. If performance metrics highlight an issue, we should revisit.
regards, Sean.
On 27/06/2018 20:57, Erik Gahlin wrote:
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/ >>>> >>> >> >
On 29 Jun 2018, at 17:34, Seán Coffey <sean.coffey@oracle.com> wrote:
I've introduced a new test helper class in the jdk/test/lib/jfr directory to help with the dual test nature of the new tests. It's helped alot with test code duplication.
My thinking was to put things like the certificates in a separate file, i.e TestCertificates, and then have a logging test and a JFR test reuse it. One rationale for adding logging was to use it if JFR is not present. By putting the tests together, it becomes impossible to compile and test logging without having JFR.
Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits. The output is displayed in units like "KiB" - not the normal when examining key lengths used in X509Certificates. i.e a 2048 bit key gets displayed as "2 KiB" - I'd prefer to keep the 2048 display version.
We should not let the JMC GUI decide how units are specified. There will be other GUIs and this is the first event that uses bits, so I don’t think it is formatted that way because it was considered superior. Erik
new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/
Regards, Sean.
On 28/06/18 17:59, Seán Coffey wrote:
Comments inline.
On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method (regardless if JFR is enabled or not).
Some more feedback:
Rename event jdk.CertChain to jdk.CertificateChain Rename event jdk.X509Cert to jdk.X509Certificate Rename field certChain to certificateChain. Rename field serialNum to serialNumber all above done. Rename field algId to AlgorithmicId or AlgorithmicID maybe "algorithm" works here also ? Rename @Label("Ciphersuite") to @Label("Cipher Suite") Rename @Label("Cert Chain") to @Label("Certificate Chain") Rename @Label("Property Name") to "Name" or "Key" if that makes sense in the context? Rename @Label("Property Value") to "Value". Put events in a subcategory, i.e @Category({"Java Development Kit", "Security"}) done. Make classes final. done. I had thought that the JFR mechanism required non-final classes. What is the unit of the key in X509Certificate event? Bits? If that is the case, use @DataAmount(DataAmount.BITS) Yes - it's essentially the bit length of the keys used. Let me look into that annotation usage. @Label("Serial numbers forming chain of trust") should not be a sentence. How about "Serial Numbers"?
I think the tests are hard to read when two things are tested at the same time. It is also likely that if a test fail due to logging issues, it will be assigned to JFR because of the test name, even thought the issues is not JFR related. I think we're always going to have some ownership issues when tests serve a dual purpose. I still think it makes sense to keep the test logic in one place. Any failures in these tests will most likely be owned by security team. (moving the tests to security directory is also an option)
If you want to reuse code between tests, I would put it in testlibrary. Let me check if there's any common patterns that could be added to the testlibary.
Thanks, Sean.
Erik
Thanks for the update Erik. By default I'm proposing that the new JFR Events and Logger be disabled. As a result the event class shouldn't escape. If performance metrics highlight an issue, we should revisit.
regards, Sean.
On 27/06/2018 20:57, Erik Gahlin wrote:
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/ >>>>> >>>> >>> >>
As per request from Erik, I separated the tests out into individual ones to test the JFR and Logger functionality. I introduced a new separate test for the CertificateChainEvent event also. Originally this was wrapped into the TLSHandshakeEvent test. Thanks to Erik for extra refactoring and modifications carried out to clean up the code up further. http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/ This enhancement has a dependency on JDK-8203629 Regards, Sean. On 02/07/18 09:49, Erik Gahlin wrote:
On 29 Jun 2018, at 17:34, Seán Coffey <sean.coffey@oracle.com> wrote:
I've introduced a new test helper class in the jdk/test/lib/jfr directory to help with the dual test nature of the new tests. It's helped alot with test code duplication.
My thinking was to put things like the certificates in a separate file, i.e TestCertificates, and then have a logging test and a JFR test reuse it.
One rationale for adding logging was to use it if JFR is not present. By putting the tests together, it becomes impossible to compile and test logging without having JFR.
Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits. The output is displayed in units like "KiB" - not the normal when examining key lengths used in X509Certificates. i.e a 2048 bit key gets displayed as "2 KiB" - I'd prefer to keep the 2048 display version. We should not let the JMC GUI decide how units are specified. There will be other GUIs and this is the first event that uses bits, so I don’t think it is formatted that way because it was considered superior.
Erik
new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/
Regards, Sean.
On 28/06/18 17:59, Seán Coffey wrote:
Comments inline.
On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method (regardless if JFR is enabled or not).
Some more feedback:
Rename event jdk.CertChain to jdk.CertificateChain Rename event jdk.X509Cert to jdk.X509Certificate Rename field certChain to certificateChain. Rename field serialNum to serialNumber all above done. Rename field algId to AlgorithmicId or AlgorithmicID maybe "algorithm" works here also ? Rename @Label("Ciphersuite") to @Label("Cipher Suite") Rename @Label("Cert Chain") to @Label("Certificate Chain") Rename @Label("Property Name") to "Name" or "Key" if that makes sense in the context? Rename @Label("Property Value") to "Value". Put events in a subcategory, i.e @Category({"Java Development Kit", "Security"}) done. Make classes final. done. I had thought that the JFR mechanism required non-final classes. What is the unit of the key in X509Certificate event? Bits? If that is the case, use @DataAmount(DataAmount.BITS) Yes - it's essentially the bit length of the keys used. Let me look into that annotation usage. @Label("Serial numbers forming chain of trust") should not be a sentence. How about "Serial Numbers"?
I think the tests are hard to read when two things are tested at the same time. It is also likely that if a test fail due to logging issues, it will be assigned to JFR because of the test name, even thought the issues is not JFR related. I think we're always going to have some ownership issues when tests serve a dual purpose. I still think it makes sense to keep the test logic in one place. Any failures in these tests will most likely be owned by security team. (moving the tests to security directory is also an option) If you want to reuse code between tests, I would put it in testlibrary. Let me check if there's any common patterns that could be added to the testlibary.
Thanks, Sean.
Erik
Thanks for the update Erik. By default I'm proposing that the new JFR Events and Logger be disabled. As a result the event class shouldn't escape. If performance metrics highlight an issue, we should revisit.
regards, Sean.
On 27/06/2018 20:57, Erik Gahlin wrote:
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/ >>>>>>
Thanks Sean. Some feedback on the code in the security libraries. - We should use camel case naming convention for variables (not underscore). - 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. Instead of: private static void recordEvent(SSLSessionImpl session) { TLSHandshakeEvent hs_event = new TLSHandshakeEvent(); if (hs_event.isEnabled() || EventHelper.loggingSecurity()) { String certIDs = ""; try { certIDs = Stream.of(session.getPeerCertificates()) .filter(c -> c instanceof X509Certificate) .map(c -> (X509Certificate) c) .map(c -> c.getSerialNumber().toString(16)) .collect(Collectors.joining(", ")); } catch (SSLPeerUnverifiedException e) { certIDs = e.getMessage(); // not verified msg } EventHelper.commitTLSHandshakeEvent(hs_event, null, session.getPeerHost(), session.getPeerPort(), session.getCipherSuite(), session.getProtocol(), certIDs); } } ... public static void commitTLSHandshakeEvent(TLSHandshakeEvent event, Instant start, String remoteHost, int remotePort, String cipherSuite, String protocolVersion, String certChain) { if (event != null && event.shouldCommit()) { event.remoteHost = remoteHost; event.remotePort = remotePort; event.cipherSuite = cipherSuite; event.protocolVersion = protocolVersion; event.certificateChain = certChain; event.commit(); } if (loggingSecurity()) { String prepend = getDurationString(start); SECURITY_LOGGER.log(LOG_LEVEL, prepend + " TLSHandshake: {0}:{1,number,#}, {2}, {3}, {4}", remoteHost, remotePort, protocolVersion, cipherSuite, certChain); } } We would do: private static void logHandshake(SSLSessionImpl s) { TLSHandshakeEvent event = new TLSHandshakeEvent(); if (event.shouldCommit()) { event.remoteHost = s.getPeerHost(); event.remotePort = s.getPeerPort(); event.cipherSuite = s.getCipherSuite(); event.protocolVersion = s.getProtocol(); event.certificateChain = EventHelper.certificateChain(s.getPeerCerticates()); event.commit(); } if (EventHelper.isLoggingSecurity()) { EventHelper.SECURITY_LOGGER.log(LOG_LEVEL, " TLSHandshake: {0}:{1,number,#}, {2}, {3}, {4}", s.getPeerHost(), s.getPeerPort(), s.getProtocol(), s.getCipherSuite(), EventHelper.certificateChain(s.getPeerCerticates())); } } ... public static String certificateChain(Certificate[] certificates) { try { return Stream.of(certificates) .filter(c -> c instanceof X509Certificate) .map(c -> (X509Certificate) c) .map(c -> c.getSerialNumber().toString(16)) .collect(Collectors.joining(", ")); } catch (SSLPeerUnverifiedException e) { return e.getMessage(); // not verified msg } } It will also meanless overhead, since only one check is needed for the log and the JIT will be able to remove the allocation. Maybe a similar pattern could be used for the other events as well? Thanks Erik
As per request from Erik, I separated the tests out into individual ones to test the JFR and Logger functionality. I introduced a new separate test for the CertificateChainEvent event also. Originally this was wrapped into the TLSHandshakeEvent test.
Thanks to Erik for extra refactoring and modifications carried out to clean up the code up further.
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/
This enhancement has a dependency on JDK-8203629
Regards, Sean.
On 02/07/18 09:49, Erik Gahlin wrote:
On 29 Jun 2018, at 17:34, Seán Coffey <sean.coffey@oracle.com> wrote:
I've introduced a new test helper class in the jdk/test/lib/jfr directory to help with the dual test nature of the new tests. It's helped alot with test code duplication.
My thinking was to put things like the certificates in a separate file, i.e TestCertificates, and then have a logging test and a JFR test reuse it.
One rationale for adding logging was to use it if JFR is not present. By putting the tests together, it becomes impossible to compile and test logging without having JFR.
Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits. The output is displayed in units like "KiB" - not the normal when examining key lengths used in X509Certificates. i.e a 2048 bit key gets displayed as "2 KiB" - I'd prefer to keep the 2048 display version. We should not let the JMC GUI decide how units are specified. There will be other GUIs and this is the first event that uses bits, so I don’t think it is formatted that way because it was considered superior.
Erik
new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/
Regards, Sean.
On 28/06/18 17:59, Seán Coffey wrote:
Comments inline.
On 28/06/2018 17:20, Erik Gahlin wrote:
It's sufficient if an event object escapes to another method (regardless if JFR is enabled or not).
Some more feedback:
Rename event jdk.CertChain to jdk.CertificateChain Rename event jdk.X509Cert to jdk.X509Certificate Rename field certChain to certificateChain. Rename field serialNum to serialNumber all above done. Rename field algId to AlgorithmicId or AlgorithmicID maybe "algorithm" works here also ? Rename @Label("Ciphersuite") to @Label("Cipher Suite") Rename @Label("Cert Chain") to @Label("Certificate Chain") Rename @Label("Property Name") to "Name" or "Key" if that makes sense in the context? Rename @Label("Property Value") to "Value". Put events in a subcategory, i.e @Category({"Java Development Kit", "Security"}) done. Make classes final. done. I had thought that the JFR mechanism required non-final classes. What is the unit of the key in X509Certificate event? Bits? If that is the case, use @DataAmount(DataAmount.BITS) Yes - it's essentially the bit length of the keys used. Let me look into that annotation usage. @Label("Serial numbers forming chain of trust") should not be a sentence. How about "Serial Numbers"?
I think the tests are hard to read when two things are tested at the same time. It is also likely that if a test fail due to logging issues, it will be assigned to JFR because of the test name, even thought the issues is not JFR related. I think we're always going to have some ownership issues when tests serve a dual purpose. I still think it makes sense to keep the test logic in one place. Any failures in these tests will most likely be owned by security team. (moving the tests to security directory is also an option) If you want to reuse code between tests, I would put it in testlibrary. Let me check if there's any common patterns that could be added to the testlibary.
Thanks, Sean.
Erik
Thanks for the update Erik. By default I'm proposing that the new JFR Events and Logger be disabled. As a result the event class shouldn't escape. If performance metrics highlight an issue, we should revisit.
regards, Sean.
On 27/06/2018 20:57, Erik Gahlin wrote: > 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/ >>>>>>>
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.
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.
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.
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.
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.
Erik, comments inline.. On 08/11/18 15:12, Erik Gahlin wrote:
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. sounds fine. I've made those changes.
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. How about 'validationCounter' ?
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. Yes - originally, I was using the certificate serial number but that may not always be unique (esp. for test generated certs). I've changed the variable name to 'certificateId' and made it a long. Renamed the annotation also.
I could not find the file: test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java whoops - forgot to add it to mercurial tracking. there now : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v8/webrev/
regards, Sean.
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.
Confused. Aren't all Security properties security-related? This is not about normal system properties. And the method name in the latest webrev is "isSecurityProperty" without the "JDK" word. I assume this means you don't care about the difference between SE properties and JDK properties. --Max
On Nov 14, 2018, at 2:53 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
* 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.
On 11/13/18 8:11 PM, Weijun Wang wrote:
Confused. Aren't all Security properties security-related? This is not about normal system properties.
Although probably not that common, an application could create their own security properties, ex: Security.setProperty("security.myPassword", "abc123"); We want to avoid logging those. We just want to record changes to the JDK security properties.
And the method name in the latest webrev is "isSecurityProperty" without the "JDK" word. I assume this means you don't care about the difference between SE properties and JDK properties.
Hmm, I was reviewing v7, and the name was changed in v8. I think isJdkSecurityProperty method is a better name. --Sean
--Max
On Nov 14, 2018, at 2:53 AM, Sean Mullan <sean.mullan@oracle.com> wrote:
* 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.
Thanks for the comments Weijun. As per other review thread, I'm now recording all properties set via Security.setProperty(String, String). regards, Sean. On 14/11/2018 01:11, Weijun Wang wrote:
Confused. Aren't all Security properties security-related? This is not about normal system properties.
And the method name in the latest webrev is "isSecurityProperty" without the "JDK" word. I assume this means you don't care about the difference between SE properties and JDK properties.
--Max
On 11/13/18 1:53 PM, Sean Mullan wrote:
* 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.
Thinking more about this, this List has the potential to contain a lot of entries if there are many certificates and there is no way to control the size. It might be better to leverage the existing in-memory certificate cache in sun/security/provider/X509Factory.java: private static final Cache<Object, X509CertImpl> certCache = Cache.newSoftMemoryCache(750); Could you look into moving the code from X509CertImpl to X509Factory instead? In engineGenerateCertificate, you could do the commit only if it is added to the cache: try { byte[] encoding = readOneBlock(is); if (encoding != null) { X509CertImpl cert = getFromCache(certCache, encoding); if (cert != null) { return cert; } cert = new X509CertImpl(encoding); addToCache(certCache, cert.getEncodedInternal(), cert); commitEvent(); return cert; This way you could leverage the same cache. Duplicates could get recorded but only if the constraints on the cache are exceeded. This seems like a fair tradeoff. --Sean
Hi Sean, comments inline.. On 13/11/2018 18:53, Sean Mullan wrote:
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. after further discussion, we've decided to log all properties operated on via Security.setProperty(String, String). It think it makes sense. The contents of a JFR recording should always be treated as sensitive. Keep in mind also that these new JFR Events will be off by default.
* src/java.base/share/classes/sun/security/x509/X509CertImpl.java Good points here. I've taken on board your suggestion to move this code logic to the sun/security/provider/X509Factory.java class as per your later email suggestion.
45 l.addExpected("SunJSSE Test Serivce");
Is that a typo? "Serivce"
That's a typo in the test certificate details. We should fix it up via another bug record.
* 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. Yes - it's almost identical to SSLSocketTemplate except that SSLSocketTemplate doesn't belong to a package. I had a hard time incorporating its use into the jtreg tests via the @lib syntax. I think it's best if we open another JBS issue to migrate other uses of SSLSocketTemplate to jdk.test.lib.security.SSLSocketTemplate
I've cleaned up the various typo/syntax issues that you've highlighted also. http://cr.openjdk.java.net/~coffeys/webrev.8148188.v9/webrev/index.html regards, Sean.
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
--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.
I've renamed the 'peerCertificateId' variable and label in TLSHandshakeEvent to 'certificateId'. This allows the event data to be co-displayed in JMC views which share the same type of data (@CertificateId). I've uploaded an example screenshot [1] I've also made an adjustment to the TestModuleEvents test to disregard the jdk.proxy1 module for test purposes. http://cr.openjdk.java.net/~coffeys/webrev.8148188.v10/webrev/ [1] http://cr.openjdk.java.net/~coffeys/jmc_screenshots/shared_selection_certifi... Regards, Sean. On 14/11/18 21:09, Seán Coffey wrote:
Hi Sean,
comments inline..
On 13/11/2018 18:53, Sean Mullan wrote:
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. after further discussion, we've decided to log all properties operated on via Security.setProperty(String, String). It think it makes sense. The contents of a JFR recording should always be treated as sensitive. Keep in mind also that these new JFR Events will be off by default.
* src/java.base/share/classes/sun/security/x509/X509CertImpl.java Good points here. I've taken on board your suggestion to move this code logic to the sun/security/provider/X509Factory.java class as per your later email suggestion.
45 l.addExpected("SunJSSE Test Serivce");
Is that a typo? "Serivce"
That's a typo in the test certificate details. We should fix it up via another bug record.
* 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. Yes - it's almost identical to SSLSocketTemplate except that SSLSocketTemplate doesn't belong to a package. I had a hard time incorporating its use into the jtreg tests via the @lib syntax. I think it's best if we open another JBS issue to migrate other uses of SSLSocketTemplate to jdk.test.lib.security.SSLSocketTemplate
I've cleaned up the various typo/syntax issues that you've highlighted also. http://cr.openjdk.java.net/~coffeys/webrev.8148188.v9/webrev/index.html
regards, Sean.
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
--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.
Updated webrev looks good. --Sean On 11/16/18 10:31 AM, Seán Coffey wrote:
I've renamed the 'peerCertificateId' variable and label in TLSHandshakeEvent to 'certificateId'. This allows the event data to be co-displayed in JMC views which share the same type of data (@CertificateId). I've uploaded an example screenshot [1]
I've also made an adjustment to the TestModuleEvents test to disregard the jdk.proxy1 module for test purposes.
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v10/webrev/
[1] http://cr.openjdk.java.net/~coffeys/jmc_screenshots/shared_selection_certifi...
Regards, Sean.
On 14/11/18 21:09, Seán Coffey wrote:
Hi Sean,
comments inline..
On 13/11/2018 18:53, Sean Mullan wrote:
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. after further discussion, we've decided to log all properties operated on via Security.setProperty(String, String). It think it makes sense. The contents of a JFR recording should always be treated as sensitive. Keep in mind also that these new JFR Events will be off by default.
* src/java.base/share/classes/sun/security/x509/X509CertImpl.java Good points here. I've taken on board your suggestion to move this code logic to the sun/security/provider/X509Factory.java class as per your later email suggestion.
45 l.addExpected("SunJSSE Test Serivce");
Is that a typo? "Serivce"
That's a typo in the test certificate details. We should fix it up via another bug record.
* 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. Yes - it's almost identical to SSLSocketTemplate except that SSLSocketTemplate doesn't belong to a package. I had a hard time incorporating its use into the jtreg tests via the @lib syntax. I think it's best if we open another JBS issue to migrate other uses of SSLSocketTemplate to jdk.test.lib.security.SSLSocketTemplate
I've cleaned up the various typo/syntax issues that you've highlighted also. http://cr.openjdk.java.net/~coffeys/webrev.8148188.v9/webrev/index.html
regards, Sean.
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
--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. >
Finished.java ------------- In each handshake, Finished messages are delivered twice. One from client, and the other one from the server side. Catch Finished message and use the final one of them should be sufficient. In the Finished.java implementation, the signal of the final Finished message is the handshakeFinished field is set to true. Please move line 582: 582 recordEvent(chc.conContext.conSession); to line 558: 556 // handshake context cleanup. 557 chc.handshakeFinished = true; 558 Please move line 632: 632 recordEvent(shc.conContext.conSession); to line 608: 606 // handshake context cleanup. 607 shc.handshakeFinished = true; 608 Please remove line 838: 838 recordEvent(shc.conContext.conSession); Please remove line 984: 984 recordEvent(chc.conContext.conSession); No more comment. Thanks, Xuelei On 6/27/2018 11:57 AM, 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?
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.
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/
Thanks for reviewing Xuelei, I do acknowledge that the new TLS v1.3 code has greatly improved the logging output for related operations. I think the main drive with this enhancement is to use the new JFR API to capture interesting events. We can revisit the Logger requirements if there's a strong opinion from users. By default, both the new Logger and JFR output will be switched off. I've just made an edit to the JFR jfc files to disable these events. "I may suggest remove this method and use Key.getAlgorithm() directly." - Yes, great idea. Done. Thanks for feedback on Finished.java edits. I did question to myself whether fewer edits were sufficient given the client/server nature of the handshake. I've refreshed the webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v3/webrev/ some jfr side edits also : * Change label from "X.509 Certificate" to "X509 Certificate" - JFR test fails with "." usage * Move the instance field variable name in CertChainEvent to "certChain" - JFR tests discourage use of "ID" in "certIDs" regards, Sean. On 27/06/2018 21:11, Xuelei Fan wrote:
Finished.java ------------- In each handshake, Finished messages are delivered twice. One from client, and the other one from the server side. Catch Finished message and use the final one of them should be sufficient.
In the Finished.java implementation, the signal of the final Finished message is the handshakeFinished field is set to true.
Please move line 582: 582 recordEvent(chc.conContext.conSession); to line 558: 556 // handshake context cleanup. 557 chc.handshakeFinished = true; 558
Please move line 632: 632 recordEvent(shc.conContext.conSession); to line 608: 606 // handshake context cleanup. 607 shc.handshakeFinished = true; 608
Please remove line 838: 838 recordEvent(shc.conContext.conSession);
Please remove line 984: 984 recordEvent(chc.conContext.conSession);
No more comment.
Thanks, Xuelei
On 6/27/2018 11:57 AM, 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?
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.
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/
Looks fine to me. Thanks! Xuelei On 6/28/2018 5:28 AM, Seán Coffey wrote:
Thanks for reviewing Xuelei,
I do acknowledge that the new TLS v1.3 code has greatly improved the logging output for related operations. I think the main drive with this enhancement is to use the new JFR API to capture interesting events. We can revisit the Logger requirements if there's a strong opinion from users. By default, both the new Logger and JFR output will be switched off. I've just made an edit to the JFR jfc files to disable these events.
"I may suggest remove this method and use Key.getAlgorithm() directly." - Yes, great idea. Done.
Thanks for feedback on Finished.java edits. I did question to myself whether fewer edits were sufficient given the client/server nature of the handshake.
I've refreshed the webrev :
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v3/webrev/
some jfr side edits also :
* Change label from "X.509 Certificate" to "X509 Certificate" - JFR test fails with "." usage * Move the instance field variable name in CertChainEvent to "certChain" - JFR tests discourage use of "ID" in "certIDs"
regards, Sean.
On 27/06/2018 21:11, Xuelei Fan wrote:
Finished.java ------------- In each handshake, Finished messages are delivered twice. One from client, and the other one from the server side. Catch Finished message and use the final one of them should be sufficient.
In the Finished.java implementation, the signal of the final Finished message is the handshakeFinished field is set to true.
Please move line 582: 582 recordEvent(chc.conContext.conSession); to line 558: 556 // handshake context cleanup. 557 chc.handshakeFinished = true; 558
Please move line 632: 632 recordEvent(shc.conContext.conSession); to line 608: 606 // handshake context cleanup. 607 shc.handshakeFinished = true; 608
Please remove line 838: 838 recordEvent(shc.conContext.conSession);
Please remove line 984: 984 recordEvent(chc.conContext.conSession);
No more comment.
Thanks, Xuelei
On 6/27/2018 11:57 AM, 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?
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.
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/ >
participants (5)
-
Erik Gahlin
-
Sean Mullan
-
Seán Coffey
-
Weijun Wang
-
Xuelei Fan