Improve definition of x-correlator header

[StefanoFalsetto-CKHIOD]

Problem description
For security reasons it is advisable to know the format of any exchanged data. The x-correlator header is just defined as "string". There is no clear definition of how this string is composed, hence it is not possible to perform any check on this header.
That's a something the security teams are trying to avoid.

Possible evolution
My proposal is to define this header in a more detailed way. My first choice is to completely define a format for this header.
We can decide to use UUID, hence checks can be performed verifying the compliancy of the received string to UUID format

Alternative solution
Another solution could be let the liberty to the developer to choose whatever string it wants, maybe jut suggesting to use UUID. But in that case it is needed to at least define a standard maximum length and set of allowed characters.

I am interested in your feedback.

[eric-murray]

It is already recommended that x-corrleator should be a UUID. See API Design Guidelines.

It was originally mandated to be a UUID, but some participants wanted that requirement relaxed. See #168 and associated issue.

[StefanoFalsetto-CKHIOD]

Eric, thank you but the topic you linked is not answering to my issue. In #168 Someone asked to remove the UUID format check and some other people answered LGTM. No reasons, no discussion. Maybe you are referring to #166 ?

Anyway, for me It is not sufficient to ONLY recommend UUID, but as I said, we need to add at least the maximum length and allowed set of characters.

Is that something we can discuss about?

[patrice-conil]

Hi @StefanoFalsetto-CKHIOD, I agree with you that an open string format is not a good thing for security reason. Since the x-correlator is meant to correlate request and response, maybe we could consider usage of open telemetry for the next release.

[eric-murray]

Maybe you are referring to #166 ?

#166 is the "associated issue". All PRs should have an associated issue for discussion. And indeed #168 does.

[StefanoFalsetto-CKHIOD]

Thank you @patrice-conil for your nice answer.
Maybe the use of Open Telemetry can be something to recommend, but on the other hand I think it is possible find a more general approach to security.
In other words: do we want to also consider security aspects when writing API specs or not?
If the answer is yes, then we should at least talk about the basic checks that any security expert is doing on API: which is the data format the API is managing? What kind of checks is possible to perform in order to avoid any flaws in code and programming languages used to implement the API?
That's why I am asking to consider adding a maximum length for this string (e.g., to avoid the off-by-one overflow classic attack) and the set of allowed characters (e.g., to avoid code injection).

[bigludo7]

We had a discussion with @patrice-conil - We should probably split in 3 parts:

1. Focus on this issue Improve definition of x-correlator header #352 about x-correlator and propose to add a pattern like ^[a-zA-Z0-9-]{1,36}$ - This pattern allowing alphanumeric characters and the '-' with a 36 max length. I guess we can move fast on this one @eric-murray @rartych @StefanoFalsetto-CKHIOD @PedroDiez ?
2. Have a more generic approach for security - globally do we want to restrict any plain string attribute with a pattern? I imagine that we could have long discussion on this. We can have a dedicated issue/discussion?
3. Explore use of open telemetry in relation with x-correlator - This is something that we can do for the third release. We can have a dedicated issue/discussion?

[StefanoFalsetto-CKHIOD]

Thank you @bigludo7, here my feedback:

1. The approach and proposed regex are OK for me.
2. I agree with you, I think we will have a long discussion here, and we can dedicate another issue or discussion.
3. Ok for me

[Kevsy]

Also note the W3C Recommendation Trace Context , worth considering as a long-term replacement for x-correlator.

[eric-murray]

1. Focus on this issue Improve definition of x-correlator header #352 about x-correlator and propose to add a pattern like ^[a-zA-Z0-9-]{1,36}$ - This pattern allowing alphanumeric characters and the '-' with a 36 max length. I guess we can move fast on this one?

Yes, that pattern is fine with me. The only test client implementation I've seen that this change would beak was one that was obviously trying to generate UUIDs but failing badly!

[PedroDiez]

Focus on this issue #352 about x-correlator and propose to add a pattern like ^[a-zA-Z0-9-]{1,36}$ - This pattern allowing alphanumeric characters and the '-' with a 36 max length. I guess we can move fast on this one

Also fine with adding that pattern

[hdamker]

A pattern looks fine for me as well, but the length of 36 might be unnessary restrictive. The current version of traceparent within W3C Recommendation Trace Context has already a length of 55 and the document has the statement:

They should expect that headers may have larger size limits in the future and only disallow prohibitively large headers.

Maybe it make sense to take the definition for value in the key/value pairs in tracestate, which are tool vendor specific:

3.3.1.3.2 Value

The value is an opaque string containing up to 256 printable ASCII [RFC0020] characters (i.e., the range 0x20 to 0x7E) except comma (,) and (=). Note that this also excludes tabs, newlines, carriage returns, etc.

value = 0*255(chr) nblk-chr
nblk-chr = %x21-2B / %x2D-3C / %x3E-7E
chr = %x20 / nblk-chr

With this definition we should be on the safe side to not restrict any tracing tool and therefore don't need to declare a breaking change with the restriction of the x-correlator format.