From 060b52201261d5bca102c3f5c02c87d68c6e6cfb Mon Sep 17 00:00:00 2001 From: Guus der Kinderen Date: Thu, 20 Jul 2023 13:24:11 +0200 Subject: [PATCH] OF-2611: S2S Unit test: clean up TODOs, add spec references This adds references to RFCs in the ExpectedOutcome calculation, and removes some of the TODO statements in that class. Most of the TODOs are 'resolved' by adding a 'strictCertificateValidation' setting in the calculation. This is used to choose between the multiple possible outcomes that were in the 'TODO'. Note taht the strictCertificateValidation setting is hardcoded in all tests. The current implementation makes it hard to configure different values for the initiating and receiving entities. Also, they would add to an already long list of tests. --- .../openfire/session/ExpectedOutcome.java | 89 +++++++++++-------- .../LocalIncomingServerSessionTest.java | 2 +- .../LocalOutgoingServerSessionTest.java | 2 +- .../openfire/session/ServerSettings.java | 25 ++++-- 4 files changed, 70 insertions(+), 48 deletions(-) diff --git a/xmppserver/src/test/java/org/jivesoftware/openfire/session/ExpectedOutcome.java b/xmppserver/src/test/java/org/jivesoftware/openfire/session/ExpectedOutcome.java index 6595c7bb37..986254be95 100644 --- a/xmppserver/src/test/java/org/jivesoftware/openfire/session/ExpectedOutcome.java +++ b/xmppserver/src/test/java/org/jivesoftware/openfire/session/ExpectedOutcome.java @@ -27,6 +27,9 @@ * @see XEP-0220: Server Dialback * @see XEP-0178: Best Practices for Use of SASL EXTERNAL with Certificates + * @see Use of Transport Layer Security (TLS) in the Extensible Messaging and Presence Protocol (XMPP) + * @see OF-2611: Improve automated tests for S2S functionality + * @see OF-2591: S2S Outbound should allow encryption if Dialback used for authentication */ public class ExpectedOutcome { @@ -142,9 +145,13 @@ public static ExpectedOutcome generateExpectedOutcome(final ServerSettings initi } break; case INVALID: - // TODO Is this possibly an allowable OF-2591 edge-case? Worry about DOWNGRADE ATTACK VECTOR? - // possibly: expectedOutcome.set(NON_ENCRYPTED_WITH_DIALBACK), // Encryption is configured to be OPTIONAL, so maybe allowable? - expectedOutcome.set(NO_CONNECTION, "the Initiating Entity will fail to negotiate TLS, as the Receiving Entity's certificate is not valid."); + if (initiatingServer.strictCertificateValidation) { + expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate, which should cause Initiating Entity to abort TLS (as per RFC 6120 section 13.7.2)."); + } else if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { + expectedOutcome.set(ENCRYPTED_WITH_DIALBACK_AUTH, "Initiating Entity may choose to ignore Receiving Entities invalid certificate (for encryption purposes only) and choose to authenticate with Server Dialback (per RFC 7590 Section 3.4)"); + } else { + expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate. As Server Dialback is not available, authentication cannot occur."); + } break; case VALID: switch (initiatingServer.certificateState) { @@ -156,13 +163,12 @@ public static ExpectedOutcome generateExpectedOutcome(final ServerSettings initi } break; case INVALID: - if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { - // TODO: should the Initiating Entity be allowed to authenticate using Dialback over an encrypted TLS connection - or should TLS fail hard when invalid certs are used (the client _could_ opt to not send those...)? Is this possibly an allowable OF-2591 edge-case, or is this a DOWNGRADE ATTACK vector? - expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS."); - // possibly: expectedOutcome.set(NON_ENCRYPTED_WITH_DIALBACK), // fail TLS but allow unencrypted. Perhaps better to fail connecting, to not give false sense of encryption security? DOWNGRADE ATTACK VECTOR? Encryption is configured to be OPTIONAL, so maybe allowable? - // possibly: expectedOutcome.set(ENCRYPTED_WITH_DIALBACK), // do not fail TLS with invalid client cert, as it's usable for encryption even if it's not used for authentication. DOWNGRADE ATTACK VECTOR? + if (receivingServer.strictCertificateValidation) { + expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS (as per RFC 6120 section 13.7.2)."); + } else if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { + expectedOutcome.set(ENCRYPTED_WITH_DIALBACK_AUTH, "Receiving Entity may choose to ignore Initiating Entities invalid certificate (for encryption purposes only) and choose to authenticate with Server Dialback (per RFC 7590 Section 3.4)"); } else { - expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS. Even if Receiving Entity would negotiate TLS for encryption, it can't use Initiating Entity's invalid cert for Authentication. As Dialback is also not available, authentication cannot occur."); + expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate. As Server Dialback is not available, authentication cannot occur."); } break; case VALID: @@ -180,11 +186,15 @@ public static ExpectedOutcome generateExpectedOutcome(final ServerSettings initi // TODO: should we take into account a manual configuration of an ANON cypher suite, so that encryption-without-authentication can occur via TLS, followed by a Dialback-based authentication? break; case INVALID: - expectedOutcome.set(NO_CONNECTION, "Receiving Entity requires encryption, but it does provides an invalid TLS certificate. The Initiating Entity cannot negotiate TLS and therefor the required encrypted connection cannot be established."); - // TODO: should we allow TLS to be used anyway, so that encryption-without-authentication can occur via TLS, followed by a Dialback-based authentication? THIS INTRODUCES DOWNGRADE ATTACK VECTOR. + if (initiatingServer.strictCertificateValidation) { + expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate, which should cause Initiating Entity to abort TLS (as per RFC 6120 section 13.7.2)."); + } else if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { + expectedOutcome.set(ENCRYPTED_WITH_DIALBACK_AUTH, "Initiating Entity may choose to ignore Receiving Entities invalid certificate (for encryption purposes only) and choose to authenticate with Server Dialback (per RFC 7590 Section 3.4)"); + } else { + expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate. As Server Dialback is not available, authentication cannot occur."); + } break; case VALID: - // TODO - AG We need to check the initiating servers certificate switch (initiatingServer.certificateState) { case MISSING: if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { @@ -194,13 +204,12 @@ public static ExpectedOutcome generateExpectedOutcome(final ServerSettings initi } break; case INVALID: - if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { - // TODO: should the Initiating Entity be allowed to authenticate using Dialback over an encrypted TLS connection - or should TLS fail hard when invalid certs are used (the client _could_ opt to not send those...)? Is this possibly an allowable OF-2591 edge-case, or is this a DOWNGRADE ATTACK vector? - expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS."); - // possibly: expectedOutcome.set(NON_ENCRYPTED_WITH_DIALBACK), // fail TLS but allow unencrypted. Perhaps better to fail connecting, to not give false sense of encryption security? DOWNGRADE ATTACK VECTOR? Encryption is configured to be OPTIONAL, so maybe allowable? - // possibly: expectedOutcome.set(ENCRYPTED_WITH_DIALBACK), // do not fail TLS with invalid client cert, as it's usable for encryption even if it's not used for authentication. DOWNGRADE ATTACK VECTOR? + if (receivingServer.strictCertificateValidation) { + expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS (as per RFC 6120 section 13.7.2)."); + } else if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { + expectedOutcome.set(ENCRYPTED_WITH_DIALBACK_AUTH, "Receiving Entity may choose to ignore Initiating Entities invalid certificate (for encryption purposes only) and choose to authenticate with Server Dialback (per RFC 7590 Section 3.4)"); } else { - expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS. Even if Receiving Entity would negotiate TLS for encryption, it can't use Initiating Entity's invalid cert for Authentication. As Dialback is also not available, authentication cannot occur."); + expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate. As Server Dialback is not available, authentication cannot occur."); } break; case VALID: @@ -226,8 +235,13 @@ public static ExpectedOutcome generateExpectedOutcome(final ServerSettings initi // TODO: should we take into account a manual configuration of an ANON cypher suite, so that encryption-without-authentication can occur via TLS, followed by a Dialback-based authentication? break; case INVALID: - expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate. The Initiating Entity cannot negotiate TLS and therefor the required encrypted connection cannot be established."); - // TODO: should we allow TLS to be used anyway, so that encryption-without-authentication can occur via TLS, followed by a Dialback-based authentication? THIS INTRODUCES DOWNGRADE ATTACK VECTOR. + if (initiatingServer.strictCertificateValidation) { + expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate, which should cause Initiating Entity to abort TLS (as per RFC 6120 section 13.7.2)."); + } else if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { + expectedOutcome.set(ENCRYPTED_WITH_DIALBACK_AUTH, "Initiating Entity may choose to ignore Receiving Entities invalid certificate (for encryption purposes only) and choose to authenticate with Server Dialback (per RFC 7590 Section 3.4)"); + } else { + expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate. As Server Dialback is not available, authentication cannot occur."); + } break; case VALID: switch (initiatingServer.certificateState) { @@ -239,14 +253,12 @@ public static ExpectedOutcome generateExpectedOutcome(final ServerSettings initi } break; case INVALID: - if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { - // TODO: should the Initiating Entity be allowed to authenticate using Dialback over an encrypted TLS connection - or should TLS fail hard when invalid certs are used (the client _could_ opt to not send those...)? Is this possibly an allowable OF-2591 edge-case, or is this a DOWNGRADE ATTACK vector? - expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS."); - // AG: this is correct I think. The language in 13.7.2 states in block capitals if a certificate is present it MUST attempt validation; if the validation fails, the connection terminates. See RFC2119 to confirm this is an absolute requirement. - // possibly: expectedOutcome.set(NON_ENCRYPTED_WITH_DIALBACK), // fail TLS but allow unencrypted. Perhaps better to fail connecting, to not give false sense of encryption security? DOWNGRADE ATTACK VECTOR? Encryption is configured to be OPTIONAL, so maybe allowable? - // possibly: expectedOutcome.set(ENCRYPTED_WITH_DIALBACK), // do not fail TLS with invalid client cert, as it's usable for encryption even if it's not used for authentication. DOWNGRADE ATTACK VECTOR? + if (receivingServer.strictCertificateValidation) { + expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS (as per RFC 6120 section 13.7.2)."); + } else if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { + expectedOutcome.set(ENCRYPTED_WITH_DIALBACK_AUTH, "Receiving Entity may choose to ignore Initiating Entities invalid certificate (for encryption purposes only) and choose to authenticate with Server Dialback (per RFC 7590 Section 3.4)"); } else { - expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS. Even if Receiving Entity would negotiate TLS for encryption, it can't use Initiating Entity's invalid cert for Authentication. As Dialback is also not available, authentication cannot occur."); + expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate. As Server Dialback is not available, authentication cannot occur."); } break; case VALID: @@ -264,9 +276,13 @@ public static ExpectedOutcome generateExpectedOutcome(final ServerSettings initi // TODO: should we take into account a manual configuration of an ANON cypher suite, so that encryption-without-authentication can occur via TLS, followed by a Dialback-based authentication? break; case INVALID: - expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate. The Initiating Entity cannot negotiate TLS and therefor the required encrypted connection cannot be established."); - // TODO: should we allow TLS to be used anyway, so that encryption-without-authentication can occur via TLS, followed by a Dialback-based authentication? THIS INTRODUCES DOWNGRADE ATTACK VECTOR. - // AG: This is the expected behaviour, OF-2555 suggests this is not the current behaviour of Openfire as "if validation fails but Dialback is available", the connection is made. + if (initiatingServer.strictCertificateValidation) { + expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate, which should cause Initiating Entity to abort TLS (as per RFC 6120 section 13.7.2)."); + } else if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { + expectedOutcome.set(ENCRYPTED_WITH_DIALBACK_AUTH, "Initiating Entity may choose to ignore Receiving Entities invalid certificate (for encryption purposes only) and choose to authenticate with Server Dialback (per RFC 7590 Section 3.4)"); + } else { + expectedOutcome.set(NO_CONNECTION, "Receiving Entity provides an invalid TLS certificate. As Server Dialback is not available, authentication cannot occur."); + } break; case VALID: switch (initiatingServer.certificateState) { @@ -278,15 +294,12 @@ public static ExpectedOutcome generateExpectedOutcome(final ServerSettings initi } break; case INVALID: - // TODO: should the Receiving Entity be allowed to authenticate using Dialback? Is this possibly an allowable OF-2591 edge-case? - if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { - // TODO: should the Initiating Entity be allowed to authenticate using Dialback over an encrypted TLS connection - or should TLS fail hard when invalid certs are used (the client _could_ opt to not send those...)? Is this possibly an allowable OF-2591 edge-case, or is this a DOWNGRADE ATTACK vector? - expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS."); - // AG: this is correct I think. The language in 13.7.2 states in block capitals if a certificate is present it MUST attempt validation; if the validation fails, the connection terminates. See RFC2119 to confirm this is an absolute requirement. - // possibly: expectedOutcome.set(NON_ENCRYPTED_WITH_DIALBACK), // fail TLS but allow unencrypted. Perhaps better to fail connecting, to not give false sense of encryption security? DOWNGRADE ATTACK VECTOR? Encryption is configured to be OPTIONAL, so maybe allowable? - // possibly: expectedOutcome.set(ENCRYPTED_WITH_DIALBACK), // do not fail TLS with invalid client cert, as it's usable for encryption even if it's not used for authentication. DOWNGRADE ATTACK VECTOR? + if (receivingServer.strictCertificateValidation) { + expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS (as per RFC 6120 section 13.7.2)."); + } else if (initiatingServer.dialbackSupported && receivingServer.dialbackSupported) { + expectedOutcome.set(ENCRYPTED_WITH_DIALBACK_AUTH, "Receiving Entity may choose to ignore Initiating Entities invalid certificate (for encryption purposes only) and choose to authenticate with Server Dialback (per RFC 7590 Section 3.4)"); } else { - expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate, which should cause Receiving Entity to abort TLS. Even if Receiving Entity would negotiate TLS for encryption, it can't use Initiating Entity's invalid cert for Authentication. As Dialback is also not available, authentication cannot occur."); + expectedOutcome.set(NO_CONNECTION, "Initiating Entity provides an invalid TLS certificate. As Server Dialback is not available, authentication cannot occur."); } break; case VALID: diff --git a/xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalIncomingServerSessionTest.java b/xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalIncomingServerSessionTest.java index 4c1551cfe8..33ab6e5964 100644 --- a/xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalIncomingServerSessionTest.java +++ b/xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalIncomingServerSessionTest.java @@ -314,7 +314,7 @@ private static Iterable arguments() { if (tlsPolicy == Connection.TLSPolicy.legacyMode) { continue; // TODO add support for DirectTLS in this unit test! } - final ServerSettings serverSettings = new ServerSettings(tlsPolicy, certificateState, dialbackSupported); + final ServerSettings serverSettings = new ServerSettings(tlsPolicy, certificateState, dialbackSupported, true); // TODO add support for both strict certificate validation settings. localServerSettings.add(serverSettings); remoteServerSettings.add(serverSettings); } diff --git a/xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalOutgoingServerSessionTest.java b/xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalOutgoingServerSessionTest.java index 4ec431c163..95375b77bd 100644 --- a/xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalOutgoingServerSessionTest.java +++ b/xmppserver/src/test/java/org/jivesoftware/openfire/session/LocalOutgoingServerSessionTest.java @@ -294,7 +294,7 @@ private static Iterable arguments() { if (tlsPolicy == Connection.TLSPolicy.legacyMode) { continue; // TODO add support for DirectTLS in this unit test! } - final ServerSettings serverSettings = new ServerSettings(tlsPolicy, certificateState, dialbackSupported); + final ServerSettings serverSettings = new ServerSettings(tlsPolicy, certificateState, dialbackSupported, true); // TODO add support for both strict certificate validation settings. localServerSettings.add(serverSettings); remoteServerSettings.add(serverSettings); } diff --git a/xmppserver/src/test/java/org/jivesoftware/openfire/session/ServerSettings.java b/xmppserver/src/test/java/org/jivesoftware/openfire/session/ServerSettings.java index 5917816aaa..4e08e230c0 100644 --- a/xmppserver/src/test/java/org/jivesoftware/openfire/session/ServerSettings.java +++ b/xmppserver/src/test/java/org/jivesoftware/openfire/session/ServerSettings.java @@ -51,31 +51,40 @@ public enum CertificateState // NEEDED // } + /** + * Defines if this entity requires/disables or can use TLS for encryption (this does not mandate TLS-based authentication). + */ + public final Connection.TLSPolicy encryptionPolicy; + /** * Describes the certificate that's offered by this entity. */ public final CertificateState certificateState; /** - * Defines if this entity will support the Dialback authentication mechanism. + * When Dialback is allowed, unauthenticated TLS encryption is better than no encryption. This, however, breaks with + * a strict interpretation of RFC 6120 section 13.7.2 (while it appears allowable in RFC 7590 Section 3.4). Openfire + * con be configured either way, by setting the 'strict certificate validation' configuration. This field will take + * into account that setting. */ - public final boolean dialbackSupported; + public final boolean strictCertificateValidation; /** - * Defines if this entity requires/disables or can use TLS for encryption (this does not mandate TLS-based authentication). + * Defines if this entity will support the Dialback authentication mechanism. */ - public final Connection.TLSPolicy encryptionPolicy; + public final boolean dialbackSupported; // /** // * Defines if this entity will attempt/require/ignore to validate the peer's certificate // */ // public final TlsMutualAuthenticationPolicy tlsMutualAuthenticationPolicy; - public ServerSettings(final Connection.TLSPolicy encryptionPolicy, final CertificateState certificateState, final boolean dialbackSupported) + public ServerSettings(final Connection.TLSPolicy encryptionPolicy, final CertificateState certificateState, final boolean strictCertificateValidation, final boolean dialbackSupported) { + this.encryptionPolicy = encryptionPolicy; this.certificateState = certificateState; + this.strictCertificateValidation = strictCertificateValidation; this.dialbackSupported = dialbackSupported; - this.encryptionPolicy = encryptionPolicy; } @Override @@ -87,9 +96,9 @@ public String toString() public String toString(int length) { if (length > 0) { - return "[encryption=" + encryptionPolicy.toString().substring(0, length) + ", certificate=" + certificateState.toString().substring(0, length) + ", dialback=" + (dialbackSupported ? "SUPPORTED" : "DISABLED").substring(0, length) + "]"; + return "[encryption=" + encryptionPolicy.toString().substring(0, length) + ", certificate=" + certificateState.toString().substring(0, length) + ", strictCertValidation=" + strictCertificateValidation + ", dialback=" + (dialbackSupported ? "SUPPORTED" : "DISABLED").substring(0, length) + "]"; } else { - return "[encryption=" + encryptionPolicy.toString() + ", certificate=" + certificateState.toString() + ", dialback=" + (dialbackSupported ? "SUPPORTED" : "DISABLED") + "]"; + return "[encryption=" + encryptionPolicy.toString() + ", certificate=" + certificateState.toString() + ", strictCertValidation=" + strictCertificateValidation + ", dialback=" + (dialbackSupported ? "SUPPORTED" : "DISABLED") + "]"; } } }