From f270ae3079b6e3a221a9fe608690029523c3f602 Mon Sep 17 00:00:00 2001 From: Joseph Heenan Date: Tue, 8 Jun 2021 21:55:38 +0100 Subject: [PATCH 1/3] FAPI-CIBA: Fix description of wrong-auth-req-id test As pointed out by Ralph this had a typo, but it was also more complicated and less precise that it could be, so rewrote it. closes #898 --- ...nsureWrongAuthenticationRequestIdInTokenEndpointRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1EnsureWrongAuthenticationRequestIdInTokenEndpointRequest.java b/src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1EnsureWrongAuthenticationRequestIdInTokenEndpointRequest.java index 7e76bce89f..b3deff47b8 100644 --- a/src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1EnsureWrongAuthenticationRequestIdInTokenEndpointRequest.java +++ b/src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1EnsureWrongAuthenticationRequestIdInTokenEndpointRequest.java @@ -14,7 +14,7 @@ @PublishTestModule( testName = "fapi-ciba-id1-ensure-wrong-auth-req-id-in-token-endpoint-request", displayName = "FAPI-CIBA-ID1: Ensure wrong auth_req_id in token endpoint request", - summary = "This test passes the clinent_2's information (e.g client_id, client_jwks, client_mutual_tls_authentication) and the client_1's auth_req_id in the token endpoint parameters to the one inside the request. The token endpoint server returned an error message that the grant permission is invalid.", + summary = "This test uses an auth_req_id issued to client 1 at the token endpoint, but authenticates as client 2. The token endpoint server must return an 'invalid_grant' error.", profile = "FAPI-CIBA-ID1", configurationFields = { "server.discoveryUrl", From 045d71b6b7a959410de4ac9cc4fdfd1c022c7f3f Mon Sep 17 00:00:00 2001 From: Joseph Heenan Date: Tue, 8 Jun 2021 23:37:42 +0100 Subject: [PATCH 2/3] FAPI-CIBA: Use mtls backchannel_authentication_endpoint If (and only if) using MTLS client authentication, we need to use the mtls aliases endpoint. We solve this using the mechanism used for the token endpoint, but it's a little messy as for the backchannel authentication endpoint we should only use the mtls alias when using mtls client authentication, whereas we always need to use the mtls token endpoint (as FAPI requires we present a TLS cert to the token endpoint for the token to be bound to). closes #897 --- .../client/AddMTLSEndpointAliasesToEnvironment.java | 7 +++++++ .../client/CallBackchannelAuthenticationEndpoint.java | 3 ++- .../openid/conformance/fapiciba/AbstractFAPICIBAID1.java | 6 ++++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/main/java/net/openid/conformance/condition/client/AddMTLSEndpointAliasesToEnvironment.java b/src/main/java/net/openid/conformance/condition/client/AddMTLSEndpointAliasesToEnvironment.java index 141808d207..99c6f3715a 100644 --- a/src/main/java/net/openid/conformance/condition/client/AddMTLSEndpointAliasesToEnvironment.java +++ b/src/main/java/net/openid/conformance/condition/client/AddMTLSEndpointAliasesToEnvironment.java @@ -7,6 +7,13 @@ import net.openid.conformance.testmodule.Environment; import net.openid.conformance.testmodule.OIDFJSON; +/** + * This copies any server endpoints into the root of the environment, overriding it with any entry found in + * mtls_endpoint_aliases. + * + * It is assumed that, when a test needs to use mtls, it will use the value in the root of the + * environment. + */ public class AddMTLSEndpointAliasesToEnvironment extends AbstractCondition { @Override diff --git a/src/main/java/net/openid/conformance/condition/client/CallBackchannelAuthenticationEndpoint.java b/src/main/java/net/openid/conformance/condition/client/CallBackchannelAuthenticationEndpoint.java index 61f2ccec01..2d5fc56f58 100644 --- a/src/main/java/net/openid/conformance/condition/client/CallBackchannelAuthenticationEndpoint.java +++ b/src/main/java/net/openid/conformance/condition/client/CallBackchannelAuthenticationEndpoint.java @@ -42,7 +42,8 @@ public class CallBackchannelAuthenticationEndpoint extends AbstractCondition { @PostEnvironment(required = "backchannel_authentication_endpoint_response") public Environment evaluate(Environment env) { - final String bcAuthEndpoint = env.getString("server", "backchannel_authentication_endpoint"); + final String endpointKey = "backchannel_authentication_endpoint"; + final String bcAuthEndpoint = env.getString(endpointKey) != null ? env.getString(endpointKey) : env.getString("server", endpointKey); if (bcAuthEndpoint == null) { throw error("Couldn't find backchannel authentication endpoint"); } diff --git a/src/main/java/net/openid/conformance/fapiciba/AbstractFAPICIBAID1.java b/src/main/java/net/openid/conformance/fapiciba/AbstractFAPICIBAID1.java index 3c4a67a149..5b98c63db7 100644 --- a/src/main/java/net/openid/conformance/fapiciba/AbstractFAPICIBAID1.java +++ b/src/main/java/net/openid/conformance/fapiciba/AbstractFAPICIBAID1.java @@ -309,6 +309,12 @@ public void configure(JsonObject config, String baseUrl, String externalUrlOverr if (supportMTLSEndpointAliases != null) { call(sequence(supportMTLSEndpointAliases)); + if (getVariant(ClientAuthType.class) != ClientAuthType.MTLS) { + // we only need to call the mtls aliased backchannel authentication endpoint when using mtls client auth + // (but need to use the mtls alias for the token endpoint whenever we're using certificate bound + // access tokens) + env.removeNativeValue("backchannel_authentication_endpoint"); + } } // make sure the server configuration passes some basic sanity checks From 5802943ce5f47e7cf400f0bb2f50e6b1c3f6de2f Mon Sep 17 00:00:00 2001 From: Joseph Heenan Date: Tue, 8 Jun 2021 23:46:12 +0100 Subject: [PATCH 3/3] FAPI-CIBA: Allow ping not to be sent on auth_req_id expiry As per WG discussion, ping is entirely optional in this case: https://bitbucket.org/openid/mobile/issues/202/error-in-fapi-ciba-certification-tests We now sleep till expiry, and for another 5 seconds (as if the server does send a ping we don't want to receive it after the test has finished), then call the token endpoint etc as before. closes #894 --- .../condition/client/WaitFor5Seconds.java | 12 ++++++++++++ .../fapiciba/FAPICIBAID1AuthReqIdExpired.java | 11 +++++++---- .../conformance/testmodule/AbstractTestModule.java | 8 +++++++- 3 files changed, 26 insertions(+), 5 deletions(-) create mode 100644 src/main/java/net/openid/conformance/condition/client/WaitFor5Seconds.java diff --git a/src/main/java/net/openid/conformance/condition/client/WaitFor5Seconds.java b/src/main/java/net/openid/conformance/condition/client/WaitFor5Seconds.java new file mode 100644 index 0000000000..3955bafe07 --- /dev/null +++ b/src/main/java/net/openid/conformance/condition/client/WaitFor5Seconds.java @@ -0,0 +1,12 @@ +package net.openid.conformance.condition.client; + +import net.openid.conformance.condition.common.AbstractWaitForSpecifiedSeconds; +import net.openid.conformance.testmodule.Environment; + +public class WaitFor5Seconds extends AbstractWaitForSpecifiedSeconds { + + @Override + protected long getExpectedWaitSeconds(Environment env) { + return 5; + } +} diff --git a/src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1AuthReqIdExpired.java b/src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1AuthReqIdExpired.java index 90082c34d5..6fa9ff69aa 100644 --- a/src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1AuthReqIdExpired.java +++ b/src/main/java/net/openid/conformance/fapiciba/FAPICIBAID1AuthReqIdExpired.java @@ -5,6 +5,7 @@ import net.openid.conformance.condition.client.CheckTokenEndpointHttpStatusNot200; import net.openid.conformance.condition.client.SleepUntilAuthReqExpires; import net.openid.conformance.condition.client.TellUserToIgnoreCIBAAuthentication; +import net.openid.conformance.condition.client.WaitFor5Seconds; import net.openid.conformance.testmodule.PublishTestModule; import net.openid.conformance.variant.CIBAMode; @@ -50,11 +51,12 @@ protected void waitForAuthenticationToComplete(long delaySeconds) { callAndStopOnFailure(TellUserToIgnoreCIBAAuthentication.class); setStatus(Status.WAITING); + callAndStopOnFailure(SleepUntilAuthReqExpires.class); if (testType == CIBAMode.PING) { - // test resumes when notification endpoint called - return; + // a ping notification may or may not be issued; allow an extra 5 seconds to make sure any ping arrives + // before we continue + callAndStopOnFailure(WaitFor5Seconds.class); } - callAndStopOnFailure(SleepUntilAuthReqExpires.class); setStatus(Status.RUNNING); callTokenEndpointAndFinishTest(); @@ -64,7 +66,8 @@ protected void waitForAuthenticationToComplete(long delaySeconds) { protected void processNotificationCallback(JsonObject requestParts) { if (testType == CIBAMode.PING) { verifyNotificationCallback(requestParts); - callTokenEndpointAndFinishTest(); + setStatus(Status.WAITING); + // test continues when the above sleep/wait completes } else { super.processNotificationCallback(requestParts); } diff --git a/src/main/java/net/openid/conformance/testmodule/AbstractTestModule.java b/src/main/java/net/openid/conformance/testmodule/AbstractTestModule.java index 075a8e439b..055bdf0a74 100644 --- a/src/main/java/net/openid/conformance/testmodule/AbstractTestModule.java +++ b/src/main/java/net/openid/conformance/testmodule/AbstractTestModule.java @@ -8,6 +8,7 @@ import net.openid.conformance.condition.Condition; import net.openid.conformance.condition.ConditionError; import net.openid.conformance.condition.client.SleepUntilAuthReqExpires; +import net.openid.conformance.condition.client.WaitFor5Seconds; import net.openid.conformance.frontChannel.BrowserControl; import net.openid.conformance.info.ImageService; import net.openid.conformance.info.TestInfoService; @@ -248,7 +249,12 @@ protected void call(ConditionCallBuilder builder) { // We skip these checks for a condition that we deliberately call without the lock held so as not to block // other threads; I suspect it means that this condition should have the ability to call setStatus or that // their functionality should be in a method in AbstractTestModule instead - if (builder.getConditionClass() != SleepUntilAuthReqExpires.class) { + // Not all the 'WaitFor' functions are listed here; that means some of them we are calling with the lock held + // which may be problematic (e.g. it prevents any incoming connections being process and I suspect may prevent + // the test being aborted until the sleep expires). It would probably be preferable to always release the + // lock whilst sleeping (which is probably best achieve by one of the ways outlined in the previous paragraph.) + if (builder.getConditionClass() != SleepUntilAuthReqExpires.class && + builder.getConditionClass() != WaitFor5Seconds.class) { if (getStatus() != Status.CREATED) { // We don't run this check for 'CREATED' as the lock is currently not held during 'configure'; see // https://gitlab.com/openid/conformance-suite/issues/688