RFR: 8148188: Enhance the security libraries to record events of interest
Seán Coffey
sean.coffey at oracle.com
Wed Jun 27 19:14:04 UTC 2018
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/
>>>>>
>>>>
>>>
>>
More information about the security-dev
mailing list