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

Keystore password is never set, configured password is used as Keymanager password only #807

Closed
schoeneu opened this issue Nov 14, 2017 · 11 comments

Comments

@schoeneu
Copy link

The current configuration seems to use the password provided to wiremock by the .keyStorePassword() builder method as a password for the keymanager (protection of the individual entries), but does not provide a way to specify a password for the store itself.

See:

https://github.com/tomakehurst/wiremock/blob/adf81e2caa681d4d10e2d193ed56f7e5b46c2a30/src/main/java/com/github/tomakehurst/wiremock/jetty9/JettyHttpServer.java#L205-L211

In particular, sslContextFactory.setKeyStorePassword() is never called.

@schoeneu
Copy link
Author

I think the sane strategy would be to just introduce another config option that maps to the keymanager password and use the keystore PW from the current wiremock opts as, well, the keystore password.

However, this breaks things for existing users with non-PW-protected keystores who may be relying on keyStorePassword actually setting the keymanager password. What do you think?

@tomakehurst
Copy link
Member

Hmmm...yeah, this is a bit confusing at the moment.

The only way I can think of to solve this while preserving backwards compatibility is to introduce new parameters for both, then add some conditional logic for setting them in JettyHttpServer.

@schoeneu
Copy link
Author

@tomakehurst Suggestion: keyStoreStorePassword and keyStoreKeyPassword, with a redirection that maps the current keyStorePassword to keyStoreKeyPassword ?

It doesn't sound super convincing to me, but I can't really think of anything else that would look consistent. (Apart from breaking the current API, but that is likely to make people users more annoyed than a slightly confusing naming scheme.)

@tomakehurst
Copy link
Member

How about tlsKeyStorePassword and tlsKeyStoreManagerPassword?

Then mark keyStorePassword as deprecated with a link to the new ones?

@schoeneu
Copy link
Author

I had another look at this over the weekend, and it seems that no current keystore spec actually uses the store password for protection, only for integrity checks.

Since checking the integrity of a keystore is likely not a feature needed by wiremock users, I think it would also be an option to just add a note about this (along the lines of "provide the password protecting your key entries here, not the store password") to the HTTPS docs, which would be far less intrusive and introduce no clutter into the existing codebase.

I'm happy to create a PR for either solution (new config & options, or just note in the docs) - which one would you prefer?

@tomakehurst
Copy link
Member

Hey, if there's a way to fix this without adding more code, I'm all for it!

@mdaley
Copy link

mdaley commented May 31, 2018

Hello, I've just come across exactly this problem doing some work for a client and I had to use the solution mentioned in issue #874. It worked like a treat! I'd like to propose that it would be very useful to fix this in the code. Like wdekker says in the issue PKCS12 is becoming used more and it would be very useful to have a fix.

@tomakehurst
Copy link
Member

Do you feel like creating a PR?

In all honesty I'm unlikely to get to this for months otherwise.

@mdaley
Copy link

mdaley commented Jun 3, 2018

Hi Tom, yes, I'll have a go at that soon. Maybe next week if I have time. I certainly understand the lack of time and I must say that wiremock itself is really useful!

@Jacuo
Copy link

Jacuo commented Apr 16, 2020

Any news about issue ? I have spent a lot of time investigating problem caused by this issue ....

@mdaley
Copy link

mdaley commented Apr 17, 2020

Hi jacuo, I never got round to doing anything about this and it's such a long time ago that I can't remember anything about how I was going to solve it :-(. I think that I just used the workaround that was mentioned in #874.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants