Remko, I see your concerns regarding simplicity and its implications on the security too, and I agree with them. In its current form, Log4j 2 allows custom ThreadContextMap's and ships a bunch by default as well. I think it is okay to advertise this *feature* in the official documentation with short recipes along with precautions due to its security related implications. This will avoid people hacking their own way out and help them to stick to recommended best practices, IMHO.
Yes, LogstashLayout handles non-String context map values. In a nutshell, all context map resolution requests are handled by ContextDataResolver#resolve() <https://github.com/vy/log4j2-logstash-layout/blob/master/layout/src/main/java/com/vlkan/log4j2/logstash/layout/resolver/ContextDataResolver.java#L31>, which delegates Object serialization to JsonGenerators.writeObject() <https://github.com/vy/log4j2-logstash-layout/blob/master/layout/src/main/java/com/vlkan/log4j2/logstash/layout/util/JsonGenerators.java#L13>. There Jackson does the rest of the JSON serialization magic. To the best of my knowledge [last famous words], serialization doesn't imply security vulnerabilities; many of the once every month popping up Jackson CVEs are connected with deserialization. (Note that LogstashLayout doesn't do any deserialization.) Volkan. On Sun, Nov 3, 2019 at 1:52 PM Remko Popma <[email protected]> wrote: > Volkan, > > Since LogstashLayout guarantees that the output is JSON instead of a > serialized Java object, I guess there’s not much risk in putting arbitrary > objects in the ThreadContext map and optionally sending these arbitrary > objects with the LogEvent. Does your layout know how to marshall and > unmarshall objects in the event’s context data (getContextData)? > > For the general Log4j project there’s a concern that people use the layout > that serializes the LogEvent. There are security risks with deserializing > any data that has potentially been modified. The mitigation of that risk > involves whitelisting and one concern was that arbitrary objects would make > this whitelisting harder. (I believe by now this is obsolete and even > whitelisting is known to have security problems, so perhaps the serializing > layout should be removed altogether but that’s maybe another topic.) > > Another issue is that as soon as arbitrary objects can be put into the > event’s context data, all layouts need to know how to Marshall/unmarshall > these objects. I don’t think we want to take on that burden. > > What it boils down to is that I don’t know how easy we want to make this. > > Remko. > > (Shameless plug) Every java main() method deserves http://picocli.info > > > On Nov 3, 2019, at 20:49, Volkan Yazıcı <[email protected]> wrote: > > > > Ralph, Remko, thanks so much for the prompt replies. I indeed got it > > working as recommended by you and/or detailed in the tickets: > > > > 1. Set log4j2.threadContextMap parameter to > > org.apache.logging.log4j.spi.CopyOnWriteSortedArrayThreadContextMap. > > 2. Add non-String values into MDC using the ObjectThreadContextMap > > interface: > > > > ObjectThreadContextMap threadContextMap = (ObjectThreadContextMap) > > ThreadContext.getThreadContextMap(); > > threadContextMap.putValue("key1", "stringValue"); > > threadContextMap.putValue("key2", 0xDEADB33F); > > > > I can add this as a FAQ entry to log4j2-logstash-layout README, that > said, > > I believe it is more convenient to have this in the official Log4j 2 > > documentation. What do you think? > > > >> On Sun, Nov 3, 2019 at 9:57 AM Remko Popma <[email protected]> > wrote: > >> > >> Short story longer: while there is no public API for putting non-String > >> values in the context map, the work done in that ticket and > >> https://issues.apache.org/jira/browse/LOG4J2-1660 provide the hooks for > >> installing a custom data structure for the context map. This data > structure > >> may support non-string values. I used these hooks to install a garbage > free > >> hybrid Object/primitive map in the trading system at work. Unfortunately > >> this data structure is not open source but it’s not rocket science > either; > >> it’s basically an extension of the array based string map in Log4j2 > with a > >> separate long[] array for the primitive values. You may need to provide > a > >> separate facade that the application can use instead of ThreadContext; > this > >> facade can provide methods like putLong(String, long) which are not > >> available in the Log4j ThreadContext. > >> > >> (Shameless plug) Every java main() method deserves http://picocli.info > >> > >>>> On Nov 3, 2019, at 17:31, Remko Popma <[email protected]> wrote: > >>> > >>> Volkan, > >>> > >>> See https://issues.apache.org/jira/browse/LOG4J2-1648 for more > details. > >>> > >>> Remko. > >>> > >>> (Shameless plug) Every java main() method deserves http://picocli.info > >>> > >>>>> On Nov 3, 2019, at 13:57, Ralph Goers <[email protected]> > >> wrote: > >>>>> > >>>> No. Our experience has shown that putting non-String values in > >> ThreadLocals has to be done very carefully, so to make sure there aren’t > >> any problems the ThreadContextMap only allows Strings. > >>>> > >>>> Ralph > >>>> > >>>>> On Nov 2, 2019, at 1:11 PM, Volkan Yazıcı <[email protected]> > >> wrote: > >>>>> > >>>>> Hello, > >>>>> > >>>>> LogEvent#getContextData() returns a ReadOnlyStringMap such that the > >>>>> provided get() and forEach() allow non-String values in the entries. > >> That > >>>>> said, ReadOnlyStringMap#toMap() returns a Map<String, String>. > Further, > >>>>> both ThreadContext.put() and ThreadContext.putAll() only allow String > >>>>> values. I am confused by this inconsistency. Is there a way to > provide > >> an > >>>>> MDC entry where the value is of a non-String type? > >>>>> > >>>>> Best. > >>>> > >>>> > >>>> > >>>> --------------------------------------------------------------------- > >>>> To unsubscribe, e-mail: [email protected] > >>>> For additional commands, e-mail: [email protected] > >>>> > >> >
