[#1145] Keep the IdP discovery cookie within RFC 6265

[vharseko]

Fixes #1145.

What the problem is

Tomcat's Rfc6265CookieProcessor refuses two things the IdP discovery configuration can produce for the preferred-IdP cookie (_saml_idp, _liberty_idp):

• a cookie domain with a leading dot, such as .example.com — An invalid domain [.example.com];
• a space in the value, which is the separator of an unencoded list of two or more IdPs (com.iplanet.am.cookie.encode=false) — An invalid character [32].

On the response.addCookie path (HttpOnly off, no SameSite: the standalone WAR's defaults) either one throws, and /saml2writer answers 500, as #1145 describes.

The list has a second half that #1145 does not cover. When the cookie goes out as the hand-built Set-Cookie header (HttpOnly on, or SameSite set), the browser stores _saml_idp=aWRw aWRwMg==, but the container drops that cookie from the Cookie header on the way back in: parseCookieHeader yields no _saml_idp, and req.getCookies() never shows it. So with encoding off the list is never read on Tomcat 10.1+, on either path. The reader finds no preferred IdP, and the writer overwrites the list with the latest IdP alone. The same servlets are mapped in the OpenAM server (openam-server-only web.xml), whose defaults are com.iplanet.am.cookie.encode=false and samesite=Lax. So an OpenAM server with default settings loses the list the same way, without a 500.

The change

• CookieUtils.newCookie sets .example.com as example.com. RFC 6265 §5.2.3 ignores the leading dot, so a browser treats both the same, and a cookie set earlier with the dot is overwritten rather than duplicated.
• With encoding off, newCookie writes the list separator as %20, and getCookieValueFromReq reads it back as a space. The IdPs are base64, whose + / = are cookie octets, so the separator is the only character that needs escaping. Base64 never contains %, so a single IdP already stored in a browser reads exactly as before. Full URL encoding is not an option here: URLEncDec.decode turns a base64 + into a space. Encoding on is unchanged.
• CookieWriterServlet catches an IllegalArgumentException from adding the cookie, logs it, and goes on to the RelayState. The servlet already works this way when the provider ID is missing: setting the preferred IdP happens behind the scenes. This covers any other domain the container refuses, e.g. example.com:8443.

Verification

openam-idpdiscovery gets tomcat-embed-core 10.1.48 in test scope. The new tests write the cookie with the real Rfc6265CookieProcessor and parse the Cookie header back with it. Surefire leaves the module's Servlet 5.0 API off the test classpath, because the processor calls Cookie.getAttribute from the 6.0 API that Tomcat carries.

Seven new tests. Against the base sources, four fail:

• aLeadingDotDomainIsAcceptedByTheContainer — An invalid domain [.example.com];
• anUnencodedListOfTwoIdpsIsAcceptedByTheContainer and anUnencodedListOfTwoIdpsIsReadBackFromTheContainer — An invalid character [32];
• CookieWriterServletTest.goesOnToTheRelayStateWhenTheContainerRefusesTheCookie — the exception escapes doGet.

A mutant that escapes the separator on write but does not unescape it on read also fails the read-back test: expected [aWRw aWRwMg==] but found [aWRw%20aWRwMg==]. aDomainWithoutALeadingDotIsSetAsItIs pins that only a leading dot is dropped: a mutant that strips the first character of any domain fails it with Domain=xample.com.

The other two tests are characterization tests. anUnencodedIdpAlreadyInTheBrowserIsReadAsItIs pins that a stored single IdP with + / = still reads unchanged. anEncodedListOfTwoIdpsIsReadBackFromTheContainer pins that the encoded mode still round-trips.

openam-idpdiscovery: 20 tests, 0 failures. openam-idpdiscovery-war builds. Not run end to end in a browser.

Compatibility

With com.iplanet.am.cookie.encode=false, a list of two or more IdPs written by this version is stored as aWRw%20aWRwMg==, and only this version reads the %20 back as a space. A reader from before it forwards the value as one token, and SAML2Utils.getPreferredIDP decodes it to nothing for two IdPs and to a wrong entity ID for three or more. Any cookie-legal separator has this property, and on Tomcat 10.1+ the previous reader could not read the space-separated list either.

With com.iplanet.am.cookie.encode=false, do not run IdP discovery nodes from before and after this change behind one common-domain host; after a downgrade, preferred-IdP lists written by this version are not read until the cookie expires.

A single IdP, and every value with encoding on, reads the same on both versions.

Relation to #1136

Rebased onto #1136, which sets Secure/HttpOnly in the same newCookie. Its comment in addCookieToResponse gave the leading-dot domain and the unencoded list as reasons to keep the hand-built header; newCookie now produces neither, so the comment names a domain the container still refuses, one with a port (example.com:8443). The #1136 test for the hand-built HttpOnly header uses that domain for the same reason.

[maximthomas]

praise: The fix sits where the container refuses the cookie, and the tests check it against the container itself rather than a mock.

• CookieUtilsTest.sentBackThroughTheContainer writes with the real Rfc6265CookieProcessor and parses the Cookie header back with it, so the read-back half of the bug is pinned too.
• % is outside base64, so escaping only the separator leaves a stored single IdP readable unchanged (anUnencodedIdpAlreadyInTheBrowserIsReadAsItIs).
• The new catch in CookieWriterServlet closes before if(isValidReturn), so a refused cookie still ends in the RelayState redirect or sendError.

---

suggestion (non-blocking): The leading-dot strip is pinned only by a dotted domain, so an unconditional strip survives.

openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java:382, openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java:98-105

aLeadingDotDomainIsAcceptedByTheContainer is the only test that gives newCookie a domain, and it passes .example.com; CookieWriterServletTest leaves the domain null. The mutant if ((domain != null) && domain.length() > 0) { domain = domain.substring(1); } produces the same example.com for that row and survives the module (14 run, 0 failures), while a configured example.com would go out as xample.com.

@Test
public void aDomainWithoutALeadingDotIsSetAsItIs() {
    Cookie cookie = CookieUtils.newCookie("_saml_idp", "aWRw", -1, "/", "example.com");

    String header = new Rfc6265CookieProcessor().generateHeader(cookie, null);

    assertTrue(header.contains("Domain=example.com"), header);
}

Pin: the mutant turns this row red with Domain=xample.com.

---

suggestion (non-blocking): Say in the description or release notes that an unencoded preferred-IdP list written by this version is misread by an IdP discovery node from before it.

openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java:367, :226

With com.iplanet.am.cookie.encode=false, newCookie stores the list as aWRw%20aWRwMg==, and only the new branch at :226 turns %20 back into a space. A node on the previous version — a rolling upgrade behind one common-domain host, or a downgrade while browsers keep the cookie — forwards the value raw, and SAML2Utils.getPreferredIDP sees a single token: two IdPs decode to nothing, and an odd list of three or more decodes to a wrong entity ID that the SP then tries to use. No code change is asked: any cookie-legal separator has this property, and on Tomcat 10.1+ the old reader could not read the spaced list either. The description argues compatibility only in the other direction.

With com.iplanet.am.cookie.encode=false, do not run IdP discovery nodes from before and after this change behind one common-domain host; after a downgrade, preferred-IdP lists written by this version are not read until the cookie expires.

[vharseko]

@maximthomas thanks, both taken. The branch is rebased onto the current master (#1136, #1144); the round is 20b03a0.

Leading-dot strip pinned only by a dotted domain — fixed. aDomainWithoutALeadingDotIsSetAsItIs is your test as written. With your mutant (strip the first character of any non-empty domain) it fails with _saml_idp=aWRw; Domain=xample.com; Path=/ expected [true] but found [false]; on the fix it passes.

Old readers and the %20 list — added as a Compatibility section in the description, with your note as the operator guidance. I checked the claim before writing it down: the old CookieReaderServlet passes the value through URLEncoder.encode, so the SP gets aWRw%20aWRwMg== as one token, and Base64.decode of that gives null for two IdPs and https://idp1.example.com<garbage>https://idp3.example.com for three.

Also in this round, from the rebase onto #1136: the addCookieToResponse comment no longer gives the leading-dot domain or the unencoded list as the reason for the hand-built header, since newCookie now produces neither. It names a domain the container still refuses, example.com:8443 (checked: An invalid domain [example.com:8443] on Tomcat 10.1.48). The #1136 HttpOnly header test uses that domain for the same reason.

openam-idpdiscovery: 20 tests, 0 failures; openam-idpdiscovery-war builds.

[maximthomas]

praise: Round 1's gaps are closed, and the rebase onto #1136 left no stale reasoning behind.

• aDomainWithoutALeadingDotIsSetAsItIs kills the strip-any-first-character mutant (Domain=xample.com), so both halves of the startsWith(".") guard in newCookie are now pinned.
• The #1136 comment in CookieUtils.addCookieToResponse and its HttpOnly header test now name example.com:8443, which the container still refuses. The old reasons, a leading dot and a space, are gone since newCookie no longer produces either.
• The Compatibility section says the %20 list format is one-way and tells operators not to mix versions behind one common-domain host.