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

[Seán Coffey]

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