Skip to content
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

Merged
merged 1 commit into from
Oct 5, 2017

Conversation

lburgazzoli
Copy link
Collaborator

@lburgazzoli lburgazzoli commented Sep 29, 2017

fixes #162 #164

@@ -0,0 +1 @@
{"signing":{"default":{"expiry":"43800h","usages":["signing","key encipherment","server auth","client auth"]}}}
Copy link
Member

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
Copy link
Member

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

<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>
Copy link
Member

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.

Copy link
Collaborator Author

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.

@lburgazzoli
Copy link
Collaborator Author

Will rework the pr

@lburgazzoli
Copy link
Collaborator Author

@fanminshi anything more I need to change ?

@@ -0,0 +1 @@
cfssl/
Copy link
Member

Choose a reason for hiding this comment

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

not needed?

Copy link
Collaborator Author

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

Copy link
Member

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>
Copy link
Member

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?

Copy link
Collaborator Author

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");
Copy link
Member

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?

Copy link
Collaborator Author

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");
Copy link
Member

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")) {
Copy link
Member

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?

Copy link
Collaborator Author

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
Copy link
Member

Choose a reason for hiding this comment

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

why adding this?

Copy link
Collaborator Author

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 .*

@fanminshi
Copy link
Member

@lburgazzoli looks good in general. I just have few questions.

@lburgazzoli
Copy link
Collaborator Author

fixed

@fanminshi
Copy link
Member

lgtm

@fanminshi fanminshi closed this Oct 5, 2017
@fanminshi fanminshi reopened this Oct 5, 2017
@fanminshi
Copy link
Member

@lburgazzoli thanks for the fix! appreciated

@fanminshi
Copy link
Member

merge when green.

@fanminshi fanminshi merged commit 1906f20 into etcd-io:master Oct 5, 2017
@lburgazzoli lburgazzoli deleted the github-162-ssl branch October 6, 2017 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for ssl/tls
2 participants