RFR: 8148188: Enhance the security libraries to record events of interest
Seán Coffey
sean.coffey at oracle.com
Tue Jun 26 22:18:45 UTC 2018
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