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

Issue #5019 - hot-reload SSL certificates if keystore file changed #5042

Merged
merged 12 commits into from Jul 15, 2020

Conversation

lachlan-roberts
Copy link
Contributor

Issue #5019

Added an ssl-reload module which uses Scanner to monitor the keystore file registered with the SslContextFactory and reloads the keystore file if it detects any changes.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…tart.jar

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…nkins

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@sbordet
Copy link
Contributor

sbordet commented Jul 13, 2020

@lachlan-roberts please redo this PR as follows:

  • remove the Maven module jetty-ssl-reload as it is only 1 class
  • move the Jetty module jetty-ssl-reload.mod to Maven module jetty-server, where the other SSL Jetty modules are
  • class and test cases should be in jetty-util.
  • add a test where the keyStore file is a symlink and change the target of the symlink

Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

See PR comment.

…tty-server

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

  • remove the Maven module jetty-ssl-reload as it is only 1 class
  • move the Jetty module jetty-ssl-reload.mod to Maven module jetty-server, where the other SSL Jetty modules are
  • class and test cases should be in jetty-util.
  • add a test where the keyStore file is a symlink and change the target of the symlink

@sbordet I did all these, but moved the tests to test-integration as it needed a Server dependency. Maybe there is a better location for this.

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Some small changes.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Please add some documentation in configuring-ssl.adoc.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…l if reload failed

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Please add *.adoc documentation as well.

…extFactory

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts merged commit bbb0f66 into jetty-9.4.x Jul 15, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-9.4.x-5019-SslReload branch July 15, 2020 23:09
@lachlan-roberts lachlan-roberts added this to In progress in Jetty 9.4.31 via automation Jul 15, 2020
@mantryashutosh
Copy link

Does the hot reload work if the keystore file is a soft link ?

@joakime
Copy link
Contributor

joakime commented Sep 25, 2020

I wouldn't expect it to, as the soft link itself wasn't updated, meaning there's no file change (last modified) detectable.

@lachlan-roberts
Copy link
Contributor Author

@mantryashutosh it should still work if the keystore file is a soft link, we even have a test for this at KeyStoreScannerTest.testReloadChangingTargetOfSymbolicLink().

@mantryashutosh
Copy link

that's great, thank you so much @lachlan-roberts . Will test it out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Jetty 9.4.31
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants