Support for Client side certificate and custom list of trusted certificates #92

Hakky54 · 2020-07-19T12:59:56Z

This pull request is regarding the implementation of the issue number #91

This feature will enable loading the client certificate into the http client which could be used when the server requires client authentication based on certificates. The client will also have the ability to trust specific certificates by loading a list of trusted certificates.

This PR is for now a WIP as the jvm part is finished but the node part is still pending

Hakky54 · 2020-07-19T13:05:48Z

@hmil I did an attempt for the initial version of the implementation. The jvm version is mostly done, but I am not quite sure how to implement node version. I think I need the following fields within the RequestOptions:

pfx: js.UndefOr[String] = js.undefined,
passphrase: js.UndefOr[String] = js.undefined,
ca: js.UndefOr[String] = js.undefined

I am also not quite sure if the type String is correct here and I am also not sure how to correctly load a file into this field as I am not sure if it should be a String. Node has the following documentation at their page for loading a pfx or pem file:

onst https = require('https');
const fs = require('fs');

const options = {
  pfx: fs.readFileSync('test/fixtures/test_cert.pfx'),
  passphrase: 'sample'
};

I can fetch the path of the pfx with from the sslconfig within the backendConfig of the request. But I am not quite sure how to properly load it. What do you think?

hmil · 2020-07-20T21:24:33Z

jvm/src/main/scala/fr/hmil/roshttp/HttpDriver.scala

@@ -6,6 +6,7 @@ import java.nio.ByteBuffer
 import fr.hmil.roshttp.exceptions._
 import fr.hmil.roshttp.response.{HttpResponse, HttpResponseFactory, HttpResponseHeader}
 import fr.hmil.roshttp.util.HeaderMap
+import javax.net.ssl.HttpsURLConnection


Just checking, how portable is this?
I'm not 100% aware of the limitations of the javax package.

What do you mean exactly by portable?

hmil · 2020-07-20T21:32:50Z

jvm/src/main/scala/fr/hmil/roshttp/SSLUtils.scala

+      trustManagers = createTrustManagers(sslConfig.trustStorePaths.get)
+    }
+
+    sslContext.init(keyManagers, trustManagers, new SecureRandom())


The doc says the last argument can be left empty, in which case the default implementation is used. Why not leave it empty?

I wasn't aware that by passing null it would use the default implementation. I will refactor it

hmil · 2020-07-20T21:34:40Z

jvm/src/main/scala/fr/hmil/roshttp/SSLUtils.scala

+      val keyStore = KeyStore.getInstance(KEYSTORE_TYPE)
+      val keyStoreInputStream = Files.newInputStream(Paths.get(keyStorePath))
+
+      Objects.requireNonNull(keyStoreInputStream)


The documentation for newInputStream doesn't specify that this can be null. So I guess this means it cannot be null?

Ah yes indeed, very sharp! I just tested it and if the file cannot be found it will throw a NoSuchFileException
I thought it was similar to getClass.getClassLoader.getResourceAsStream() which will return a null when it cant find the file. I will drop the null check

hmil · 2020-07-20T21:35:48Z

jvm/src/main/scala/fr/hmil/roshttp/SSLUtils.scala

+      val keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm)
+      keyManagerFactory.init(keyStore, keyStorePassphrase)
+
+      closeSafely(keyStoreInputStream)


Same here for non-nullable

Yes, I will also refactor this one

hmil · 2020-07-20T21:47:37Z

I can fetch the path of the pfx with from the sslconfig within the backendConfig of the request

Just use fs.readFileSync. It's the easiest to get started.

I am realizing now that this solution re-creates the whole SSL config from disk for each request. It can be quite wasteful and can even become a performance bottleneck if someone uses the lib to make many requests. A better solution would be to pre-configure a client with the SSL config and then use that client to make requests.

But this is a long shot. All in all implementing proper support for SSL might take more than just one PR.

I would advise that you focus only on what you need for your specific use case. If you do not need Node.js, then in the JS backend just throw an exception when an ssl config is specified. We'll add to the limitations that SSL is only supported on the JVM. If you feel like adding it or if someone else needs this feature, then support for Node.js can be added in a following PR.

Hakky54 · 2020-07-20T22:03:58Z

I am realizing now that this solution re-creates the whole SSL config from disk for each request. It can be quite wasteful and can even become a performance bottleneck if someone uses the lib to make many requests. A better solution would be to pre-configure a client with the SSL config and then use that client to make requests.

Yes the best way would actually create the sslcontext once, reuse it multiple times for every new request. But I wasn't quite sure what would be a good implementation for that as every new request within the current implementation "recreates" the HttpConnection and executes a request with the given parameters.

But this is a long shot. All in all implementing proper support for SSL might take more than just one PR.

I would advise that you focus only on what you need for your specific use case. If you do not need Node.js, then in the JS backend just throw an exception when an ssl config is specified. We'll add to the limitations that SSL is only supported on the JVM. If you feel like adding it or if someone else needs this feature, then support for Node.js can be added in a following PR.

I actually don't feel very comfortable applying changes to the Node.js part as I have no experience yet with it. So I would gladly pass it to someone else who wants to implement it if you are also ok with it.

hmil · 2020-08-11T15:59:32Z

Hey, sorry I don't have any time at all to maintain this package right now. Feel free to publish your own version to bintray.

I refer to #58 to focus discussions around finding a package maintainer or a serious fork.

Hakky54 · 2020-08-15T11:06:15Z

Ah too bad to hear that, but I can understand that

Added jvm implementation of loading key and trust material

a0d3e37

Hakky54 added 2 commits July 19, 2020 15:09

Renamed variable

4d11a30

Added javadoc

9c37161

Hakky54 mentioned this pull request Jul 20, 2020

Feature Support for SSLContext #91

Closed

hmil reviewed Jul 20, 2020

View reviewed changes

Applied PR feedback

d39f1ee

Hakky54 closed this Dec 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Client side certificate and custom list of trusted certificates #92

Support for Client side certificate and custom list of trusted certificates #92

Hakky54 commented Jul 19, 2020 •

edited

Loading

Hakky54 commented Jul 19, 2020

hmil Jul 20, 2020

Hakky54 Jul 20, 2020 •

edited

Loading

hmil Jul 20, 2020

Hakky54 Jul 20, 2020

hmil Jul 20, 2020

Hakky54 Jul 20, 2020

hmil Jul 20, 2020

Hakky54 Jul 20, 2020

hmil commented Jul 20, 2020

Hakky54 commented Jul 20, 2020 •

edited

Loading

hmil commented Aug 11, 2020

Hakky54 commented Aug 15, 2020 •

edited

Loading

Support for Client side certificate and custom list of trusted certificates #92

Support for Client side certificate and custom list of trusted certificates #92

Conversation

Hakky54 commented Jul 19, 2020 • edited Loading

Hakky54 commented Jul 19, 2020

hmil Jul 20, 2020

Choose a reason for hiding this comment

Hakky54 Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

hmil Jul 20, 2020

Choose a reason for hiding this comment

Hakky54 Jul 20, 2020

Choose a reason for hiding this comment

hmil Jul 20, 2020

Choose a reason for hiding this comment

Hakky54 Jul 20, 2020

Choose a reason for hiding this comment

hmil Jul 20, 2020

Choose a reason for hiding this comment

Hakky54 Jul 20, 2020

Choose a reason for hiding this comment

hmil commented Jul 20, 2020

Hakky54 commented Jul 20, 2020 • edited Loading

hmil commented Aug 11, 2020

Hakky54 commented Aug 15, 2020 • edited Loading

Hakky54 commented Jul 19, 2020 •

edited

Loading

Hakky54 Jul 20, 2020 •

edited

Loading

Hakky54 commented Jul 20, 2020 •

edited

Loading

Hakky54 commented Aug 15, 2020 •

edited

Loading