-
Notifications
You must be signed in to change notification settings - Fork 316
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
test(ssl): add tests for ssl/tls #162 #245
Conversation
etc/cert/ca-config.json
Outdated
@@ -0,0 +1 @@ | |||
{"signing":{"default":{"expiry":"43800h","usages":["signing","key encipherment","server auth","client auth"]}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to have cert/* to be under test/resources/cert? since it is part of test?
@@ -0,0 +1,42 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best if it can be under test/resources/folder
jetcd-core/pom.xml
Outdated
<systemPropertyVariables> | ||
<ssl.cert.path>${project.basedir}/../etc/cert</ssl.cert.path> | ||
<ssl.cert.authority>etcd-ssl</ssl.cert.authority> | ||
<ssl.cert.endpoints>https://127.0.0.1:42379</ssl.cert.endpoints> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand what this part is doing. could you elaborate?
also I am not sure of the official pom.xml should have configs that'sspecific to ssl tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are here to avoid hard coding in tests, you may have your own instance and certs you want to use but that can be improved.
Will rework the pr |
49f79f7
to
769f51f
Compare
@fanminshi anything more I need to change ? |
@@ -0,0 +1 @@ | |||
cfssl/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed only to generate the certs and it is created by the script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
pom.xml
Outdated
@@ -319,6 +319,13 @@ | |||
<arg>-Xlint:all</arg> | |||
<!-- TODO <arg>-Werror</arg> --> | |||
</compilerArgs> | |||
<annotationProcessorPaths> | |||
<annotationProcessorPath> | |||
<groupId>com.google.auto.service</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is autoservice being used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to move it on jetcd-core but it does not hurt :)
public void testSimpleSllSetup() throws Exception { | ||
final ByteSequence key = ByteSequence.fromString(TestUtil.randomString()); | ||
final ByteSequence val = ByteSequence.fromString(TestUtil.randomString()); | ||
final String capath = System.getProperty("ssl.cert.capath"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we not using property file anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have properties so you can test against your own secured etcd instance setting properties
final ByteSequence val = ByteSequence.fromString(TestUtil.randomString()); | ||
final String capath = System.getProperty("ssl.cert.capath"); | ||
final String authority = System.getProperty("ssl.cert.authority", "etcd-ssl"); | ||
final String endpoints = System.getProperty("ssl.cert.endpoints", "https://127.0.0.1:42379"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe have a test constant for ssl endpoint?
|
||
try (InputStream is = Objects.nonNull(capath) | ||
? new FileInputStream(new File(capath)) | ||
: getClass().getResourceAsStream("/ssl/cert/ca.pem")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are not using property to get the capth anymore. maybe just getClass().getResourceAsStream("/ssl/cert/ca.pem")
? will be enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to be able to test against my own etcd setup with my own ca, so this is still needed
@@ -1,5 +1,6 @@ | |||
.* | |||
!.gitignore | |||
!.travis.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why adding this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because travis.yml should not be ignored :) but it is by default because of .*
@lburgazzoli looks good in general. I just have few questions. |
769f51f
to
2296d59
Compare
fixed |
lgtm |
@lburgazzoli thanks for the fix! appreciated |
merge when green. |
fixes #162 #164