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

Reloading SSL Configuration #4453

Closed
wants to merge 20 commits into from
Closed

Reloading SSL Configuration #4453

wants to merge 20 commits into from

Conversation

Hakky54
Copy link
Contributor

@Hakky54 Hakky54 commented Aug 9, 2022

I discovered that the community of Vert.x wants ssl reloading mechanism from the following 4 issues:

They all have different use-cases. Some need to reload the ssl configuration more frequently than others, but the main goal is to have some kind of reloading mechanism without the need of restarting the server or recreating the client. A kind developer, @tsaarni has even created this pull request which does the trick already: #4372 However it is limited to a KeyStore/PEM File. The Vert.x core library has two interfaces which needs to be supported: TrustOptions and KeyCertOptions which should return a KeyManager/TrustManager.

I was reading through all of the comments and noticed this one of @vietj and it seems like he prefers a solution outside of the core library:

ok, I would like to see if we can have it as something more decoupled from vertx core

I even added my input in this issue: #2870 (comment)
However there is no follow-up till this day although the library maintainer appreciated the solution:

thanks @Hakky54

I think we could provide in Vert.x an SPI to integrate Vertx to more easily integrate with SSLContext library

So based on the remarks of @vietj I thought it would be maybe a good to provide additional documentation to the project for this kind of use case. It seems like the maintainer does not want to have this in the core library, but want to provide it as a separate util library combining for other utilities which can be added in the future. I already have created a library couple of years ago which has this capability since last year, see here: GitHub - SSLCcontext Kickstart. I also created a reference implementation with a live demo to demonstrate how to configure it and demonstrate that it is working. So therefor I though to create a pull request to include code snippets as example ssl configuration to help the end-user of enabling this kind of feature. The core library won't get additional code complexity and also does not need to maintain it. If developers would like to enable this feature they can just add an additional library. Next to that by giving the control to the end-user he/she can decide when to update the ssl configuration, either via a file listener, scheduled on a fixed interval of minutes, hours, or maybe days, or based on triggers?

This pull request should close the following issues/pull requests:

@Hakky54
Copy link
Contributor Author

Hakky54 commented Oct 31, 2022

This PR has been updated. The initial PR contained only documentation to accomplish it with an external library, however the latest changes have now built in support for reloading the ssl configuration at runtime. The server ssl reloading can be enabled with a HttpServerOption setSSLRefreshOptions(TimeUnit.SECONDS, 10)

Below is an example server configuration:

Vertx vertx = Vertx.vertx();

Router router = Router.router(vertx);
router.route().handler(context -> context.json(
        new JsonObject().put("message", "Hello World!")
));

KeyCertOptions keyCertOptions = new PfxOptions()
        .setPath("/path/to/server-key.p12")
        .setPassword("secret");

TrustOptions trustOptions = new PfxOptions()
        .setPath("/path/to/server-trust.p12")
        .setPassword("secret");

HttpServerOptions serverOptions = new HttpServerOptions()
        .setSsl(true)
        .setSslRefreshOptions(TimeUnit.DAYS, 1)
        .setKeyCertOptions(keyCertOptions)
        .setTrustOptions(trustOptions);

HttpServer httpServer = vertx.createHttpServer(serverOptions);
httpServer.requestHandler(router)
        .listen(8443)
        .onSuccess(server -> System.out.println("HTTP server started on port " + server.actualPort()));

The reloading mechanism has been tested with:

  • PfxOptions
  • JksOptions
  • PemKeyCertOptions
  • PemTrustOptions

Any implementation of KeyCertOptions and TrustOptions which ovverides the method isUpdated and returns true will be eligible to use as a new ssl material for the server ssl configuration. To prevent any braking changes these interfaces have a default method which returns false, so anyone is not forced to implement the method if they don't want to and to prevent compilation issues.

@vietj can you maybe have another look at the changes whether you think this is in the right direction?

@Hakky54
Copy link
Contributor Author

Hakky54 commented Nov 8, 2022

Hi guys @vietj and @pmlopes ,

Any news regarding this topic and this PR?
What do you think of it, any adjustments needed maybe?

Looking forward to hear from you guys

@vietj
Copy link
Member

vietj commented Nov 23, 2022

I belive that instead of having reload options in the SSL configuration (for now), we should instead have a method to update the SSL configuration on HttpServer which gives more flexiblity and can easily integrated with a timer.

@Hakky54
Copy link
Contributor Author

Hakky54 commented Nov 23, 2022

Thank you for your feedback. I am trying to clarify things for myself to apply the correct changes you want. With my current changes the refresh rate will be set in the HttpServerOptions including the base and sub classes of it. You prefer this to be reverted and have a new method in the HttpServer which is able to reload the ssl configuration right? And in this way the end-user can reload it by calling that new method whenever they want. If that is the case, do you still want the mechanism of checking whether the ssl material (keystores, pem files) have been updated or should that be dropped?

@vietj
Copy link
Member

vietj commented Nov 23, 2022

I think we should only have a single method on HttpServer that computes a new SSL helper and replace the current one with the new one. This makes actionnable from outside and easy to integrate with a periodic or the event-bus.

@Hakky54
Copy link
Contributor Author

Hakky54 commented Nov 23, 2022

I tried replacing the SSLHelper at runtime, but it didn't work. The server is keeping the SSLContext/TrustManager/KeyManager in memory and therefor replacing the SSLHelper is not sufficient. I kept most of my changes and applied your feedback. I removed the refresh options from the HttpOptions. There is now a new method available within the server which is capable of reloading the ssl configuration. The usage with the current state of the code changes is:

Vertx vertx = Vertx.vertx();
Router router = ...; // initialized Router
KeyCertOptions keyCertOptions = ...; // initialized KeyCertOptions
TrustOptions trustOptions = ...; // initialized TrustOptions

HttpServerOptions serverOptions = new HttpServerOptions()
        .setSsl(true)
        .setKeyCertOptions(keyCertOptions)
        .setTrustOptions(trustOptions);

HttpServer httpServer = vertx.createHttpServer(serverOptions);
httpServer.requestHandler(router)
        .listen(8443)
        .onSuccess(server -> System.out.println("HTTP server started on port " + server.actualPort()));

// the method below will reread the keystores/pem files and apply it to the running server
httpServer.reloadSsl();

The httpServer.reloadSsl() method can be called like you preferred, so anyone can trigger it periodically or with an event bus. I kept the logic to check whether a file has been updated to prevent redundant reloading as there will be no need to reload the ssl configuration if the files have not been changed, what do you think of it or should I drop those validation? And do you have any other suggestions or remarks?

@Hakky54
Copy link
Contributor Author

Hakky54 commented Dec 9, 2022

@vietj ok to merge?

@vietj
Copy link
Member

vietj commented Dec 14, 2022

let me have al ook

/**
* Attempts to reload the ssl configuration if the server has been configured with ssl
*/
void reloadSsl();
Copy link
Member

Choose a reason for hiding this comment

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

that should be an asynchronous operation that might fail

Copy link
Contributor Author

@Hakky54 Hakky54 Dec 14, 2022

Choose a reason for hiding this comment

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

Let me refactor it to an async call. But you assume it might fail, how?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And... I am not sure how to make this operation async, can you share an example maybe? I found out I could use a Future, Promise, AsyncResult but I am having trouble understanding the best practice with it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I attempted to make the reloading of ssl material async and it works, next to that the tests also passes 😄 Can you have another look at the latest changes?

@vietj
Copy link
Member

vietj commented Dec 17, 2022

See #4568

@vietj vietj closed this Dec 17, 2022
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

Successfully merging this pull request may close these issues.

2 participants