Skip to content

RTECO-524 - Add proxy support for OIDC authentication#593

Merged
agrasth merged 6 commits intojfrog:v2from
agrasth:bugfix/RTECO-524
Apr 13, 2026
Merged

RTECO-524 - Add proxy support for OIDC authentication#593
agrasth merged 6 commits intojfrog:v2from
agrasth:bugfix/RTECO-524

Conversation

@agrasth
Copy link
Copy Markdown
Contributor

@agrasth agrasth commented Mar 26, 2026

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • This pull request is on the dev branch.
  • I used npm run format for formatting the code before submitting the pull request.

Add proxy support for OIDC authentication

Summary

The JFrog Azure DevOps extension now supports proxy configurations when retrieving OIDC tokens from Azure DevOps. Previously, the OIDC token request (POST https://dev.azure.com/.../_apis/distributedtask/hubs/.../oidctoken) used the synchronous sync-request library, which does not honor proxy settings. This caused OIDC authentication to fail when the Azure DevOps Agent runs behind a proxy.

Problem

When an Azure DevOps Agent is deployed within an intranet behind a proxy, the extension's OIDC flow fails silently because:

  1. sync-request does not support proxy configuration.
  2. The extension did not read Azure DevOps Agent proxy variables (Agent.ProxyUrl, Agent.ProxyUsername, Agent.ProxyPassword, Agent.ProxyBypassList).

Solution

  • Replaced sync-request with the async typed-rest-client/HttpClient (already a project dependency) for the OIDC token fetch. This client natively supports proxy configuration and automatically respects HTTP_PROXY/HTTPS_PROXY environment variables.
  • Added getProxyConfiguration() to explicitly read Azure DevOps Agent proxy variables and pass them to the HTTP client.
  • Moved the OIDC resolution logic from configureSpecificCliServer (shared by all connection types) into configureJfrogCliServer (JFrog platform connections only), keeping the async footprint minimal.

Async impact

Only the OIDC code path is async. All other connection types (Artifactory, Distribution, Xray) and all other task files remain fully synchronous and unchanged.

Proxy support matrix

Proxy source Supported
HTTP_PROXY / HTTPS_PROXY env vars Yes (handled automatically by typed-rest-client)
Agent.ProxyUrl / Agent.ProxyUsername / Agent.ProxyPassword / Agent.ProxyBypassList Yes (handled explicitly by getProxyConfiguration())

@agrasth agrasth marked this pull request as ready for review March 30, 2026 06:49
@bhanurp bhanurp added the safe to test Approve running integration tests on a pull request label Apr 6, 2026
@github-actions github-actions Bot removed the safe to test Approve running integration tests on a pull request label Apr 6, 2026
Comment thread tests/proxyConfigTest.js Outdated
Comment thread jfrog-tasks-utils/utils.js Outdated
Comment thread jfrog-tasks-utils/utils.js Outdated
Comment thread jfrog-tasks-utils/utils.js
Comment thread jfrog-tasks-utils/utils.js
Comment thread jfrog-tasks-utils/utils.js Outdated
proxyUrl = parsed.toString();
} catch (e) {
tl.warning('Failed to parse proxy URL: ' + e.message);
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the URL parse fails (malformed proxy URL), the function returns without setting any proxy env vars. That means the user's proxy is silently dropped. Is that correct?

I think better approach: warn and fall through with the original proxyUrl (without credentials), rather than abandoning proxy support entirely. The user gets a degraded experience (unauthenticated proxy) rather than no proxy at all.

@bhanurp any thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Removed the return on parse failure — the function now warns and falls through with the original proxyUrl (without credentials), so proxy support isn't lost when the URL can't be parsed.

Comment thread tests/proxyConfigTest.js
assert.strictEqual(config.proxy.proxyPassword, 'pass');
clearProxyVars();
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a test for forwardProxyToEnv() when URL parse throws (the return on parse failure path) should be covered.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the existing forwardProxyToEnv handles invalid proxy URL gracefully test to match the new behavior. It now asserts that HTTP_PROXY and HTTPS_PROXY are set to the original URL when parsing fails (credentials not included).

Comment thread tests/proxyConfigTest.js
assert.strictEqual(config.proxy.proxyUsername, undefined);
clearProxyVars();
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any tests for Agent.ProxyUrl set + HTTP_PROXY already in env- we need to check taht don't override kind of guard you wrote right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already covered — check forwardProxyToEnv does not override existing HTTP_PROXY (line 172 in proxyConfigTest.js).
It sets both process.env.HTTP_PROXY = 'http://existing:9090' and Agent.ProxyUrl = 'http://myproxy:8080' and asserts HTTP_PROXY stays unchanged.

proxyUrl = process.env.HTTPS_PROXY || process.env.https_proxy || process.env.HTTP_PROXY || process.env.http_proxy;
if (proxyUrl) {
tl.debug('Using proxy from environment variable: ' + proxyUrl);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback here is inside the else branch meaning it only applies when Agent.ProxyUrl is absent. But forwardProxyToEnv() already set HTTP_PROXY/HTTPS_PROXY from Agent.ProxyUrl earlier in executeCliTask.

So if Agent.ProxyUrl is set:
forwardProxyToEnv() sets HTTP_PROXY/HTTPS_PROXY in the environment
getProxyConfiguration() reads Agent.ProxyUrl directly (not the env vars)

But if a user sets HTTP_PROXY manually and Agent.ProxyUrl is also set, getProxyConfiguration() will use Agent.ProxyUrl for the typed-rest-client while HTTP_PROXY might point somewhere different.

This dual-source behavior should be documented right? WTDY?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a JSDoc block on getProxyConfiguration explaining the behavior: when Agent.ProxyUrl is set, typed-rest-client uses it directly via this function, while forwardProxyToEnv separately forwards it into HTTP_PROXY/HTTPS_PROXY for JFrog CLI.
If a user also has a different HTTP_PROXY set manually, the two sources diverge intentionally — forwardProxyToEnv never overrides a pre-existing env var.

const requestOptions = getProxyConfiguration();
const httpClient = new httpm.HttpClient(buildAgent, [new credentialsHandler.BearerCredentialHandler(token, false)], requestOptions);
tl.debug('Requesting OIDC token from: ' + url);
const response = await httpClient.post(url, JSON.stringify({}), {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the OIDC endpoint hangs or is unreachable, await httpClient.post(...) will never resolve, leaving the pipeline task stuck indefinitely with no way to recover short of the agent's own job timeout (which is typically 60 minutes based on documentation I read)

So I think requestOptions should have timeout also I feel.

const requestOptions = { ...getProxyConfiguration(), socketTimeout: 30000 };

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Added socketTimeout: 30000 to the request options on the fetchAzureOidcToken call so a hung endpoint won't block the pipeline indefinitely.

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.

3 participants