-
Notifications
You must be signed in to change notification settings - Fork 557
fix: respect NODE_TLS_REJECT_UNAUTHORIZED environment variable #2602
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,7 +202,9 @@ export class KubeConfig implements SecurityAuthentication { | |
agentOptions.key = opts.key; | ||
agentOptions.pfx = opts.pfx; | ||
agentOptions.passphrase = opts.passphrase; | ||
agentOptions.rejectUnauthorized = opts.rejectUnauthorized; | ||
if (opts.rejectUnauthorized !== undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes in this file are very likely to be refactored in the future by someone missing the context of this PR. Can you either add comments to the two code blocks in this file, or rewrite as something along the lines of |
||
agentOptions.rejectUnauthorized = opts.rejectUnauthorized; | ||
} | ||
// The ws docs say that it accepts anything that https.RequestOptions accepts, | ||
// but Typescript doesn't understand that idea (yet) probably could be fixed in | ||
// the typings, but for now just cast to any | ||
|
@@ -259,7 +261,9 @@ export class KubeConfig implements SecurityAuthentication { | |
agentOptions.key = httpsOptions.key; | ||
agentOptions.pfx = httpsOptions.pfx; | ||
agentOptions.passphrase = httpsOptions.passphrase; | ||
agentOptions.rejectUnauthorized = httpsOptions.rejectUnauthorized; | ||
if (httpsOptions.rejectUnauthorized !== undefined) { | ||
agentOptions.rejectUnauthorized = httpsOptions.rejectUnauthorized; | ||
} | ||
|
||
context.setAgent(this.createAgent(cluster, agentOptions)); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ | |
import mockfs from 'mock-fs'; | ||
|
||
import { Authenticator } from './auth.js'; | ||
import { Headers } from 'node-fetch'; | ||
import fetch, { Headers } from 'node-fetch'; | ||
import { HttpMethod } from './index.js'; | ||
import { assertRequestAgentsEqual, assertRequestOptionsEqual } from './test/match-buffer.js'; | ||
import { CoreV1Api, RequestContext } from './api.js'; | ||
|
@@ -27,6 +27,8 @@ | |
import { ExecAuth } from './exec_auth.js'; | ||
import { HttpProxyAgent, HttpsProxyAgent } from 'hpagent'; | ||
import { SocksProxyAgent } from 'socks-proxy-agent'; | ||
import { AddressInfo } from 'node:net'; | ||
import selfsigned from 'selfsigned'; | ||
|
||
const kcFileName = 'testdata/kubeconfig.yaml'; | ||
const kc2FileName = 'testdata/kubeconfig-2.yaml'; | ||
|
@@ -491,6 +493,63 @@ | |
|
||
strictEqual(rc.getAgent() instanceof https.Agent, true); | ||
}); | ||
|
||
it('should apply NODE_TLS_REJECT_UNAUTHORIZED from environment to agent', async () => { | ||
const { server, host, port } = await createTestHttpsServer((req, res) => { | ||
res.setHeader('Content-Type', 'application/json'); | ||
if (req.url?.includes('/api/v1/namespaces')) { | ||
res.writeHead(200); | ||
res.end( | ||
JSON.stringify({ | ||
apiVersion: 'v1', | ||
kind: 'NamespaceList', | ||
items: [ | ||
{ | ||
apiVersion: 'v1', | ||
kind: 'Namespace', | ||
metadata: { name: 'default' }, | ||
}, | ||
], | ||
}), | ||
); | ||
} else { | ||
res.writeHead(200); | ||
res.end('ok'); | ||
} | ||
}); | ||
|
||
const originalValue = process.env.NODE_TLS_REJECT_UNAUTHORIZED; | ||
process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'; | ||
|
||
after(() => { | ||
server.close(); | ||
process.env.NODE_TLS_REJECT_UNAUTHORIZED = originalValue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would reverse the order of these two lines in case |
||
}); | ||
|
||
const kc = new KubeConfig(); | ||
kc.loadFromClusterAndUser( | ||
{ | ||
name: 'test-cluster', | ||
server: `https://${host}:${port}`, | ||
// ignore skipTLSVerify specified from environment variables | ||
} as Cluster, | ||
{ | ||
name: 'test-user', | ||
token: 'test-token', | ||
}, | ||
); | ||
const coreV1Api = kc.makeApiClient(CoreV1Api); | ||
const namespaceList = await coreV1Api.listNamespace(); | ||
|
||
strictEqual(namespaceList.kind, 'NamespaceList'); | ||
strictEqual(namespaceList.items.length, 1); | ||
strictEqual(namespaceList.items[0].metadata?.name, 'default'); | ||
|
||
const res2 = await fetch(`https://${host}:${port}`, await kc.applyToFetchOptions({})); | ||
strictEqual(res2.status, 200); | ||
strictEqual(await res2.text(), 'ok'); | ||
|
||
delete process.env.NODE_TLS_REJECT_UNAUTHORIZED; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this line necessary since the |
||
}); | ||
}); | ||
|
||
describe('loadClusterConfigObjects', () => { | ||
|
@@ -1827,3 +1886,36 @@ | |
}); | ||
}); | ||
}); | ||
|
||
// create a self-signed HTTPS test server | ||
async function createTestHttpsServer( | ||
requestHandler?: (req: http.IncomingMessage, res: http.ServerResponse) => void, | ||
): Promise<{ | ||
server: https.Server; | ||
host: string; | ||
port: number; | ||
ca: string; | ||
}> { | ||
const host = 'localhost'; | ||
const { private: key, cert } = selfsigned.generate([{ name: 'commonName', value: host }]); | ||
|
||
const defaultHandler = (req: http.IncomingMessage, res: http.ServerResponse) => { | ||
res.writeHead(200); | ||
res.end('ok'); | ||
}; | ||
|
||
const server = https.createServer({ key, cert }, requestHandler ?? defaultHandler); | ||
|
||
const port = await new Promise<number>((resolve) => { | ||
server.listen(0, () => { | ||
resolve((server.address() as AddressInfo).port); | ||
}); | ||
}); | ||
|
||
return { | ||
server, | ||
host, | ||
port, | ||
ca: cert, // ca is the same as cert here | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So,
selfsigned
looks to be about 1.7MB, and it is only used in one test. It might be simpler to use a fixture certificate.