Skip to content

Refactor CertUtils. Support ECDSA and PrivateKey.#529

Merged
marcuslinke merged 1 commit intodocker-java:masterfrom
KostyaSha:refactorKeys
Jun 7, 2016
Merged

Refactor CertUtils. Support ECDSA and PrivateKey.#529
marcuslinke merged 1 commit intodocker-java:masterfrom
KostyaSha:refactorKeys

Conversation

@KostyaSha
Copy link
Copy Markdown
Member


This change is Reviewable

@codecov-io
Copy link
Copy Markdown

Current coverage is 23.11%

Merging #529 into master will decrease coverage by -0.02% as of 64f1176

@@            master    #529   diff @@
======================================
  Files          295     295       
  Stmts         6102    6112    +10
  Branches       530     537     +7
  Methods          0       0       
======================================
+ Hit           1412    1413     +1
  Partial         83      83       
- Missed        4607    4616     +9

Review entire Coverage Diff as of 64f1176

Powered by Codecov. Updated on successful CI builds.

@KostyaSha
Copy link
Copy Markdown
Member Author

@marcuslinke could you review?

@marcuslinke
Copy link
Copy Markdown
Contributor

@KostyaSha LGTM 👍

@KostyaSha
Copy link
Copy Markdown
Member Author

@marcuslinke how do you handle certs? I have shell scripts that generates server/client certs, but they should be tied to some hostname/address. I used 192.168.99.100 for my docker-machine but it not ideal.
It should be possible generate certs during container build and then attach volume to DIND + fetch client file from this DND and then try execute connection. Would it be right way?

@marcuslinke
Copy link
Copy Markdown
Contributor

@KostyaSha Currently I manage IP/certs manually in my local ~/.docker-java.properties. So when IP of docker-machine VM changes I have to modify it. So it looks something like this currently:

DOCKER_HOST=tcp://192.168.99.102:2376
DOCKER_CERT_PATH=/Users/marcus/.docker/machine/machines/docker-1.10.2

#DOCKER_HOST=tcp://192.168.99.105:2376
#DOCKER_CERT_PATH=/Users/marcus/.docker/machine/machines/docker-1.8.1

#DOCKER_HOST=tcp://192.168.99.106:2376
#DOCKER_CERT_PATH=/Users/marcus/.docker/machine/machines/docker-1.9.1

However when working with DIND we need to generate certs that are tied to the current DOCKER_HOST ip, right? This could be done when starting the DIND container I think. After that we could copy them via https://docs.docker.com/engine/reference/api/docker_remote_api_v1.22/#get-an-archive-of-a-filesystem-resource-in-a-container to our local java env. WDYT?

@KostyaSha
Copy link
Copy Markdown
Member Author

First of all i found that openssl tool may differ, so we should run generation in reproducible env.
Host would be hostname from outer dockerClient connection (i like @JunitRule/@ExternalResource instead of abstract subclasses).

dind easily runs, but i don't remember about port in cert validability... I could tre reimplement my tests.

@KostyaSha
Copy link
Copy Markdown
Member Author

And this wouldn't work because netty uses sslcontext now?

@KostyaSha
Copy link
Copy Markdown
Member Author

Found, it wrapped now.. Maybe let's merge without tests? Too much time left...

@KostyaSha
Copy link
Copy Markdown
Member Author

@marcuslinke let's merge? This should fix user issues.

InvalidKeySpecException, IOException, CertificateException, KeyStoreException {
KeyPair keyPair = loadPrivateKey(dockerCertPath);
List<Certificate> privateCertificates = loadCertificates(dockerCertPath);
return createKeyStore("key.pem", "cert.pem");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dockerCertPath is ignored completely here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

mm...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Trying to fix locally

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Smells like i missed this wrapper when refactored methods to be reusable for non-file based usage.
Could you fix or should i?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i guess it should just load files into string, or pass streams and everything would work

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tried to load into strings but I ended with a javax.ws.rs.ProcessingException: javax.net.ssl.SSLHandshakeException: sun.security.validator.ValidatorException: PKIX path validation failed: java.security.cert.CertPathValidatorException: signature check failed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

maybe ca.cert is missing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

call to createTrustStore...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, had a typo there. Now it seems to work.

@marcuslinke marcuslinke merged commit 9639cb7 into docker-java:master Jun 7, 2016
@KostyaSha KostyaSha added this to the 3.0.1 milestone Jun 13, 2016
@KostyaSha KostyaSha self-assigned this Jun 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants