Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Client TLS enabled #319

Merged
merged 15 commits into from
Jan 20, 2020
Merged

Client TLS enabled #319

merged 15 commits into from
Jan 20, 2020

Conversation

rain-on
Copy link
Contributor

@rain-on rain-on commented Jan 10, 2020

Orion is now able to accept TLS connections from a client (web3 provider).

Orion is now able to accept TLS connections from a client (web3 provider).
+ "\n"
+ " - strict: All connections to the client connection of this node must use TLS with client authentication\n"
+ " See the documentation 'clientconnectiontlsservertrust'\n"
+ " - off: Mutually authenticated TLS is not used for in- and outbound\n"

Choose a reason for hiding this comment

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

given this is what people are actually reading, i'd suggest changing 'in-' to 'inbound'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wanted to not change as much as possible - so have literally cut-and-paste existing descriptions.

Choose a reason for hiding this comment

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

fair enough, i thought this was 'new'.. all good

schemaBuilder.addString(
"clientconnectiontlsservertrust",
"tofu",
"TLS trust mode for the internal server endpoint. This decides who's allowed to connect to it. Options:\n"

Choose a reason for hiding this comment

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

break options to new line to make it easier to see when scanning down the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I understand - example?

Choose a reason for hiding this comment

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

...connect to it. Options:\n
vs
connect to it.\nOptions:\n

So that in the usage, its

TLS trust mode for the internal server endpoint. This decides who's allowed to connect to it.
Options:

  • blah...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhh - going to be difficult and say taht Given this was cut-and-paste, don't want to change all instances.

"ca-or-whitelist",
"tofu",
"insecure-tofa",
"ca-or-tofu",

Choose a reason for hiding this comment

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

this, for one, isn't documented above... is the intent to provide usage on all of these options? It'll be hard for people to know they're there if they're not documented...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to update them ... not sure I was successful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it turns out this is probably a bigger issue.
I believe TOFU should be used for clients, TOFA is for servers.
Really - the Valid cases for a server param shouldn't have TOFU in them at all.... but that's where this becomes bigger.
Generally - Orion needs a sweep out - don't want to do it here.

src/main/java/net/consensys/orion/cmd/Orion.java Outdated Show resolved Hide resolved
src/main/java/net/consensys/orion/cmd/Orion.java Outdated Show resolved Hide resolved
src/main/java/net/consensys/orion/cmd/Orion.java Outdated Show resolved Hide resolved
src/main/java/net/consensys/orion/cmd/Orion.java Outdated Show resolved Hide resolved
src/main/java/net/consensys/orion/cmd/Orion.java Outdated Show resolved Hide resolved
src/main/java/net/consensys/orion/cmd/Orion.java Outdated Show resolved Hide resolved
src/main/java/net/consensys/orion/cmd/Orion.java Outdated Show resolved Hide resolved
+ " will be allowed only if it's the first certificate this node has seen for that host.\n";

*/

Copy link
Member

Choose a reason for hiding this comment

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

NIT: remove the blank line

lucassaldanha
lucassaldanha previously approved these changes Jan 15, 2020
Copy link
Member

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

LGTM

assertThrows(SSLHandshakeException.class, () -> upcheckFailsUsingNonTlsClient(config.clientPort()));
}

private void upcheckFailsUsingNonTlsClient(final int orionPort) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: captialise the c in upCheck

@rain-on rain-on merged commit 3cecaf6 into Consensys:master Jan 20, 2020
@rain-on rain-on deleted the client_tls branch January 20, 2020 05:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants