adding options to support mtls with the coder server. This supports adding PEM certs and keys to the tls requests, and also supports adding a CA cert to the trust store. Also allowing for an alternate hostname that may appear in the certs which is useful for testing or for non-standard cert usage.

This is similar to the change for the vscode plugin:coder/vscode-coder#126

I am not terribly familiar with kotlin or java so let me know if there are non-idiomatic things you want changed.

coryb commented

Oct 23, 2023

View reviewed changes

src/main/kotlin/com/coder/gateway/sdk/CoderRestClientService.kt OutdatedShow resolvedHide resolved

feat: add configuration options to support mtls

2721631

adding options to support mtls with the coder server.  This supportsadding PEM certs and keys to the tls requests, and also supportsadding a CA cert to the trust store.  Also allowing for an alternatehostname that may appear in the certs which is useful for testing orfor non-standard cert usage.

coryb force-pushed thetls-properties branch from8a27010 to2721631Compare

October 23, 2023 21:28

coryb commented

Oct 23, 2023

View reviewed changes

gradle.properties

		@@ -29,4 +29,4 @@ gradleVersion=7.4
		# Opt-out flag for bundling Kotlin standard library.
		# See https://plugins.jetbrains.com/docs/intellij/kotlin.html#kotlin-standard-library for details.
		# suppress inspection "UnusedProperty"
		kotlin.stdlib.default.dependency=false
		kotlin.stdlib.default.dependency=true

Copy link

ContributorAuthor

corybOct 23, 2023

Choose a reason for hiding this comment

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

It seems like we need the kotlin stdlib, without this I was getting:

java.lang.ClassNotFoundException: kotlin.enums.EnumEntries

I suspect this is required since the kotlin 9.x upgrade?

Copy link

Member

code-asherOct 26, 2023

Choose a reason for hiding this comment

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

Where do you see this error? I tried building with this set tofalse and running the resulting plugin in Gateway (2023.2.4) but so far all seems well.

Copy link

ContributorAuthor

corybOct 29, 2023

Choose a reason for hiding this comment

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

I see when running./gradlew build --info or equivalent command via IntelliJ. Strange that you don't see it also 🤔

code-asher approved these changes

Oct 26, 2023

View reviewed changes

Copy link

Member

code-asher left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Fabulous, code looks good to me, keeping in mind that I also lack Kotlin/Java expertise so one might say this is the blind leading the blind. 😆

The way to do this in Kotlin is absolutely wild compared to Node! Honestly I mostly glazed over that part, but I would like to read through it more carefully when I have more time.

I only did some very surface-level testing for regressions but have not had a change to try the new settings yet; I would feel more comfortable if we threw in some unit tests proving the behavior but one must opt into the changes by filling out the settings anyway so I think it is reasonable to merge this in and get a release out so we can start trying it out. I left some minor comments but I am going to have very spotty availability starting tomorrow for a couple weeks which is why I am thinking we rush a bit to publish.

Edit: heading out but will merge/release when I get back tonight.

gradle.properties

		@@ -29,4 +29,4 @@ gradleVersion=7.4
		# Opt-out flag for bundling Kotlin standard library.
		# See https://plugins.jetbrains.com/docs/intellij/kotlin.html#kotlin-standard-library for details.
		# suppress inspection "UnusedProperty"
		kotlin.stdlib.default.dependency=false
		kotlin.stdlib.default.dependency=true

Copy link

Member

code-asherOct 26, 2023

Choose a reason for hiding this comment

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

Where do you see this error? I tried building with this set tofalse and running the resulting plugin in Gateway (2023.2.4) but so far all seems well.

src/main/kotlin/com/coder/gateway/CoderGatewayConnectionProvider.kt

		@@ -140,7 +140,7 @@ class CoderGatewayConnectionProvider : GatewayConnectionProvider {
		if (token == null) { // User aborted.
		throw IllegalArgumentException("Unable to connect to $deploymentURL, $TOKEN is missing")
		}
		val client = CoderRestClient(deploymentURL, token.first, settings.headerCommand, null)
		val client = CoderRestClient(deploymentURL, token.first,null, settings)

Copy link

Member

code-asherOct 26, 2023

Choose a reason for hiding this comment

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

Suggested change

	val client=CoderRestClient(deploymentURL, token.first,null, settings)
	val client=CoderRestClient(deploymentURL, token.first,null, settings)

src/main/kotlin/com/coder/gateway/sdk/CoderRestClientService.kt


		val sslContext = SSLContext.getInstance("TLS")

		val trustManagers = coderTrustManagers(settings.tlsCAPath)

Copy link

Member

code-asherOct 26, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Is it a problem that we only usetlsCAPath if we did not return early above whentlsCertPath ortlsKeyPath are blank? Thinking of a use case were someone only setstlsCAPath but not the others. I see we separately callcoderTrustManagers() in the rest client so maybe it works there, but possibly the binary download could have issues since we are only using the socket factory there.

src/main/kotlin/com/coder/gateway/sdk/CoderRestClientService.kt

		class CoderHostnameVerifier(private val alternateName: String) : HostnameVerifier {
		override fun verify(host: String, session: SSLSession): Boolean {
		if (alternateName.isEmpty()) {
		println("using default hostname verifier, alternateName is empty")

Copy link

Member

code-asherOct 26, 2023

Choose a reason for hiding this comment

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

We should copy the logger pattern used elsewhere instead of usingprintln directly.

src/main/kotlin/com/coder/gateway/views/CoderGatewayRecentWorkspaceConnectionsView.kt

		@@ -256,7 +256,7 @@ class CoderGatewayRecentWorkspaceConnectionsView(private val setContentCallback:
		deployments[dir] ?: try {
		val url = Path.of(dir).resolve("url").readText()
		val token = Path.of(dir).resolve("session").readText()
		DeploymentInfo(CoderRestClient(url.toURL(), token, settings.headerCommand, null))
		DeploymentInfo(CoderRestClient(url.toURL(), token,null, settings))

Copy link

Member

code-asherOct 26, 2023

Choose a reason for hiding this comment

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

Suggested change

	DeploymentInfo(CoderRestClient(url.toURL(), token,null, settings))
	DeploymentInfo(CoderRestClient(url.toURL(), token,null, settings))

src/main/resources/messages/CoderGatewayBundle.properties

		@@ -93,3 +93,21 @@ gateway.connector.settings.header-command.comment=An external command that \
		outputs additional HTTP headers added to all requests. The command must \
		output each header as `key=value` on its own line. The following \
		environment variables will be available to the process: CODER_URL.
		gateway.connector.settings.tls-cert-path.title=Cert Path:

Copy link

Member

code-asherOct 26, 2023

Choose a reason for hiding this comment

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

Not really important, but the other settings areCapitalized like this: so changing these toCert path: etc might be good for a more consistent look (or change the capitalization of the others).