Skip to content

KNOX-3395: Add KNOXTOKEN issuance and JWTProvider federation integration tests - #1325

Open
Raghav-Mah3shwari wants to merge 2 commits into
apache:masterfrom
Raghav-Mah3shwari:KNOX-3395
Open

KNOX-3395: Add KNOXTOKEN issuance and JWTProvider federation integration tests#1325
Raghav-Mah3shwari wants to merge 2 commits into
apache:masterfrom
Raghav-Mah3shwari:KNOX-3395

Conversation

@Raghav-Mah3shwari

@Raghav-Mah3shwari Raghav-Mah3shwari commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR adds Docker Compose integration coverage for the knoxtoken topology (JWTProvider federation) and the KNOXTOKEN service. Both were baked into the CI test image but previously had no integration tests.

End-to-end flow:

Mint a Knox JWT from the KNOXTOKEN service (gateway/knoxldap/knoxtoken/api/v1/token) using Basic auth.
Present that bearer token to the JWTProvider-protected topology (gateway/knoxtoken/auth/api/v1/pre) and assert caller identity.
Exercise lifecycle operations (renew / revoke / enable / disable) and verify they are enforced at federation, not only acknowledged by the management API.
Issuance / federation:

test_token_endpoint_returns_jwt_and_metadata — access_token, token_type: Bearer, expires_in, well-formed 3-segment JWT
test_token_requires_authentication — anonymous request → 401
test_jwt_grants_access_to_federated_topology — valid JWT → x-knox-actor-username: guest
test_federated_topology_requires_token — no token → 401
test_federated_topology_rejects_malformed_token — structurally invalid bearer token → 401
test_federated_topology_rejects_wrong_signature — parseable JWT with a tampered signature segment → 401 (RS256 verification)
Lifecycle (server-managed token state):

test_revoke_is_enforced_at_federation — mint → federate OK → revoke → federate 401
test_renew_extends_and_token_still_federates — renewed: true and token still federates
test_renew_forbidden_for_non_whitelisted_user — non-whitelisted admin renew → 403
test_revoke_forbidden_for_non_owner_non_whitelisted_user — admin revoke of guest token → 403
test_disable_is_enforced_at_federation — disable → 401; enable → federation restored
test_enable_already_enabled_returns_400 / test_disable_already_disabled_returns_400
Topology / harness prerequisites
Lifecycle APIs require server-managed token state. This PR also:

How was this patch tested?

Verified API contracts against the codebase (TokenResource / TokenResourceV2, PreAuthResource, JWTProvider signature verification).
Ran the Docker Compose integration suite locally:
docker compose -f ./.github/workflows/compose/docker-compose.yml build knox-dev
docker compose -f ./.github/workflows/compose/docker-compose.yml up -d --force-recreate knox
docker compose -f ./.github/workflows/compose/docker-compose.yml run --rm tests
Confirmed the new file is auto-discovered by the default pytest run (not in the single-EKU --ignore list).

Integration Tests

This PR adds/extends integration tests under .github/workflows/tests. No new Java unit tests are included; KNOXTOKEN and JWTProvider already have module-level Java coverage. The black-box suite covers the packaged-gateway end-to-end path that was previously missing.

UI changes

N/A — no UI changes.

@Raghav-Mah3shwari

Copy link
Copy Markdown
Contributor Author

Hi @pzampino , @smolnar82, can i get a review on this? thanks

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

Test Results

53 tests   53 ✅  8s ⏱️
 3 suites   0 💤
 3 files     0 ❌

Results for commit 804d921.

♻️ This comment has been updated with latest results.

@smolnar82

Copy link
Copy Markdown
Contributor

Nice addition, @Raghav-Mah3shwari, the end-to-end path here (mint a token in knoxldap → present it to the JWTProvider-federated knoxtoken topology → assert x-knox-actor-username) is the highest-value scenario and it's covered cleanly. The file follows the suite conventions (common_utils, unittest, HTTPBasicAuth) and the issuance + negative auth cases all look correct.

A couple of things before merge:

1. Missing token lifecyle operation test cases
Would you be up for extending the scope to cover the token lifecycle operations (renew / revoke / enable / disable) in a follow-up (or here, if you prefer)? Since KNOX-3395 indicates KNOXTOKEN issuance, it's a natural next step and there's a config prerequisite worth capturing while it's fresh:

These endpoints require server-managed token state, which neither knoxtoken.xml nor knoxldap.xml currently enables (both set only token TTL). So it's a topology-config change first, tests second. Against the current config:

  • enable / disable / revoke → tokenStateService == null → 400 CONFIGURATION_ERROR
  • renew → falls into the renewalDisabled branch and echoes the token's own expiry — no actual renewal

To exercise them you'd need, on the KNOXTOKEN service:

<param><name>knox.token.exp.server-managed</name><value>true</value></param>
<param><name>knox.token.renewer.whitelist</name><value>guest</value></param>

Without the whitelist, token renewal or revocation will return 403 (revoking your own token is the one exception).

Cases I'd suggest for that round:

  • Revoke → enforcement (the key one): mint → revoke → re-present to the federated endpoint and assert it's now 401. The {"revoked":"true"} response alone doesn't prove revocation is enforced.
  • Renew: returns {"renewed":"true", ...} and the renewed token still federates.
  • Authorization negative: a non-whitelisted user gets 403 on renew/revoke.
  • enable/disable: disable a token, confirm it stops working; cover the "already enabled/disabled" 400 paths.

2. Two minor notes on the existing tests:

  • test_federated_topology_rejects_invalid_token uses "not.a.valid.jwt" (structurally malformed). Consider also a well-formed-but-wrong-signature token so the RS256 signature check (jwt.expected.sigalg) is exercised, not just the parser. This is doable by removing the last character of the generated token's access_key field.
  • The topologies set knoxsso.token.ttl, but the documented KNOXTOKEN TTL param is knox.token.ttl if I were you, I'd change them in the scope of this PR.

@Raghav-Mah3shwari

Copy link
Copy Markdown
Contributor Author

Hi @smolnar82 sure i will add them as well, thank you

@Raghav-Mah3shwari

Copy link
Copy Markdown
Contributor Author

Hi @smolnar82 , can you review the pr again?, thanks

Comment on lines +37 to +39
# 3) Provision the gateway-level JWK required for server-managed Knox token state
# (renew / revoke / enable / disable and JWTProvider enforcement).
/knox-runtime/bin/knoxcli.sh generate-jwk --jwkAlg HS256 --saveAlias knox.token.hash.key

@smolnar82 smolnar82 Aug 10, 2026

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.

The comment change here is misleading. You removed the original

Trust that certificate in the JVM default truststore (cacerts) so the JNDI-based
Shiro LDAP realm accepts it. This is additive - it does not remove the default CAs.

before the keytool command.

I thinks it should look like this:

# 3) Provision the gateway-level JWK required for server-managed Knox token state (renew / revoke / enable / disable and JWTProvider enforcement).
/knox-runtime/bin/knoxcli.sh generate-jwk --jwkAlg HS256 --saveAlias knox.token.hash.key

# 4) Trust that certificate in the JVM default truststore (cacerts) so the JNDI-based
Shiro LDAP realm accepts it. This is additive - it does not remove the default CAs.
keytool -exportcert -alias ldaps -rfc
-keystore "$KEYSTORE" -storepass "$KEYSTORE_PASSWORD" -file /tmp/ldaps-cert.pem

expected_username,
)

def assert_federation_rejected(self, access_token):

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.

Every other helper in the class uses a _ prefix (_issue_token, _federate, _assert_federates). assert_federation_rejected is the only exception; it'll also show up in test discovery frameworks as a test method (they scan for names starting with assert). Please rename to _assert_federation_rejected.

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