Enable TLS on lens-k8s-proxy #4941

jakolehm · 2022-03-01T08:11:56Z

Enables secure TLS connections from clients (mainly LensProxy) to lens-k8s-proxy. Follow-up PR will enable TLS also from clients (renderer etc.) to LensProxy.

Signed-off-by: Jari Kolehmainen <[email protected]>

src/main/kube-auth-proxy/kube-auth-proxy-cert.injectable.ts

src/main/kube-auth-proxy/kube-auth-proxy-cert.ts

src/main/kube-auth-proxy/kube-auth-proxy.ts

Iku-turso · 2022-03-01T08:48:03Z

src/main/kube-auth-proxy/kube-auth-proxy-cert.ts

+export async function createKubeAuthProxyCertificateFiles(dir: string): Promise<string> {
+  const cert = getKubeAuthProxyCertificate();
+
+  try {


Avoidable try/catch as part of business-logic, which is a code-smell. Try/catch is best reserved for fatal errors, and using it for something else damages credibility of eg. centralized error handling.

src/main/kube-auth-proxy/kube-auth-proxy-cert.ts

Iku-turso · 2022-03-01T08:51:23Z

src/main/kube-auth-proxy/kube-auth-proxy-cert.ts

+
+  try {
+    await access(dir);
+  } catch {


"catch all" could be "catch specific" if decision is to use catch here.

src/main/context-handler/context-handler.ts

Iku-turso · 2022-03-01T09:04:14Z

src/main/kube-auth-proxy/kube-auth-proxy-cert.ts

+import path from "path";
+import * as selfsigned from "selfsigned";
+
+export function getKubeAuthProxyCertificate(): selfsigned.SelfSignedCert {


Does not require to be a function?

Iku-turso · 2022-03-01T09:05:40Z

src/main/kube-auth-proxy/kube-auth-proxy-cert.ts

+    { name: "organizationName", value: "Lens" },
+  ];
+
+  return selfsigned.generate(opts, {


Is selfSigned.generate deterministic? If not, it is an explicit side-effect, and should be a dependency instead.

Not 100% deterministic because cert creation always creates an unique certificate. That said, it has deterministic fields... not sure where we should draw the line here.

src/main/kube-auth-proxy/create-kube-auth-proxy.injectable.ts

Iku-turso · 2022-03-01T09:14:56Z

src/main/kube-auth-proxy/kube-auth-proxy-cert.ts

+  });
+}
+
+export function getKubeAuthProxyCertificatePath(baseDir: string) {


Thought: a convention is to use function to advertise that this has special significance. Here it does not, and lambda could be used to advertise that instead.

I would disagree, this codebase only uses class-es for this significant code and lambdas can sometimes make the code harder to read.

Iku-turso · 2022-03-01T09:17:25Z

src/main/kube-auth-proxy/kube-auth-proxy-cert.injectable.ts

+import directoryForUserDataInjectable from "../../common/app-paths/directory-for-user-data/directory-for-user-data.injectable";
+import { createKubeAuthProxyCertificateFiles, getKubeAuthProxyCertificatePath } from "./kube-auth-proxy-cert";
+
+const getKubeAuthProxyCertDirInjectable = getInjectable({


Name of injectable does not document unrelated stuff done in setup.

What would be better naming convention for injectables that have setup?

Iku-turso · 2022-03-01T09:26:20Z

src/main/kube-auth-proxy/kube-auth-proxy-cert.injectable.ts

+import directoryForUserDataInjectable from "../../common/app-paths/directory-for-user-data/directory-for-user-data.injectable";
+import { createKubeAuthProxyCertificateFiles, getKubeAuthProxyCertificatePath } from "./kube-auth-proxy-cert";
+
+const getKubeAuthProxyCertDirInjectable = getInjectable({


This could be wonderfully unit-testable either locally, or preferably as part of a larger system ;)

Iku-turso · 2022-03-01T09:28:40Z

src/main/context-handler/context-handler.ts

@@ -27,6 +29,7 @@ interface PrometheusServicePreferences {

 interface Dependencies {
  createKubeAuthProxy: (cluster: Cluster, environmentVariables: NodeJS.ProcessEnv) => KubeAuthProxy;
+  certPath: string;
 }

 export class ContextHandler {


Note: some unrelated janky code around these parts needs a lovin' champion at some.

Signed-off-by: Jari Kolehmainen <[email protected]>

github-actions · 2022-03-02T22:23:51Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2022-03-10T13:06:36Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

…s-k8s-proxy-tls

Signed-off-by: Jari Kolehmainen <[email protected]>

jakolehm · 2022-03-11T06:51:07Z

src/main/index.ts

@@ -62,9 +62,11 @@ const di = getDi();

 app.setName(appName);

-di.runSetups().then(() => {
-  injectSystemCAs();
+app.on("ready", async () => {


App start order was previously wrong... any async setups caused app.on("ready") registration happen too late -> we missed it completely.

So that we can catch these async errors I would suggest a slightly different shape:

async function main() { const di = getDi(); await app.whenReady(); await di.runSetups(); ... } main().catch(error => { console.error(error); dialog.showErrorBox("Lens Startup Failed", String(error)); app.exit(1); });

Signed-off-by: Jari Kolehmainen <[email protected]>

jakolehm · 2022-03-11T12:05:25Z

src/main/proxy-functions/kube-api-upgrade-request.ts

 import url from "url";
 import { apiKubePrefix } from "../../common/vars";
 import type { ProxyApiRequestArgs } from "./types";

 const skipRawHeaders = new Set(["Host", "Authorization"]);

-export async function kubeApiRequest({ req, socket, head, cluster }: ProxyApiRequestArgs) {
+export async function kubeApiUpgradeRequest({ req, socket, head, cluster }: ProxyApiRequestArgs) {


Renamed because name was misleading, only upgrade requests are handled here.

Signed-off-by: Jari Kolehmainen <[email protected]>

src/main/context-handler/context-handler.ts

Nokel81 · 2022-03-11T14:20:03Z

src/main/context-handler/context-handler.ts

+  async resolveAuthProxyCa() {
+    return readFile(path.join(this.dependencies.certPath, "proxy.crt"));


If this file never changes I think it should be a setup-able dependency.

Refactored. Now authProxyCa is a dependency (but still a promise because of createContextHandlerInjectable instantiate has to be non-async for now). This way ContextHandler does not need to know readFile at all.

Nokel81 · 2022-03-11T14:26:40Z

src/main/index.ts

@@ -62,9 +62,11 @@ const di = getDi();

 app.setName(appName);

-di.runSetups().then(() => {
-  injectSystemCAs();
+app.on("ready", async () => {


So that we can catch these async errors I would suggest a slightly different shape:

async function main() { const di = getDi(); await app.whenReady(); await di.runSetups(); ... } main().catch(error => { console.error(error); dialog.showErrorBox("Lens Startup Failed", String(error)); app.exit(1); });

Nokel81 · 2022-03-11T14:28:04Z

src/main/kube-auth-proxy/create-kube-auth-proxy-cert-files.injectable.ts

+
+  instantiate: async (di) => {
+    const userData = di.inject(directoryForUserDataInjectable);
+    const certPath = getKubeAuthProxyCertificatePath(userData);


This sounds more like a dependency which should depend on directoryForUserDataInjectable.

Indeed, there was already an injectable for this. Refactored.

Nokel81 · 2022-03-11T14:31:16Z

src/main/kube-auth-proxy/create-kube-auth-proxy-cert-files.injectable.ts

+    const userData = di.inject(directoryForUserDataInjectable);
+    const certPath = getKubeAuthProxyCertificatePath(userData);
+
+    return createKubeAuthProxyCertFiles(certPath, {


Given that this creates the files and then returns certPath (which coincendentally should probably be called something like directoryForProxyCertificates. I think that this injectable should actually be mostly just setup and then instantiate should return undefined or maybe a new SetupOnly marker type.

Then whatever depends on createKubeAuthProxyCertFilesInjectable could instead just depend on directoryForProxyCertificatesInjectable and know that the setup and creation of the files will already have been done.

Nokel81 · 2022-03-11T14:32:52Z

src/main/kube-auth-proxy/create-kube-auth-proxy.injectable.ts

    const binaryName = getBinaryName("lens-k8s-proxy");
    const dependencies: KubeAuthProxyDependencies = {
      proxyBinPath: path.join(di.inject(directoryForBundledBinariesInjectable), binaryName),
+      proxyCertPath: di.inject(getKubeAuthProxyCertDirInjectable),
+      spawn,


This should be its own injectable, maybe within a folder src/main/child-process since we probably shouldn't have any of those calls within renderer

Nokel81 · 2022-03-11T14:34:30Z

types/selfsigned.d.ts

+ * Licensed under MIT License. See LICENSE in root directory for more information.
+ */
+
+ declare module "selfsigned" {


I think it would be better if these were hosted into @types. The PR process is actually really simple, I have done it a couple of times.

It would be better yes but I think it can be separately from this PR. Note: this declaration is probably not complete, it only touches parts this PR needs.

Signed-off-by: Jari Kolehmainen <[email protected]>

github-actions · 2022-03-14T13:45:42Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: Jari Kolehmainen <[email protected]>

github-actions · 2022-03-14T13:55:46Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

Signed-off-by: Jari Kolehmainen <[email protected]>

jakolehm · 2022-03-15T06:13:27Z

@Nokel81 PTAL again.

Nokel81 · 2022-03-15T13:07:40Z

src/main/lens-proxy.ts

@@ -61,7 +61,7 @@ export class LensProxy extends Singleton {

  public port: number;

-  constructor(protected router: Router, protected proxy: httpProxy, { shellApiRequest, kubeApiRequest, getClusterForRequest }: LensProxyFunctions) {
+  constructor(protected router: Router, protected proxy: httpProxy, { shellApiRequest, kubeApiUpgradeRequest, getClusterForRequest }: LensProxyFunctions) {


nit: all of these feel like dependencies. Should we consolidate them?

Probably yes but sounds like out of scope for this PR?

Nokel81 · 2022-03-15T13:08:19Z

src/renderer/components/delete-cluster-dialog/__tests__/delete-cluster-dialog.test.tsx

@@ -91,6 +92,8 @@ describe("<DeleteClusterDialog />", () => {
  beforeEach(async () => {
    const { mainDi, runSetups } = getDisForUnitTesting({ doGeneralOverrides: true });

+    mainDi.override(createContextHandlerInjectable, () => () => undefined);


Should this be done in getDiForUnitTesting so that it is always run by default?

It cannot be there because some tests do require the real implementation.

Oh yes, that is why I would put it behind some sort of flag (like doGeneralOverrides is). I suggested moving it because I think that the vast majority of tests would want it overridden.

Nokel81

We can change how certain injectables are overridden by default in a future PR

wip: enable tls on lens-k8s-proxy

ad1776d

Signed-off-by: Jari Kolehmainen <[email protected]>

jakolehm added enhancement New feature or request area/security labels Mar 1, 2022

jakolehm added this to the 5.5.0 milestone Mar 1, 2022