Make Neo4j transport encryption configurable #6265
Conversation
| */ | ||
| @ConfigItem | ||
| @ConfigDocSection | ||
| public Config config; |
There was a problem hiding this comment.
You can name it DriverSettingsConfig and name the property driverSettings. Or maybe just driver (we are in the config anyway so you clearly expect settings).
It's the name of the property that is taken as the name of the config key. So you would have quarkus.neo4j.driver-settings.encrypted for instance.
There was a problem hiding this comment.
I think I than would rather prefer pulling that setting up into top level. I don't expect an object mapper for Neo4j in Quarkus anytime soon. Wdyt?
There was a problem hiding this comment.
That was my first thought when I saw the patch so I think it will be good enough.
| static class Config { | ||
|
|
||
| /** | ||
| * Flag, if the driver should use encrypted traffic. |
There was a problem hiding this comment.
| * Flag, if the driver should use encrypted traffic. | |
| * If the driver should use encrypted traffic. |
michael-simons
force-pushed
the
feature/make_neo4j_encryption_configurable
branch
2 times, most recently
from
446872c to
ae8cb3f
Compare
|
I have implemented the suggestion and added further settings, which I think will be necessary. Thanks for looking into this. |
|
Ping. |
extensions/neo4j/runtime/src/main/java/io/quarkus/neo4j/runtime/Neo4jConfiguration.java
Outdated
Show resolved
Hide resolved
| * Configures which trust strategy to apply when using encrypted traffic. | ||
| */ | ||
| @ConfigItem(defaultValue = "TRUST_SYSTEM_CA_SIGNED_CERTIFICATES") | ||
| public Strategy strategy = Strategy.TRUST_SYSTEM_CA_SIGNED_CERTIFICATES; |
There was a problem hiding this comment.
| public Strategy strategy = Strategy.TRUST_SYSTEM_CA_SIGNED_CERTIFICATES; | |
| public Strategy strategy; |
There was a problem hiding this comment.
Oh I just saw that there is test defaultsShouldWork() so maybe let's keep it the way it was.
extensions/neo4j/runtime/src/main/java/io/quarkus/neo4j/runtime/Neo4jConfiguration.java
Outdated
Show resolved
Hide resolved
extensions/neo4j/runtime/src/main/java/io/quarkus/neo4j/runtime/Neo4jDriverRecorder.java
Outdated
Show resolved
Hide resolved
extensions/neo4j/runtime/src/test/java/io/quarkus/neo4j/runtime/Neo4jConfigurationTest.java
Outdated
Show resolved
Hide resolved
michael-simons
force-pushed
the
feature/make_neo4j_encryption_configurable
branch
from
ae8cb3f to
5d6d679
Compare
|
Thanks @machi1990 for your kind review. I adapted to not using default values in the class itself (I'm used to the way of Boot, need to switch here when working on Quarkus) and also rebased onto the current master. |
Thanks for the contribution. I do not know much about neo4j, @michael-simons you are the expert here but LGTM otherwise. @gsmet WDYT? |
|
The change will allow connecting to Neo4j server using encrypted connection without manually providing the driver bean. I plan to do a Quarkus example connecting to our cloud offering with it, so yes, I think it fulfills this goal. |
|
Ping? |
1 similar comment
|
Ping? |
|
@michael-simons just a heads up that I'm back to addressing the huge backlog of PRs so you will have some news soon. Sorry about the delay. Things have been a bit crazy until now! |
gastaldi
added
the
triage/needs-rebase
|
@michael-simons mind rebasing this PR and squashing the commits? Looks good to me |
|
I’d love to. Will do next Monday (away from keyboard and things) |
michael-simons
force-pushed
the
feature/make_neo4j_encryption_configurable
branch
from
5d6d679 to
388afea
Compare
This adds the encrypted flag to the top level configuration and a new group that makes it possible to change the general trust settings. All settings have the same default as the underlying driver. Polishing.
michael-simons
force-pushed
the
feature/make_neo4j_encryption_configurable
branch
from
388afea to
67f6073
Compare
|
Rebased, squashed and pushed…. Thanks for reviving this. I noticed that the configuration has been polished. While I of course accept Quarkus' way of doing it, I do think it's easier to read the explicitly stated defaults, even for scalar values (related: https://twitter.com/rotnroll666/status/1288009552777097216) Thanks again! |
gastaldi
removed
the
triage/needs-rebase
gsmet
changed the title
|
Sorry it took me so long to review it and merge it! Thanks! |
This makes the encryption-option of the driver configurable (at least when not in native image mode). I chose the name
Configfor the class to align the naming with the integrations with other platforms. In case you can tell me how to have another class name map toconfigin the properties, than I would rather nameDriverSettings(analogue to https://github.com/neo4j/neo4j-java-driver-spring-boot-starter/blob/master/neo4j-java-driver-spring-boot-autoconfigure/src/main/java/org/neo4j/driver/springframework/boot/autoconfigure/Neo4jDriverProperties.java#L287).