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