Skip to content
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

Native Sockets TLS Client/Server support for linux #2939

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

olme04
Copy link
Contributor

@olme04 olme04 commented Apr 9, 2022

Based on: #2929

Subsystem
Network, CIO

Motivation
partially fixes https://youtrack.jetbrains.com/issue/KTOR-1181 (no darwin support yet)

Solution

  • Use cinterop with openssl on linux to handle TLS
  • Support both client/server authentication and verification
  • Added support for multiplatform configuration of TLS via PKCS12 certificates
  • Update JVM implementation of TLS to use SSLEngine with new API (server TLS, authentication for client and so on), but use old implementation if new API isn't used.

Note:

  • I have no ability to make it working on darwin, as I have no mac, so implementation is fully in linux sourceset
  • on Darwin, we can use SecureTransport api, which is out of the box coming in kotlin-native via Foundation. openssl isn't needed, which will simplify adopting it. SecureTransport has similar API to both openssl and JVM SSLEngine

TODO:

  • support certificate generation via openssl - for now, I created 2 certificates with 1000 days validity for tests via tls-certificates module
  • Implementations of both SSLEngine and via openssl is behaving differently when working with certificates - f.e. JVM don't check certificate validity and server name validity.
  • May be we can introduce such an API with ExperimentalTLSApi annotation, because of possible bugs, and get more feedback on cases, where it can have unexpected behavior?
  • More tests for TLS specifics is needed: different certificates (expired, wrong data), passwords validation, TLS session graceful closing
  • For now, cinterop on linux points to brew location, because, at least on my machine, if to use same paths as in curl, it will fail too link binary because of GLIBC issues (related: https://youtrack.jetbrains.com/issue/KT-43501, https://youtrack.jetbrains.com/issue/KT-47061#focus=Comments-27-4947040.0-0)
  • Need to restore binary compatibility on JVM - need to add some hidden declarations, from source perspective, changes are compatible

handleResult(result)
if (result.status == SSLEngineResult.Status.CLOSED) break@loop
}
} catch (cause: Throwable) {
Copy link

Choose a reason for hiding this comment

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

TooGenericExceptionCaught: Caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.

(at-me in a reply with help or ignore)

if (unwrapDestination !== initialUnwrapDestination) updateUnwrapDestination(unwrapDestination)
resumeUnwrapContinuation(result)
return result
} catch (cause: Throwable) {
Copy link

Choose a reason for hiding this comment

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

TooGenericExceptionCaught: Caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.

(at-me in a reply with help or ignore)

}

return result
} catch (cause: Throwable) {
Copy link

Choose a reason for hiding this comment

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

TooGenericExceptionCaught: Caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.

(at-me in a reply with help or ignore)

"Failed to parse PKCS12 file"
}
return PKCS12Certificate(privateKey.value!!, x509Certificate.value!!)
} catch (cause: Throwable) {
Copy link

Choose a reason for hiding this comment

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

TooGenericExceptionCaught: Caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.

(at-me in a reply with help or ignore)

this.channel.flush()
//println("[$debugString] READING STEP RESULT: $result")
} while (result > 0)
} catch (cause: Throwable) {
Copy link

Choose a reason for hiding this comment

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

TooGenericExceptionCaught: Caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled.

(at-me in a reply with help or ignore)

api(project(":ktor-network"))
api(project(":ktor-utils"))
kotlin {
linuxX64 {
Copy link
Member

Choose a reason for hiding this comment

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

could we try adding other platofrms?


package io.ktor.network.tls

public class PKCS12Certificate(
Copy link
Member

Choose a reason for hiding this comment

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

kdocs are missing


public class PKCS12Certificate(
public val path: String,
public val password: (() -> CharArray)?
Copy link
Member

Choose a reason for hiding this comment

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

consider adding default if password really can be null

public actual val verification: TLSVerificationConfig?,
) : Closeable {
override fun close() {
//NOOP
Copy link
Member

Choose a reason for hiding this comment

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

obsolete

import io.ktor.utils.io.core.*

//TODO: make it closeable
public expect class TLSConfig: Closeable {
Copy link
Member

Choose a reason for hiding this comment

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

Could you tell me why the config is closeable?

writeStringUtf8("Connection: close\r\n\r\n")
flush()
}
println(socket.openReadChannel().readRemaining().readText())
Copy link
Member

Choose a reason for hiding this comment

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

please consider dropping println

@Tiebe
Copy link

Tiebe commented Aug 2, 2023

@olme04 are you still working on this?

EDIT: ah, zero contributions the past year, on any github repo :(

@whyoleg
Copy link
Contributor

whyoleg commented Aug 3, 2023

@Tiebe just in case, while using openssl here could be a good idea, the main problem with this approach is how to distribute it. I mean, we can do or dynamic or static linking to openssl or 2 separate artefacts for each, but it will be not so straightforward for end-users and library developers what to choose. Also there could be issues with how cinterop for curl (client engine) works with this cinterop for openssl. While this could be now implemented, there are a lot of questions and issues which should be done to make this production ready. As you see, there are a lot of other things in TODO column.

@Tiebe
Copy link

Tiebe commented Aug 3, 2023

@whyoleg Fair enough. Would just really love to have a websocket client with TLS support in Kotlin Native. Would you have any recommendations?

@whyoleg
Copy link
Contributor

whyoleg commented Aug 4, 2023

@Tiebe I think, that for Apple platforms you can use darwin engine, for Windows - winhttp, for linux - there is really nothing now, because there are 2 ways: write TLS impl from scratch (you will still need cryptography support, from scratch or library, which have similar limitations) or use openssl/borinssl etc and leave with inconveniences :(
So, if you need linux support - you have no good ways for now

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.

None yet

4 participants