RFR: 8148188: Enhance the security libraries to record events of interest

Xuelei Fan xuelei.fan at oracle.com
Wed Jun 27 18:57:37 UTC 2018


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/
>>>>
>>>
>>
> 


More information about the security-dev mailing list