Version cookie attribute added to the Set-Cookie header causes cookie parsing failures in a Reverse Proxy appliance.

[thomasdarimont]

Description

In a customer project, we had issues with a quarkus application (Keycloak) where a commercial reverse proxy appliance would partially remove the cookies set via the set-cookie header (all attributes except cookie name and value attribute were removed). After investigating the problem, we learned that the root cause of the
failure was the Version=1; cookie attribute, which caused the reverse proxy appliance to fail to parse the cookie completely.

I know that in this case, it is probably the fault of the reverse proxy appliance for not parsing cookie values in a more robust way,
but since the product claims to follow the RFC 6265 standard, I wondered whether the current quarkus implementation anticipates a too-old RFC 2965, and obsolete, set-cookie handling.

The current implementation of NewCookieHeaderDelegate seems to follow the Set-Cookie2 syntax defined by RFC 2965 HTTP State Management Mechanism from October 2000 but for the Set-Cookie header.

RFC 2965 HTTP State Management Mechanism defines the Set-Cookie2 syntax in

 3.2.2  Set-Cookie2 Syntax
 
  Version=value
      REQUIRED. The value of the Version attribute, a decimal integer,
      identifies the version of the state management specification to
      which the cookie conforms. For this specification, Version=1
      applies.

This RFC is Obsoleted by RFC 6265 HTTP State Management Mechanism (April 2011, RFC - Proposed Standard)

In RFC the syntax for the Set-Cookie header is defined in 4.1.1. Syntax

The "Version" attribute is no longer listed there.

I propose to make it possible to skip sending the Version attribute in the Set-Cookie header.
From my experience the Version attribute in set-cookie is not used in practice that much (if at all).

The current implementation in tomcat https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/util/http/Rfc6265CookieProcessor.java#L107 does not generate the Version attribute either.

Current Workaround

In the concrete customer project I had to work around this issue, by shadowing the org.keycloak.quarkus.runtime.integration.resteasy.QuarkusHttpResponse
and replace the code in org.keycloak.quarkus.runtime.integration.resteasy.QuarkusHttpResponse#setCookieIfAbsent
with the following:

    @Override
    public void setCookieIfAbsent(NewCookie newCookie) {
        if (newCookie == null) {
            throw new IllegalArgumentException("Cookie is null");
        }

        if (newCookies == null) {
            newCookies = new HashSet<>();
        }

        if (newCookies.add(newCookie)) {
            String headerValue = NEW_COOKIE_HEADER_DELEGATE.toString(newCookie);

            // PATCH:BEGIN
            // Remove Version=1; fragment from set-cookie response header line.
            headerValue = headerValue.replace("Version=1;","");
            // PATCH:END

            addHeader(HttpHeaders.SET_COOKIE, headerValue);
        }
    }

Implementation ideas

Perhaps it would be possible to check a quarkus configuration property in org.jboss.resteasy.reactive.common.headers.NewCookieHeaderDelegate to use old "RFC 2965" or new "RFC 6265" style cookies (the latter without the Version attribute).
Or have a specific property like ...cookies.add-version with the default true.

[gsmet]

Interesting and thanks for the research.

From what I can see RFC 2965 has been replaced by https://www.rfc-editor.org/rfc/rfc6265 which has no trace of the Version field.

I'm not sure if we are using this version field somewhere but MDN also doesn't reference anything about the version here: https://developer.mozilla.org/fr/docs/Web/HTTP/Headers/Set-Cookie .

So I think I would be in favor of dropping it entirely instead of having a configuration property.

But I would like to have feedback from @geoand @FroMage @sberyozkin @cescoffier and maybe @vietj from the Vert.x team who deals with low level HTTP stuff all the time, @jamezp from the RESTEasy team and @vmuzikar from the Keycloak team.

[quarkus-bot]

/cc @FroMage (rest), @geoand (rest), @stuartwdouglas (rest)

[geoand]

@FroMage is the expert at reading and interpreting specs :)

[FroMage]

I already noticed that we were using outdated specs for cookies. Even the javadoc for NewCookie doesn't mention anything besides the outdated RFC and version 1.

What I'm amazed at is that nobody reported this until now.

Honestly, if it's as simple as dropping the version field, let's go for it. I assume the TCK will fail, but it's outdated anyway.

[thomasdarimont]

What I'm amazed at is that nobody reported this until now.

If customers don't have to use "limited" reverse proxy appliances they might not hit the issue, or could simply work around it and the admins never told the developers how they did it :D

[jamezp]

Not really related, but something I had just happened to be working on so I'll share here too. The Jakarta Servlet 6.1 spec requires RFC 6265 now, jakartaee/servlet#37.

The Jakarta REST spec itself doesn't specify a specific RFC for the cookie, but it does indicate in the jakarta.ws.rs.core.NewCookie that it implements RFC-2109.

I don't think we can just remove the version, but I do think we could add a property which could control whether or not it gets added. I'll have to do some more research on whether we could fully support RFC 6265. I do think we should propose the REST spec should implement this though. I'll file an issue for this when I get some time.

[gsmet]

I have another idea: could we not include the version if it's the default i.e. 1?

This wouldn't change the behavior AFAIK and would solve the issue at hand?

[vmuzikar]

I have another idea: could we not include the version if it's the default i.e. 1?

If I understand it correctly, according to the RFC 2109, the version is required in any case – default or not. And it's for Set-Cookie, not Set-Cookie2. So if we're following RFC 2109, I'd say Quarkus is doing it correctly that it's including the version. But the question from my perspective is which of the RFCs it should follow.

[thomasdarimont]

I don't think that the Version=1; is that relevant to be honest.
Neither google chrome:

nor firefox:

devtools provide an option to see the cookie version attribute...

I also couldn't find a site that currently uses the Version=1; attribute in the set-cookie header.

So I think it's veeeeery rarely used. Most services seems to follow RFC 6265 as I can see.

[vmuzikar]

@thomasdarimont I agree but if I understood it correctly, the problem is the NewCookie Jakarta API is hinting the old RFC 2109 should be followed.

[thomasdarimont]

Thanks @vmuzikar for your input. Since the current cookie RFC 6265 does not mention the Version attribute, it is not required in the Set-Cookie header. In fact, it doesn't seem to be used in modern HTTP specifications (I checked a few big sites, like google, amazon, apple, bing).
Btw. even the quarkus website doesn't use the Version attribute with Set-Cookie: https://code.quarkus.io/ :)

IMHO the Version attribute was originally part of RFC 2109 for "cookie versioning," but it has since been deprecated and isn’t recognized or needed in current cookie handling standards (RFC 6265).

[thomasdarimont]

Just checked and the new / upcoming Jakarta RESTful Web Services 4.0 also refers to the outdated / deprecated cookie RFC2109 from 1997 instead of the newer RFC 6265.

[FroMage]

RFC 6265: April 2011

IMO, we should absolutely default to that RFC. Now, perhaps that will break some legacy code, but it would have to be legacy code that predates Quarkus' existence by almost a decade :-/

[gsmet]

My take on this is that we should drop the Version from Quarkus REST and see how it goes and then discuss with @jamezp to see what we do for RESTEasy and the spec. My guess is that they just never updated the spec.

[geoand]

That sounds very reasonable

[jamezp]

I need to make a proposal to update to the Jakarta REST spec about updating the spec. It's definitely out of date for sure.

I think for RESTEasy we need to figure out a way to support both the obsolete RFC 2109 and the new RFC 6265. My initial thought will be to use a configuration option to be able to turn on support for RFC 6265. This would be similar to what Undertow does.

That said, the NewCookie is somewhat limited in what it can expose about an RFC 6265 cookie. For example there's not really a way to expose things like Partitioned as we can't set arbitrary attributes on the NewCookie.

FYI I've filed https://issues.redhat.com/browse/RESTEASY-3556.