Non-String values in Thread Context Map (MDC)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Non-String values in Thread Context Map (MDC)

Volkan Yazıcı
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.
Reply | Threaded
Open this post in threaded view
|

Re: Non-String values in Thread Context Map (MDC)

Ralph Goers
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ı <[hidden email]> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Non-String values in Thread Context Map (MDC)

Remko Popma-2
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 <[hidden email]> 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ı <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>
Reply | Threaded
Open this post in threaded view
|

Re: Non-String values in Thread Context Map (MDC)

Remko Popma-2
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 <[hidden email]> 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 <[hidden email]> 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ı <[hidden email]> 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: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
Reply | Threaded
Open this post in threaded view
|

Re: Non-String values in Thread Context Map (MDC)

Volkan Yazıcı
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
> 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ı <[hidden email]>
> 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: [hidden email]
> >> For additional commands, e-mail: [hidden email]
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: Non-String values in Thread Context Map (MDC)

Remko Popma-2
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ı <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>> 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ı <[hidden email]>
>> 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: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>
Reply | Threaded
Open this post in threaded view
|

Re: Non-String values in Thread Context Map (MDC)

Volkan Yazıcı
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 <[hidden email]> 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ı <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]> 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 <[hidden email]>
> >> 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ı <[hidden email]>
> >> 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: [hidden email]
> >>>> For additional commands, e-mail: [hidden email]
> >>>>
> >>
>
Reply | Threaded
Open this post in threaded view
|

Re: Non-String values in Thread Context Map (MDC)

Volkan Yazıcı
In reply to this post by Remko Popma-2
+1 happy Log4j 2 user:
https://github.com/vy/log4j2-logstash-layout/issues/46#issuecomment-549137987

On Sun, Nov 3, 2019 at 1:52 PM Remko Popma <[hidden email]> 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ı <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]> 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 <[hidden email]>
> >> 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ı <[hidden email]>
> >> 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: [hidden email]
> >>>> For additional commands, e-mail: [hidden email]
> >>>>
> >>
>