Skip to content

KNOX-3519 - Set the JWKS host on the TLS socket so HTTPS endpoint identification works under BC-FIPS - #1459

Open
moresandeep wants to merge 1 commit into
apache:masterfrom
moresandeep:KNOX-3519
Open

moresandeep wants to merge 1 commit into
apache:masterfrom
moresandeep:KNOX-3519

Conversation

@moresandeep

Copy link
Copy Markdown
Contributor

KNOX-3519 - Set the JWKS host on the TLS socket so HTTPS endpoint identification works under BC-FIPS

What changes were proposed in this pull request?

Under the BouncyCastle FIPS JSSE provider, fetching a JWKS over HTTPS fails before any
certificate is examined:

java.security.cert.CertificateException: No hostname specified for HTTPS endpoint ID check
    at org.bouncycastle.jsse.provider.HostnameUtil.checkHostname(HostnameUtil.java:31)
    at org.bouncycastle.jsse.provider.ProvX509TrustManager.checkEndpointID(ProvX509TrustManager.java:333)

JWT verification then fails and the request is rejected with 401, even when the JWKS
endpoint's certificate is fully trusted and carries a matching SAN.

The change. A new SetHostSSLSocketFactory decorator stamps the JWKS URL's host onto
every socket it creates, via setHost(String) as declared by
org.bouncycastle.jsse.BCSSLSocket. It is wired in at both JWKS retrieval paths:
DefaultTokenAuthorityService.buildJwkSource (default truststore) and
IssuerTrustResourceRetriever (per-issuer CA bundle).

Notes on the implementation:

  • The BC type is reached reflectively, so Knox takes no compile dependency on the BouncyCastle
    TLS provider, it is present only in FIPS deployments. Lookup walks interfaces first,
    because BC's socket implementations are package-private and the method is only invocable
    through the public BCSSLSocket interface.
  • On sockets that do not expose setHost the decorator is a no-op. That is correct for the
    stock JSSE, which identifies HTTPS endpoints from the URL already, so non-FIPS deployments
    are unaffected.
  • Endpoint identification is preserved, not bypassed. The server certificate must still
    carry a SAN matching the JWKS host.
  • The retriever built for the default path reproduces Nimbus's own defaults exactly
    (connect/read 500 ms, 51200 byte limit, disconnectAfterUse = true), so timeout and size
    behaviour are unchanged.

How was this patch tested?

Patch was tested locally.

@github-actions

Copy link
Copy Markdown

Test Results

  4 files    4 suites   1m 29s ⏱️
145 tests 145 ✅ 0 💤 0 ❌
169 runs  169 ✅ 0 💤 0 ❌

Results for commit bef5a3e.

@lmccay lmccay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some comments and asked questions.

* deployments. On sockets that do not expose the method this decorator is a no-op, which is
* correct for the stock JSSE: it identifies IP endpoints from the URL already.
*/
class SetHostSSLSocketFactory extends SSLSocketFactory {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit - I don't like that this classname reads like a verb rather than a noun.
Not a blocker though. Maybe HostSettingSSLSocketFactory is a better noun based name?

/* Not reachable through this type; a public interface may still expose it. */
continue;
} catch (InvocationTargetException | RuntimeException e) {
return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a single failure to set it even though it appears to have such a method fail the entire iteration over the types? Should we not continue to walk the hierarchy to see if one will work? Are we assuming that someone overrode another implementation in order to purposely not support it and throw a RTE rather than just no-op it?

* @return {@code true} when the host was applied, {@code false} when the socket does not
* support it or the call failed
*/
static boolean setHost(Socket socket, String host) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we are iterating over the class hierarchy of the socket looking for the first one that accepts or rejects a setHost method call. IIUC, then this looks for the most specific implementation of setHost that is public. If it fails due to method visibility it continues to walk the hierarchy to try and find a public one that will work. If that's accurate then I think we should document this a bit here to describe the intent at least.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants