Use temporary self signed certificates for JSON-RPC HTTP TLS unit testing #311

usmansaleem · 2020-01-18T09:15:40Z

PR description

-- Generate and use temporary self signed certificates in PKCS12 format with SAN extensions for dns and IP to be used by TLS unit/integration tests
-- Remove pre-generated key stores and trust stores used by TLS tests

Signed-off-by: Usman Saleem usman@usmans.info

-- Generate self signed certificates in PKCS12 format with SAN extensions for testing -- Remove pre-generated key stores and trust stores used by TLS tests Signed-off-by: Usman Saleem <usman@usmans.info>

Signed-off-by: Usman Saleem <usman@usmans.info>

gradle/versions.gradle

-- required by Tuweni TLS Signed-off-by: Usman Saleem <usman@usmans.info>

ethereum/api/build.gradle

...reum/api/src/test-support/java/org/hyperledger/besu/ethereum/api/tls/SelfSignedPfxStore.java

Signed-off-by: Usman Saleem <usman@usmans.info>

…Jar needs to be exported. Signed-off-by: Usman Saleem <usman@usmans.info>

Signed-off-by: Usman Saleem <usman@usmans.info>

CjHare

What was the motivation for locating this self signing code under ethereum/api rather then say crypto?

CjHare · 2020-01-22T06:22:18Z

...m/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/JsonRpcHttpServiceTlsTest.java


  @Before
  public void initServer() throws Exception {
+    selfSignedPfxStore = SelfSignedPfxStore.create();
+    selfSignedPfxStoreForHttpClient = SelfSignedPfxStore.create();
+    selfSignedPfxStoreForHttpClientNotTrustedByServer = SelfSignedPfxStore.create();


This seems odd, the same method of creation gives a PfxStore, PfxStoreForHttpClient and PfxStoreForHttpClientNotTrustedByServer.

If there are three different stores, then perhaps separate helper methods to create each of them and their corresponding state (e.g. that makes one trusted and another not trusted), might be an idea?

The utility class generate both keystore and truststore containing self-sligned certificate. The keystore is used by server/json-rpc and truststore is used by okhttp/client vice-versa is also applicable for testing client-auth. The keystore used by okhttp client, its truststore is used by server. Hence there is a need of having three different instances of self-signed certs. I can wrap them in a method for the sake of reaability, but all it will do is return instances which will again store in three different variables.

...ava/org/hyperledger/besu/ethereum/api/jsonrpc/JsonRpcHttpServiceTlsMisconfigurationTest.java

ethereum/api/src/test-support/java/org/hyperledger/besu/ethereum/api/tls/TrustStoreUtil.java

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem · 2020-01-28T09:13:01Z

What was the motivation for locating this self signing code under ethereum/api rather then say crypto?

@CjHare The test code (and the unit under testing) which required self signed certificates was in ethereum/api. I don't mind moving it under crypto/testSupport (I think it make more sense there)

-- Renaming class to clear intent -- Introducing builder in TlsHttpClient test utility -- cleaning unit tests Signed-off-by: Usman Saleem <usman@usmans.info>

Signed-off-by: Usman Saleem <usman@usmans.info>

…r to construct OkHttpClient Signed-off-by: Usman Saleem <usman@usmans.info>

Signed-off-by: Usman Saleem <usman@usmans.info>

rain-on · 2020-01-29T00:47:22Z

...eum/api/src/test-support/java/org/hyperledger/besu/ethereum/api/tls/KnownClientFileUtil.java

+      final String fingerprint = TLS.certificateHexFingerprint(certificate);
+      final String commonName = extractCommonName(certificate);
+      final String knownClientsLine = String.format("%s %s", commonName, fingerprint);
+      final Path temporaryKnownClientsFile = Files.createTempFile("testKnownClients", ".txt");


nit: I'd remove "test" - you never know if someone wants to make this a general capability in an app/library.
Having said that - having this create a tempFile, when you don't know the use of this function is kinda dangerous.
But am wondering if these two functions should effeictvely exist on the SelfSignedP12Certificate - such taht someone can say:
cert.getFingerPrint();
cert.getCommonName();
writeFile(fingerprint, commonName)

I've refactored it.

...pi/src/test-support/java/org/hyperledger/besu/ethereum/api/tls/SelfSignedP12Certificate.java

rain-on · 2020-01-29T00:51:25Z

...ava/org/hyperledger/besu/ethereum/api/jsonrpc/JsonRpcHttpServiceTlsMisconfigurationTest.java

  private static final NatService natService = new NatService(Optional.empty());
-
+  private final SelfSignedP12Certificate besuCertificate = SelfSignedP12Certificate.create();


nit: is besuCertificate correct here, or is this the certificate presented by the client?

besuCertificate contains certificate keystore used by Besu. It also exposes trustStore that can be used by OkHttp client library to trust Besu.

rain-on · 2020-01-29T00:52:39Z

...reum/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/TlsOkHttpClientBuilder.java

+import okhttp3.OkHttpClient;
+
+public final class TlsOkHttpClientBuilder {
+  private SelfSignedP12Certificate besuCertificate;


whats the difference between the besuCertificate and the okHttpCertificate?

In case of this builder class, we need to trust Besu certificate, okhttpcertificate is optional and required if besu enables client-auth ... OkHttpClient set its keystore exposed by okhttpcertificate.

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem added 4 commits January 18, 2020 19:13

SelfSignedPfxStore in testSupport for JSON-RPC HTTP TLS testing

ec4ce0b

-- Generate self signed certificates in PKCS12 format with SAN extensions for testing -- Remove pre-generated key stores and trust stores used by TLS tests Signed-off-by: Usman Saleem <usman@usmans.info>

spotless fix

17b364f

Signed-off-by: Usman Saleem <usman@usmans.info>

Removing pre-generated tls test resources

e84089c

Signed-off-by: Usman Saleem <usman@usmans.info>

Fixing common name extraction for known clients fingerprint file

0a2b9f7

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem changed the title ~~SelfSignedPfxStore in testSupport for JSON-RPC HTTP TLS testing~~ Use temporary self signed certificates for JSON-RPC HTTP TLS unit testing Jan 19, 2020

usmansaleem marked this pull request as ready for review January 19, 2020 00:48

usmansaleem requested review from shemnon and rain-on January 19, 2020 00:48

adding final keyword in try/resource block

38aba5d

Signed-off-by: Usman Saleem <usman@usmans.info>

rolfyone reviewed Jan 19, 2020

View reviewed changes

gradle/versions.gradle Outdated Show resolved Hide resolved

adding pcpkix to be available on runtime

5c37b01

-- required by Tuweni TLS Signed-off-by: Usman Saleem <usman@usmans.info>

rain-on reviewed Jan 20, 2020

View reviewed changes

usmansaleem added 3 commits January 22, 2020 12:39

Merge remote-tracking branch 'upstream/master' into rpc_tls_tests_certs

4ab15c2

Signed-off-by: Usman Saleem <usman@usmans.info>

Don't create testJar artifact as it is not required, only testSupport…

b948504

…Jar needs to be exported. Signed-off-by: Usman Saleem <usman@usmans.info>

Refactoring and simplifying SelfSignedPfxStore

f4b7c91

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem requested review from rain-on and CjHare January 22, 2020 03:38

Adding apache header

150ba1a

Signed-off-by: Usman Saleem <usman@usmans.info>

CjHare reviewed Jan 22, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rpc_tls_tests_certs

fc995ab

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem added 2 commits January 28, 2020 22:16

Refactoring after review suggestions

fe467ea

-- Renaming class to clear intent -- Introducing builder in TlsHttpClient test utility -- cleaning unit tests Signed-off-by: Usman Saleem <usman@usmans.info>

refactoring method name

b5bdb30

Signed-off-by: Usman Saleem <usman@usmans.info>

usmansaleem requested a review from CjHare January 28, 2020 12:20

usmansaleem added 2 commits January 28, 2020 22:37

Refactoring TlsOkHttpClientBuilder by eliminating an indirection laye…

f664efc

…r to construct OkHttpClient Signed-off-by: Usman Saleem <usman@usmans.info>

Merge remote-tracking branch 'upstream/master' into rpc_tls_tests_certs

db36e91

Signed-off-by: Usman Saleem <usman@usmans.info>

rain-on reviewed Jan 29, 2020

View reviewed changes

review suggestions - refactoring

b994dab

Signed-off-by: Usman Saleem <usman@usmans.info>

rain-on approved these changes Jan 29, 2020

View reviewed changes

usmansaleem merged commit 5b1ee14 into hyperledger:master Jan 29, 2020

usmansaleem deleted the rpc_tls_tests_certs branch January 29, 2020 02:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use temporary self signed certificates for JSON-RPC HTTP TLS unit testing #311

Use temporary self signed certificates for JSON-RPC HTTP TLS unit testing #311

usmansaleem commented Jan 18, 2020 •

edited

Loading

CjHare left a comment

CjHare Jan 22, 2020

usmansaleem Jan 28, 2020

usmansaleem commented Jan 28, 2020

rain-on Jan 29, 2020

usmansaleem Jan 29, 2020

rain-on Jan 29, 2020

usmansaleem Jan 29, 2020

rain-on Jan 29, 2020

usmansaleem Jan 29, 2020

		private static final NatService natService = new NatService(Optional.empty());

		private final SelfSignedP12Certificate besuCertificate = SelfSignedP12Certificate.create();

Use temporary self signed certificates for JSON-RPC HTTP TLS unit testing #311

Use temporary self signed certificates for JSON-RPC HTTP TLS unit testing #311

Conversation

usmansaleem commented Jan 18, 2020 • edited Loading

PR description

CjHare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usmansaleem commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

usmansaleem commented Jan 18, 2020 •

edited

Loading