feat(upstream-proxy): add HTTP Basic and NTLM auth methods#1551
feat(upstream-proxy): add HTTP Basic and NTLM auth methods#1551Andreybest wants to merge 7 commits into
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1551 +/- ##
==========================================
- Coverage 85.51% 85.49% -0.03%
==========================================
Files 83 84 +1
Lines 7877 8093 +216
Branches 1312 1361 +49
==========================================
+ Hits 6736 6919 +183
- Misses 1114 1146 +32
- Partials 27 28 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
@Andreybest LGTM, thanks for this! Just wondering if we want to double-check if this works for other Windows proxy setups, since you mentioned this was verified in Squid + Samba.
Pinging @kriswest @coopernetes as you were active in the previous PR discussion: #1458
| // Returns undefined when no header should be injected. | ||
| const buildProxyAuthHeader = (auth: Auth | undefined): string | undefined => { | ||
| if (!auth) return undefined; | ||
| if (auth.type === 'basic') { |
There was a problem hiding this comment.
Could use the AuthType.Basic enum for consistency
There was a problem hiding this comment.
Makes sense, thanks for pointing out, changed!
| import * as tls from 'tls'; | ||
| import * as http from 'http'; | ||
| import type { Duplex } from 'stream'; | ||
| import { Agent, AgentConnectOpts } from 'agent-base'; |
There was a problem hiding this comment.
Wondering if we should add agent-base and pin it in package.json instead of bringing it in through http-proxy-agent to prevent types from breaking when http-proxy-agent gets autobumped by Dependabot
There was a problem hiding this comment.
My bad, forgot to add it to package.json. Added!
There was a problem hiding this comment.
We might want to verify that the logic here still works with anything other than Squid + Samba. What other popular proxy combinations do we want to support?
There was a problem hiding this comment.
From my undesrstanding - NTLM is Windows protocol, so probably the closest implementation would be a windows one :), and I haven't found a way to emulate it except for "Squid + Samba", I have an access to check it on my side, and would appreciate if somebody would be able to check it on their side.
|
Also: don't forget to bump or pin 
|
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
Found another issue while probing with AI agents 👍🏼
We might need some extra tests to cover these three scenarios on round 1:
- Chunked 407 body (Transfer-Encoding: chunked)
- Unframed 407 body
- A 407 with a body spanning more than one TCP segment
| // and push any over-read bytes back onto the socket via `unshift`. This is | ||
| // load-bearing for NTLM: between Type1 and Type3 we must consume the 407 body | ||
| // so the next read starts cleanly at the second response. | ||
| const readProxyResponse = (socket: net.Socket): Promise<ParsedResponse> => { |
There was a problem hiding this comment.
I tested this with agentic AI to probe a bit on what would happen if the 407 response was structured differently (via a handmade proxy), and there are a few edge cases we might want to look into:
- Chunked 407 body (
Transfer-Encoding: chunked) - Unframed 407 body
- A 407 with a body spanning more than one TCP segment
Apparently these scenarios crash the app. Feel free to ignore if proxies other than Squid don't structure their 407 response body like this - otherwise we should exit gracefully and document the behaviour.
Here's the related proxy and script output:
Proxy test script and output
probe.ts
import * as net from 'net';
import * as http from 'http';
import { NtlmProxyAgent } from './src/proxy/upstream/ntlm-proxy-agent';
const t2 = () => {
const b = Buffer.alloc(48);
b.write('NTLMSSP\0', 0, 8, 'ascii');
b.writeUInt32LE(2, 8);
b.writeUInt32LE(0x00000201, 20);
Buffer.from([1, 2, 3, 4, 5, 6, 7, 8]).copy(b, 24);
return `NTLM ${b.toString('base64')}`;
};
const body = 'Proxy authentication required by gateway';
const readHeaders = (s: net.Socket): Promise<string> =>
new Promise((res) => {
const cs: Buffer[] = [];
const on = (c: Buffer) => {
cs.push(c);
const all = Buffer.concat(cs);
const i = all.indexOf('\r\n\r\n');
if (i !== -1) {
s.removeListener('data', on);
res(all.subarray(0, i).toString('utf8'));
}
};
s.on('data', on);
});
const server = net.createServer(async (socket) => {
socket.on('error', () => {});
await readHeaders(socket);
console.log(`${Date.now() - T0}ms proxy: got round-1 CONNECT, sending chunked 407`);
socket.write(
`HTTP/1.1 407 Proxy Authentication Required\r\nProxy-Authenticate: ${t2()}\r\nTransfer-Encoding: chunked\r\n\r\n${body.length.toString(16)}\r\n${body}\r\n0\r\n\r\n`,
);
const h2 = await readHeaders(socket);
console.log(`${Date.now() - T0}ms proxy: got round-2, first line = ${JSON.stringify(h2.split('\r\n')[0])}`);
socket.write('HTTP/1.1 200 Connection Established\r\n\r\n');
});
const T0 = Date.now();
server.listen(0, '127.0.0.1', () => {
const port = (server.address() as net.AddressInfo).port;
console.log(`${Date.now() - T0}ms proxy listening on ${port}`);
// Independent watchdog — fires no matter what the agent does.
const wd = setTimeout(() => {
console.log(`${Date.now() - T0}ms WATCHDOG: connect() still pending — HANG confirmed`);
process.exit(2);
}, 4000);
const agent = new NtlmProxyAgent({
proxy: `http://127.0.0.1:${port}`,
username: 'user',
password: 'pass',
domain: 'CORP',
workstation: 'WS01',
});
console.log(`${Date.now() - T0}ms calling agent.connect()...`);
agent
.connect({} as http.ClientRequest, { host: 'example.com', port: 80, secureEndpoint: false } as any)
.then((sock) => {
clearTimeout(wd);
(sock as net.Socket).on('error', () => {});
(sock as net.Socket).destroy();
console.log(`${Date.now() - T0}ms connect() RESOLVED (handshake succeeded)`);
server.close();
process.exit(0);
})
.catch((err) => {
clearTimeout(wd);
console.log(`${Date.now() - T0}ms connect() REJECTED: ${(err as Error).message}`);
server.close();
process.exit(1);
});
});Script
cd /tmp/gp && timeout 30 npx tsx probe2.ts 2>&1 | grep -vE "ExperimentalWarning|--import|^\(Use \`node"; echo "(script exit: $?)"Output
9ms proxy listening on 38051
9ms calling agent.connect()...
14ms proxy: got round-1 CONNECT, sending chunked 407
(script exit: 0)
There was a problem hiding this comment.
Lets see if it works on other proxy implementations and circle back to this :) Thanks for testing!
There was a problem hiding this comment.
This is behaviour I have also encountered not with the proxy-being-proxied scenario but with a load balancer in front of git-proxy that doesn't pass through unmodified the chunked encoded payloads or sends it out-of-order, to multiple backend git-proxy servers, etc. I'm fairly certain this is a just a "curse of default" thing - most major load balancers (nginx, HAProxy, etc) support chunked encoding more gracefully and match what git-proxy has to receive in order to do its parsePush steps.
The workaround I've found helpful is to force the git client to use git config http.postBuffer 5452880001 so that a POST /git-receive-pack is sent as a single segment (by default, 1MiB is the upper pack limit when segmenting occurs). This avoids having to deal with chunked encoding entirely and seems a reasonable trade-off for proxy-of-git-proxy support. Otherwise, the NTLM exchange/challenge has to be stateful across multiple requests.
@jescalada do you see the same behaviour with the client side config above? Be curious if that works as an acceptable workaround to include this support while not having to do TCP segment processing.
Footnotes
|
@coopernetes and @kriswest to try and test this - for soft inclusion in 2.1.0 |
|
Seems to work fine in my environment. I did merge all the latest changes from upstream main though. Would suggest updating the branch the same since it's roughly ~200 commits behind. proxy logs with some added verbosity Details |
…nerability in httpntlm package
Andreybest
force-pushed
the
1541-http-basic-ntlm
branch
from
b4870e6 to
c6df24b
Compare
|
Thanks for the review @jescalada! Addressed your concerns. Thanks for testing @coopernetes! |
|
Please note that was just a simple upload pack (fetch) test. A push based test is warranted based on Juan's findings above. I'll try to get that setup and share the results back. |
4 participants
Description
Added HTTP Basic and NTLM authentication methods for upstream proxy.
HTTP Basic was already available by providing
username:passwordin url of proxy, now it is also can be passed inauthparam inproxy.config.json.NTLM was added, tested on local docker squid + samba proxy. But not sure if it is the sme as the one on windows production proxy environments. Would appreciate testing/suggestions on that :)
Negotiate was omitted because support for it has already been merged into the
https-proxy-agentpackage upstream, but a new release containing it hasn't been published yet.Related Issue
Resolves #1541 (partially)
Checklist
General
Documentation
Configuration
config.schema.json) was modified:npm run generate-config-types)npm run gen-schema-doc)Tests
npm test)npm run lintandnpm run format:check)npm run check-types)