Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SNOW-1898533: Race condition in HttpUtil#buildHttpClient causing incorrect proxy settings to be used #2047

Open
ngucandy opened this issue Jan 26, 2025 · 3 comments
Assignees
Labels
bug status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. status-triage_done Initial triage done, will be further handled by the driver team

Comments

@ngucandy
Copy link

Please answer these questions before submitting your issue.
In order to accurately debug the issue this information is required. Thanks!

  1. What version of JDBC driver are you using?
    3.20 but confirmed my test fails with main branch as of Jan 26, 2025

  2. What operating system and processor architecture are you using?
    macOS x64

  3. What version of Java are you using?
    17

  4. What did you do?
    Unit test that reproduces the problem:

  @Test
  public void buildHttpClientRace() throws InterruptedException {
    // start two threads but only need one to fail
    CountDownLatch latch = new CountDownLatch(1);
    final Queue<AbstractMap.SimpleEntry<Thread,Throwable>> failures = new ConcurrentLinkedQueue<>();
    final HttpClientSettingsKey noProxyKey = new HttpClientSettingsKey(null);
    final HttpClientSettingsKey proxyKey =
        new HttpClientSettingsKey(null, "some.proxy.host", 8080, null, null, null, "http", null, false);

    Thread noProxyThread = new Thread( () -> {
        while (!Thread.currentThread().isInterrupted()) {
          try (CloseableHttpClient client = HttpUtil.buildHttpClient(noProxyKey, null, false)) {
            MatcherAssert.assertThat(client, CoreMatchers.instanceOf(Configurable.class));
            Configurable c = (Configurable) client;
            RequestConfig config = c.getConfig();
            MatcherAssert.assertThat("client using proxy when it should not be", config.getProxy(), CoreMatchers.nullValue());
          } catch (IOException | AssertionError e) {
            failures.add(new AbstractMap.SimpleEntry<>(Thread.currentThread(),e));
            latch.countDown();
            break;
          }
        }
    }, "noProxyThread");
    noProxyThread.start();

    Thread withProxyThread = new Thread( () -> {
        while (!Thread.currentThread().isInterrupted()) {
          try (CloseableHttpClient client = HttpUtil.buildHttpClient(proxyKey, null, false)) {
            MatcherAssert.assertThat(client, CoreMatchers.instanceOf(Configurable.class));
            Configurable c = (Configurable) client;
            RequestConfig config = c.getConfig();
            HttpHost proxy = config.getProxy();
            MatcherAssert.assertThat("client not using proxy when it should", proxy, CoreMatchers.notNullValue());
            MatcherAssert.assertThat("client using incorrect proxy", proxy.getHostName(),
                CoreMatchers.equalTo("some.proxy.host"));
          } catch (IOException | AssertionError e) {
            failures.add(new AbstractMap.SimpleEntry<>(Thread.currentThread(),e));
            latch.countDown();
            break;
          }
        }
    }, "withProxyThread");
    withProxyThread.start();

    // if latch goes to zero, then one of the threads failed
    // if await times out (returns false), then neither thread has failed (both still running)
    boolean failed = latch.await(1, TimeUnit.SECONDS);
    noProxyThread.interrupt();
    withProxyThread.interrupt();
    if (failed) {
      AbstractMap.SimpleEntry<Thread, Throwable> failure = failures.remove();
      fail(failure.getKey().getName() + " failed", failure.getValue());
    }
  }
}
  1. What did you expect to see?
    Test should pass but instead fails. Sample failure output I get:
Jan 26, 2025 12:20:43 PM net.snowflake.client.core.FileUtil logFileUsage
INFO: Cache file creation: Accessing file: /Users/andynguyen/Library/Caches/Snowflake/ocsp_response_cache.json
Jan 26, 2025 12:20:43 PM net.snowflake.client.core.FileUtil logFileUsage
INFO: Read cache: Accessing file: /Users/andynguyen/Library/Caches/Snowflake/ocsp_response_cache.json

org.opentest4j.AssertionFailedError: noProxyThread failed

	at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:42)
	at org.junit.jupiter.api.Assertions.fail(Assertions.java:150)
	at net.snowflake.client.core.HttpUtilLatestIT.buildHttpClientRace(HttpUtilLatestIT.java:119)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.AssertionError: client using proxy when it should not be
Expected: null
     but: was <http://some.proxy.host:8080>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at net.snowflake.client.core.HttpUtilLatestIT.lambda$buildHttpClientRace$0(HttpUtilLatestIT.java:83)
	at java.base/java.lang.Thread.run(Thread.java:833)

What should have happened and what happened instead?

  1. Can you set logging to DEBUG and collect the logs?
    Don't think this is applicable here since I can reproduce this with a unit test outside of actually using the driver.

I believe this bug is causing problems in our multi-tenant BI application where one user's Snowflake connection is incorrectly connecting to another user's proxy. Some users of our application connect to Snowflake through proxies while others do not (i.e., connect directly). This generally results in Snowflake rejecting the connection due to an IP address allowlist violation, but is still a major security concern for us.

@ngucandy ngucandy added the bug label Jan 26, 2025
@github-actions github-actions bot changed the title Race condition in HttpUtil#buildHttpClient causing incorrect proxy settings to be used SNOW-1898533: Race condition in HttpUtil#buildHttpClient causing incorrect proxy settings to be used Jan 26, 2025
@ngucandy
Copy link
Author

Based on my understanding of HttpUtil this race exists because DefaultRequestConfig is being held statically and each request can potentially change the static reference to DefaultRequestConfig along with the proxy information held within.

@sfc-gh-dszmolka sfc-gh-dszmolka added the status-triage_done Initial triage done, will be further handled by the driver team label Jan 27, 2025
@sfc-gh-dszmolka
Copy link
Contributor

hi - thank you for filing this issue with us and the pointer! we'll take a look.

@sfc-gh-dszmolka sfc-gh-dszmolka added status-in_progress Issue is worked on by the driver team status-pr_pending_merge A PR is made and is under review and removed status-in_progress Issue is worked on by the driver team labels Jan 30, 2025
@sfc-gh-dszmolka
Copy link
Contributor

sfc-gh-dszmolka commented Feb 3, 2025

PR is up: #2060

edit: PR is merged and will be part of the next release

@sfc-gh-dszmolka sfc-gh-dszmolka added status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. and removed status-pr_pending_merge A PR is made and is under review labels Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug status-fixed_awaiting_release The issue has been fixed, its PR merged, and now awaiting the next release cycle of the connector. status-triage_done Initial triage done, will be further handled by the driver team
Projects
None yet
Development

No branches or pull requests

3 participants