RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values). No test regressions observed in jdk/com/sun/jndi/ldap. -- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 ------------- Commit messages: - Initial commit for JDK-8275535. Changes: https://git.openjdk.java.net/jdk/pull/6043/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6043&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275535 Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6043.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6043/head:pull/6043 PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Hi Martin, The change looks reasonable to me. I would suggest having a CSR logged for this change due to the following [behavioral incompatibility](https://wiki.openjdk.java.net/display/csr/Kinds+of+Compatibility): Before the change - all available endpoints/URLs are tried to create an LDAP context. With the proposed change - incorrect credentials will prevent other endpoints to be exercised to create an LDAP context. Having a CSR will also help to document difference in handling `AuthenticationException` and `NamingException` during construction of an LDAP context from the list of endpoints acquired from a LDAP DNS provider. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 10 Nov 2021 12:58:13 GMT, Aleksei Efimov <aefimov@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Hi Martin,
The change looks reasonable to me. I would suggest having a CSR logged for this change due to the following [behavioral incompatibility](https://wiki.openjdk.java.net/display/csr/Kinds+of+Compatibility): Before the change - all available endpoints/URLs are tried to create an LDAP context. With the proposed change - incorrect credentials will prevent other endpoints to be exercised to create an LDAP context.
Having a CSR will also help to document difference in handling `AuthenticationException` and `NamingException` during construction of an LDAP context from the list of endpoints acquired from a LDAP DNS provider.
Hi @AlekseiEfimov , Thanks for your feedback. I'll open a CSR as suggested and wait for approval before integrating this fix. With that said, I could not find information in the CSR associated to JDK-8160768 (JDK-8192975) about this behavioral change. My intention here is to restore the previous JDK behavior; and not to introduce a new behavior or revert a previously-approved one. Martin.- ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 10 Nov 2021 12:58:13 GMT, Aleksei Efimov <aefimov@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Hi Martin,
The change looks reasonable to me. I would suggest having a CSR logged for this change due to the following [behavioral incompatibility](https://wiki.openjdk.java.net/display/csr/Kinds+of+Compatibility): Before the change - all available endpoints/URLs are tried to create an LDAP context. With the proposed change - incorrect credentials will prevent other endpoints to be exercised to create an LDAP context.
Having a CSR will also help to document difference in handling `AuthenticationException` and `NamingException` during construction of an LDAP context from the list of endpoints acquired from a LDAP DNS provider.
Hi @AlekseiEfimov Can you please review the CSR [1]? Thanks, Martin.- -- [1] - https://bugs.openjdk.java.net/browse/JDK-8276959 ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 17 Nov 2021 20:04:50 GMT, Martin Balao <mbalao@openjdk.org> wrote:
Hi Martin,
The change looks reasonable to me. I would suggest having a CSR logged for this change due to the following [behavioral incompatibility](https://wiki.openjdk.java.net/display/csr/Kinds+of+Compatibility): Before the change - all available endpoints/URLs are tried to create an LDAP context. With the proposed change - incorrect credentials will prevent other endpoints to be exercised to create an LDAP context.
Having a CSR will also help to document difference in handling `AuthenticationException` and `NamingException` during construction of an LDAP context from the list of endpoints acquired from a LDAP DNS provider.
Hi @AlekseiEfimov
Can you please review the CSR [1]?
Thanks, Martin.-
@martinuy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
Please do not close, waiting for CSR approval. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Thu, 16 Dec 2021 01:23:11 GMT, Martin Balao <mbalao@openjdk.org> wrote:
Hi @AlekseiEfimov
Can you please review the CSR [1]?
Thanks, Martin.-
@martinuy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
Please do not close, waiting for CSR approval.
@martinuy, I am the reporter of JDK-8160768. Regarding this PR, isn't everything protocol related a fail-fast issue? E.g., if the socket is up and running, but the LDAP message is rejected can we assume that all subsequent servers for the same resolution will reject the request as well before authentication has happened? The purpose of JDK-8160768 was to discover LDAP servers and connect to the first one reachable. BTW, this code has been running for years now at work: https://github.com/michael-o/activedirectory-dns-locator ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Thu, 16 Dec 2021 01:23:11 GMT, Martin Balao <mbalao@openjdk.org> wrote:
Hi @AlekseiEfimov
Can you please review the CSR [1]?
Thanks, Martin.-
@martinuy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
Please do not close, waiting for CSR approval.
@martinuy Also your Compatibility Risk talks about KDCs, but this is about directory servers. Not sure how this relates here. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Thu, 16 Dec 2021 01:23:11 GMT, Martin Balao <mbalao@openjdk.org> wrote:
Hi @AlekseiEfimov
Can you please review the CSR [1]?
Thanks, Martin.-
@martinuy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
Please do not close, waiting for CSR approval.
@martinuy The CSR is approved, fwiw. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Thu, 16 Dec 2021 01:23:11 GMT, Martin Balao <mbalao@openjdk.org> wrote:
Hi @AlekseiEfimov
Can you please review the CSR [1]?
Thanks, Martin.-
@martinuy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
Please do not close, waiting for CSR approval.
@martinuy Also your Compatibility Risk talks about KDCs, but this is about directory servers. Not sure how this relates here.
Correct, it was an unconscious mistake :) I will try to get this fixed (as the CSR was approved, I'll ask before editing directly). ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Tue, 8 Feb 2022 13:41:28 GMT, Martin Balao <mbalao@openjdk.org> wrote:
@martinuy This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
Please do not close, waiting for CSR approval.
@martinuy Also your Compatibility Risk talks about KDCs, but this is about directory servers. Not sure how this relates here.
Correct, it was an unconscious mistake :) I will try to get this fixed (as the CSR was approved, I'll ask before editing directly).
@martinuy, I am the reporter of JDK-8160768. Regarding this PR, isn't everything protocol related a fail-fast issue? E.g., if the socket is up and running, but the LDAP message is rejected can we assume that all subsequent servers for the same resolution will reject the request as well before authentication has happened?
It looks to me that it's not only a fail-fast issue because the state on the directory side might be altered by each try, as it happened in the reported case. In other words, the client might cause a denial-of-service blocking an account by trying multiple times the same incorrect authentication credentials on each resolved server. In regards to the 2nd question, I guess that we cannot assume that. But the revert is intended for failed authentication only. Is there a risk that you foresee by reverting the failed-authentication behavior back to pre-JDK-8160768? ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Tue, 8 Feb 2022 13:51:57 GMT, Martin Balao <mbalao@openjdk.org> wrote:
@martinuy, I am the reporter of JDK-8160768. Regarding this PR, isn't everything protocol related a fail-fast issue? E.g., if the socket is up and running, but the LDAP message is rejected can we assume that all subsequent servers for the same resolution will reject the request as well before authentication has happened?
It looks to me that it's not only a fail-fast issue because the state on the directory side might be altered by each try, as it happened in the reported case. In other words, the client might cause a denial-of-service blocking an account by trying multiple times the same incorrect authentication credentials on each resolved server.
Let me add a few points for consideration from my usecases, since I don't use any password-based authentication with SASL and Active Directory only: * `SASL EXTERNAL`: What if the certificate is rejected? Trust store is not properly populated, CRL misconfigured, etc. Will an exception also thrown here? * `SASL GSSAPI/GSS-SPNEGO`: Dir server does not manage creds, but KDC does. I am thinking whether fail fast is reasonable. SPN not registered, next server might have. replay/out of sequence, etc.
In regards to the 2nd question, I guess that we cannot assume that. But the revert is intended for failed authentication only.
But the auth is part of the LDAP message as well since the auth does not happen out of band. I would expect that any non-transport issue should fail fast.
Is there a risk that you foresee by reverting the failed-authentication behavior back to pre-JDK-8160768?
Not really, I virtually never had problems, thought might need annotations when this does not work, see above. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 9 Feb 2022 11:10:14 GMT, Michael Osipov <duke@openjdk.java.net> wrote:
@martinuy, I am the reporter of JDK-8160768. Regarding this PR, isn't everything protocol related a fail-fast issue? E.g., if the socket is up and running, but the LDAP message is rejected can we assume that all subsequent servers for the same resolution will reject the request as well before authentication has happened?
It looks to me that it's not only a fail-fast issue because the state on the directory side might be altered by each try, as it happened in the reported case. In other words, the client might cause a denial-of-service blocking an account by trying multiple times the same incorrect authentication credentials on each resolved server.
In regards to the 2nd question, I guess that we cannot assume that. But the revert is intended for failed authentication only.
Is there a risk that you foresee by reverting the failed-authentication behavior back to pre-JDK-8160768?
@martinuy, I am the reporter of JDK-8160768. Regarding this PR, isn't everything protocol related a fail-fast issue? E.g., if the socket is up and running, but the LDAP message is rejected can we assume that all subsequent servers for the same resolution will reject the request as well before authentication has happened?
It looks to me that it's not only a fail-fast issue because the state on the directory side might be altered by each try, as it happened in the reported case. In other words, the client might cause a denial-of-service blocking an account by trying multiple times the same incorrect authentication credentials on each resolved server.
Let me add a few points for consideration from my usecases, since I don't use any password-based authentication with SASL and Active Directory only: * `SASL EXTERNAL`: What if the certificate is rejected? Trust store is not properly populated, CRL misconfigured, etc. Will an exception also thrown here? * `SASL GSSAPI/GSS-SPNEGO`: Dir server does not manage creds, but KDC does. I am thinking whether fail fast is reasonable. SPN not registered, next server might have. replay/out of sequence, etc.
In regards to the 2nd question, I guess that we cannot assume that. But the revert is intended for failed authentication only.
But the auth is part of the LDAP message as well since the auth does not happen out of band. I would expect that any non-transport issue should fail fast.
Is there a risk that you foresee by reverting the failed-authentication behavior back to pre-JDK-8160768?
Not really, I virtually never had problems, thought might need annotations when this does not work, see above.
Thanks @michael-o for your input. I see the observations that you raised. In my view, we should revert to the previous behavior (as there are users currently affected by the change), and perhaps we can discuss future improvements from there, based on actual cases. @AlekseiEfimov can you please review this change? We need the formal review-approval to move forward. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 9 Feb 2022 15:05:24 GMT, Martin Balao <mbalao@openjdk.org> wrote:
@martinuy, I am the reporter of JDK-8160768. Regarding this PR, isn't everything protocol related a fail-fast issue? E.g., if the socket is up and running, but the LDAP message is rejected can we assume that all subsequent servers for the same resolution will reject the request as well before authentication has happened?
It looks to me that it's not only a fail-fast issue because the state on the directory side might be altered by each try, as it happened in the reported case. In other words, the client might cause a denial-of-service blocking an account by trying multiple times the same incorrect authentication credentials on each resolved server.
Let me add a few points for consideration from my usecases, since I don't use any password-based authentication with SASL and Active Directory only: * `SASL EXTERNAL`: What if the certificate is rejected? Trust store is not properly populated, CRL misconfigured, etc. Will an exception also thrown here? * `SASL GSSAPI/GSS-SPNEGO`: Dir server does not manage creds, but KDC does. I am thinking whether fail fast is reasonable. SPN not registered, next server might have. replay/out of sequence, etc.
In regards to the 2nd question, I guess that we cannot assume that. But the revert is intended for failed authentication only.
But the auth is part of the LDAP message as well since the auth does not happen out of band. I would expect that any non-transport issue should fail fast.
Is there a risk that you foresee by reverting the failed-authentication behavior back to pre-JDK-8160768?
Not really, I virtually never had problems, thought might need annotations when this does not work, see above.
Thanks @michael-o for your input. I see the observations that you raised. In my view, we should revert to the previous behavior (as there are users currently affected by the change), and perhaps we can discuss future improvements from there, based on actual cases.
@AlekseiEfimov can you please review this change? We need the formal review-approval to move forward.
@martinuy Agree ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Update: * CSR: https://bugs.openjdk.java.net/browse/JDK-8276959 ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Is the new behavior something that could be tested with a new non regression test? See existing tests in `test/jdk/com/sun/jndi/ldap` ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Hi Martin, The source changes looks good to me. In case you have an LDAP server that can be used to reproduce this problem then maybe you could try to create a test that uses classes from LDAP test library (`LDAPServer`,`LDAPTestUtils`)? In a nutshell, it could be done by following these steps: - Create a regression tests which uses LDAPServer/LDAPTestUtils similar to tests available in `test/jdk/com/sun/jndi/ldap/blits/AddTests` and `test/jdk/javax/naming/module/src/test/test` - Add `-trace` flag (see test/jdk/com/sun/jndi/ldap/lib/LDAPTestUtils.java for details) as an argument to the test app. When this argument is passed to LDAPTestUtils.initEnv (see first snippet below) '.ldap' trace file is generated by a framework. This is where the real LDAP server is required. - Once `.ldap` trace file is collected use LDAPTestUtils.initEnv and a ServerSocket to create a test server that will replay the collected trace file (see second snippet below). Snippet 1: How trace file can be collected: /* * @test * @library /test/lib ../../lib * @build LDAPServer LDAPTestUtils /javax/naming/module/src/test/test/ * @run main/othervm TraceExampleTest -trace */ public static void collectTrace(String [] args) throws Exception { Hashtable<Object, Object> env; // initialize test env = LDAPTestUtils.initEnv(null, "ldap://127.0.0.1:1389", TraceExampleTest.class.getName(), args, true); DirContext ctx = null; // connect to server ctx = new InitialDirContext(env); } Snippet 2: How trace file can be used (note that the trace file name should match the test class name in this example) it can be used to create an instance of the LDAPServer: /* * @test * @library /test/lib ../../lib * @build LDAPServer LDAPTestUtils /javax/naming/module/src/test/test/ * @run main/othervm TraceExampleTest */ public static void runWithTrace(String [] args) throws Exception { // Create unbound server socket ServerSocket serverSocket = new ServerSocket(); // Bind it to the loopback address SocketAddress sockAddr = new InetSocketAddress( InetAddress.getLoopbackAddress(), 0); serverSocket.bind(sockAddr); // Construct the provider URL for LDAPTestUtils String providerURL = URIBuilder.newBuilder() .scheme("ldap") .loopback() .port(serverSocket.getLocalPort()) .buildUnchecked().toString(); Hashtable<Object, Object> env; // initialize test env = LDAPTestUtils.initEnv(serverSocket, providerURL, TraceExampleTest.class.getName(), args, true); DirContext ctx = null; // connect to the test LDAP server ctx = new InitialDirContext(env); } ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Unfortunately I don't have access to the environment where this problem reproduces and will be difficult/impossible for me to get a real trace from there. What I can say, though, is that the fail-fast authentication behavior previous to the changes in JDK-8160768 was working fine in such environment. Besides that, we didn't have any users reporting issues regarding authentication. The change to revert to the previous behavior is, in my view, trivial. I can try to build a whole new environment that reproduces this problem or see if it's feasible to mock something, but before getting into that I need to understand what the concerns or motivation for that are. This would require more time than originally planned and might postpone this for a while. Martin.- ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
The concerns are two folds: - without a regression test, you have to trust that the source changes actually work, and there's typically no verification possible - without a regression test, the next refactoring change in this area two years from now could undo the fix and nobody would notice I agree that the changes seem safe and given the history seem to be doing what the fix/PR says they do, so I'd be more concerned with the latter. So if it's practical to add a test I'd rather have one. If the behavior is too hard to test - then the appropriate keyword would be `noreg-hard` rather than `noreg-trivial` (I am not sure trivial actually qualifies here - I can see no obvious flaw but I'm not sure we can say that there are obviously no flaws - or that a flaw would become obvious to future maintainers if that code was refactored in a way that removed the fix). ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 9 Feb 2022 19:12:34 GMT, Daniel Fuchs <dfuchs@openjdk.org> wrote:
The concerns are two folds:
* without a regression test, you have to trust that the source changes actually work, and there's typically no verification possible * without a regression test, the next refactoring change in this area two years from now could undo the fix and nobody would notice
Thanks, that was what I initially thought but wanted to check if I was missing something else given the previous discussion. I should be able to generate a build for the user and ask him to test in his real environment. As for the other concern, I'll evaluate options and let you know. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 9 Feb 2022 19:47:19 GMT, Martin Balao <mbalao@openjdk.org> wrote:
Thanks, that was what I initially thought but wanted to check if I was missing something else given the previous discussion. I should be able to generate a build for the user and ask him to test in his real environment. As for the other concern, I'll evaluate options and let you know.
Thanks for the update, Martin. I'm ok with pushing the fix without a test given that the user will verify it in his real environment. Maybe, you could also log a bug for a test addition and describe in it an environment, and sort of a high-level scenario of how JDK-8275535 can be reproduced. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Thu, 10 Feb 2022 15:25:09 GMT, Aleksei Efimov <aefimov@openjdk.org> wrote:
I'm ok with pushing the fix without a test given that the user will verify it in his real environment. Maybe, you could also log a bug for a test addition and describe in it an environment, and sort of a high-level scenario of how JDK-8275535 can be reproduced.
Right - I would be fine with that too. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Thu, 10 Feb 2022 15:25:09 GMT, Aleksei Efimov <aefimov@openjdk.org> wrote:
The concerns are two folds:
* without a regression test, you have to trust that the source changes actually work, and there's typically no verification possible * without a regression test, the next refactoring change in this area two years from now could undo the fix and nobody would notice
Thanks, that was what I initially thought but wanted to check if I was missing something else given the previous discussion. I should be able to generate a build for the user and ask him to test in his real environment. As for the other concern, I'll evaluate options and let you know.
Thanks, that was what I initially thought but wanted to check if I was missing something else given the previous discussion. I should be able to generate a build for the user and ask him to test in his real environment. As for the other concern, I'll evaluate options and let you know.
Thanks for the update, Martin. I'm ok with pushing the fix without a test given that the user will verify it in his real environment. Maybe, you could also log a bug for a test addition and describe in it an environment, and sort of a high-level scenario of how JDK-8275535 can be reproduced.
Hi @AlekseiEfimov , Apologies for the delay. We had no feedback from our customer. However, we released the fix in our builds and there were no regressions reported. I'll proceed with the integration of this fix. Thanks, Martin.- ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Please don't auto-close this bot :) ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
What's the status of this? The last comment reads as if this is good to go. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Tue, 12 Apr 2022 01:17:37 GMT, Andrew John Hughes <andrew@openjdk.org> wrote:
What's the status of this? The last comment reads as if this is good to go.
I believe we're still waiting for Martin to reply to the last comment:
Maybe, you could also log a bug for a test addition and describe in it an environment, and sort of a high-level scenario of how JDK-8275535 can be reproduced.
Otherwise yes - this would be good to go. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Look [here](https://github.com/spring-projects/spring-security/issues/10318) for spring based test case. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
please don't auto-close this bot. ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Hi @martinuy, I think this fix is in a good state: code changes look good, the CSR is approved and our CI shows no JNDI test failures related to this change. As it was mentioned before the only thing we're waiting for is a bug logged for a test addition with a scenario of how issue can be reproduced. If it is not feasible to do that we can proceed without it - I will log a bug and will use a Spring reproducer shared by Carsten (thank you) as a starting point. Therefore, I'm approving this change. ------------- Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Marked as reviewed by dfuchs (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
This pull request has now been integrated. Changeset: 3be394e1 Author: Martin Balao <mbalao@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/3be394e1606dd17c2c14ce806c796f5eb2b1... Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked Reviewed-by: aefimov, dfuchs ------------- PR: https://git.openjdk.java.net/jdk/pull/6043
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao <mbalao@openjdk.org> wrote:
I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to the state previous to JDK-8160768, where an authentication failure stops from trying other LDAP servers with the same credentials [1]. After JDK-8160768 we have 2 possible loops to stop: the one that iterates over different URLs and the one that iterates over different endpoints (after a DNS query that returns multiple values).
No test regressions observed in jdk/com/sun/jndi/ldap.
-- [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137
Will this fix be back-ported to JDK 8/11/17? ------------- PR: https://git.openjdk.org/jdk/pull/6043
participants (8)
-
Aleksei Efimov
-
Andrew John Hughes
-
Carsten Madsen
-
Daniel Fuchs
-
Martin Balao
-
Michael Osipov
-
Ryan Flegel
-
Severin Gehwolf