[GitHub] [james-project] chibenwa commented on pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…

["chibenwa (via GitHub)" <gi...@apache.org>]

chibenwa commented on PR #1559:
URL: https://github.com/apache/james-project/pull/1559#issuecomment-1547154365

   >  ...I initially had that in mind and asked about dedicated module, to handle it, to which you replied with "Each protocol module can independently define the bindings without needing a centralized place at all..." hence I followed your guidance :-)
   
   Well, the above quoted statement is compatible with "one route to reload all certificates" and do not mean "one route per protocol" ;-)
   
   > TBH, in our downstream project I used that approach and simply rely on injected Set<AbstractServerFactory> to reload certificates for all servers/protocols :-)
   
   This, would look great to me, indeed!
   
   > I'll add it later on, though would be nice to clarify/confirm endpoints - do we want to have it directly or under `/server/?
   
   Is:
   
   ```
   POST /servers?reloadSSLCertificate
   ```
   
   Ok with you?
   
   Else what do you propose?
   
   > I would also say that we lack some tests. Did you think of a potential testing strategy for this changeset?
   
   `server/container/guice/common/src/main/java/org/apache/james/TemporaryJamesServer.java` defines a way to programmatically override configuration files, and thus test numerous configuration set up within the same maven project.
   
   The way it works is writing the desired configuration to a temporary file and use it upon application startup.
   
   We could easily...
   
    - 1. Have `keystore` and `keystore2` as test resources
    - 2. Have `smtpserver.xml` pointing to `<keystore>file://conf/keystore</keystore>` (default)
    - 3. Start james
    - 4. Manually modify the smtpserver.xml temporary file (bit tricky but doable) to use `<keystore>file://conf/keystore2</keystore>`
    - 5. relead the cetrificate
    - 6. Ensure that a SSL client is now presented with the newer certificate.
    
    The easiest way to achieve (4.) would be to enhance `TemporaryJamesServer` in order to handle renames upon copying a resource to its configuration folder...
    
    Eg:
    
    ```
       private void copyResources() {
           configurationFileNames
               .forEach(this::copyResource);
       }
   
       public void copyResource(String resourceName) {
           copyResource(resourceName, resourceName);
       }
   
       public void copyResource(String resourceName, String targetName) {
           var resolvedResource = Paths.get(configurationFolder.getAbsolutePath()).resolve(targetName);
           try (OutputStream outputStream = new FileOutputStream(resolvedResource.toFile())) {
               URL resource = ClassLoader.getSystemClassLoader().getResource(resourceName);
               if (resource != null) {
                   try (InputStream stream = resource.openStream()) {
                       stream.transferTo(outputStream);
                   }
               } else {
                   throw new RuntimeException("Failed to load configuration resource " + resourceName);
               }
           } catch (IOException e) {
               throw new RuntimeException(e);
           }
       }
    ```
    
    Then a test could look like:
    
    ```
    class SSLCertificateReloadTest {
   
       private static final List<String> BASE_CONFIGURATION_FILE_NAMES = ImmutableList.of("dnsservice.xml",
           "dnsservice.xml",
           "imapserver.xml",
           "jwt_publickey",
           "lmtpserver.xml",
           "mailetcontainer.xml",
           "mailrepositorystore.xml",
           "managesieveserver.xml",
           "pop3server.xml",
           "smtpserver.xml");
   
       private GuiceJamesServer jamesServer;
       private TemporaryJamesServer temporaryJamesServer;
   
       @BeforeEach
       void beforeEach(@TempDir Path workingPath) throws Exception {
           temporaryJamesServer = new TemporaryJamesServer(workingPath.toFile(), BASE_CONFIGURATION_FILE_NAMES);
           jamesServer = temporaryJamesServer.getJamesServer()
               .combineWith(IN_MEMORY_SERVER_AGGREGATE_MODULE)
               .combineWith(new UsersRepositoryModuleChooser(new MemoryUsersRepositoryModule())
                   .chooseModules(UsersRepositoryModuleChooser.Implementation.DEFAULT))
               .overrideWith(new JMXServerModule(),
                   binder -> binder.bind(ListeningMessageSearchIndex.class).toInstance(mock(ListeningMessageSearchIndex.class)));
   
   
       }
   
       @AfterEach
       void afterEach() {
           if (jamesServer != null && jamesServer.isStarted()) {
               jamesServer.stop();
           }
       }
   
       @Test
       void jamesCliShouldFailWhenNotGiveAuthCredential() throws Exception {
           // Start james with certificated within smtpserver.xml
           jamesServer.start();
   
           // change certificate
           temporaryJamesServer.copyResource("smtpserver-new-certificates.xml", "smtpserver.xml");
   
           // Reload certificates
           // XPOST /servers?reloadSSLCertificate
           
           // Open an SSL connection and check certificate
       }
   }
   ```
   
   (Possible test location: `memory-webadmin-integration-test` module?)
   
    We could also add a test ensuring reloading ssl certificate do not close existing connections.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@james.apache.org
For additional commands, e-mail: notifications-help@james.apache.org