You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by "woj-tek (via GitHub)" <gi...@apache.org> on 2023/05/11 21:36:53 UTC
[GitHub] [james-project] woj-tek opened a new pull request, #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
woj-tek opened a new pull request, #1559:
URL: https://github.com/apache/james-project/pull/1559
JAMES-3906 Add hot reloading/updating of the certificate:
* new interface with `reloadSSLCertificate()` method;
* new abstract class `AbstractServerRoutes` with implementation for all available/known servers;
* fix issue with `ManageSieveServerFactory` not being singleton causing issues.
--
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
[GitHub] [james-project] woj-tek commented on a diff in pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #1559:
URL: https://github.com/apache/james-project/pull/1559#discussion_r1192512444
##########
server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/webadmin/ImapRoutes.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.james.imapserver.webadmin;
Review Comment:
Licence header added
##########
server/protocols/protocols-library/src/main/java/org/apache/james/protocols/lib/netty/CertificateReloadable.java:
##########
@@ -0,0 +1,6 @@
+package org.apache.james.protocols.lib.netty;
Review Comment:
Licence header added
--
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
[GitHub] [james-project] chibenwa commented on pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #1559:
URL: https://github.com/apache/james-project/pull/1559#issuecomment-1562251286
Hello @woj-tek
I took the liberty today to shoot a PR with my suggestions.
I would encourage you taking a look at it.
--
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
[GitHub] [james-project] chibenwa commented on a diff in pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on code in PR #1559:
URL: https://github.com/apache/james-project/pull/1559#discussion_r1191828967
##########
server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/webadmin/ImapRoutes.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.james.imapserver.webadmin;
Review Comment:
You are missing the compulsary Apache V2 license file header
##########
server/protocols/protocols-library/src/main/java/org/apache/james/protocols/lib/netty/CertificateReloadable.java:
##########
@@ -0,0 +1,6 @@
+package org.apache.james.protocols.lib.netty;
Review Comment:
Idem
##########
server/protocols/protocols-library/pom.xml:
##########
@@ -52,6 +52,10 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>${james.groupId}</groupId>
+ <artifactId>james-server-webadmin-core</artifactId>
+ </dependency>
Review Comment:
I think this dependency is a bad idea: now I can no longer use IMAP without pulling webadmin.
I would prefer that we have a dedicated webadmin maven module for protocols.
--
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
[GitHub] [james-project] chibenwa closed pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa closed pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
URL: https://github.com/apache/james-project/pull/1559
--
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
[GitHub] [james-project] woj-tek commented on a diff in pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on code in PR #1559:
URL: https://github.com/apache/james-project/pull/1559#discussion_r1192821699
##########
server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/webadmin/ImapRoutes.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.james.imapserver.webadmin;
Review Comment:
@chibenwa btw licencing headers - it seems not all files have them (I just saw https://github.com/apache/james-project/blob/master/server/protocols/protocols-library/src/test/java/org/apache/james/protocols/lib/mock/MockProtocolHandlerLoader.java)... Maybe use https://github.com/mathieucarbou/license-maven-plugin for checking and formatting?
--
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
[GitHub] [james-project] woj-tek commented on pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on PR #1559:
URL: https://github.com/apache/james-project/pull/1559#issuecomment-1565096267
Hi @chibenwa , I wasn't able to make the changes earlier. Looks good! 👍
--
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
[GitHub] [james-project] chibenwa commented on pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "chibenwa (via GitHub)" <gi...@apache.org>.
chibenwa commented on PR #1559:
URL: https://github.com/apache/james-project/pull/1559#issuecomment-1571625307
Follow up PR is merged hence I close this.
--
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
[GitHub] [james-project] woj-tek commented on pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "woj-tek (via GitHub)" <gi...@apache.org>.
woj-tek commented on PR #1559:
URL: https://github.com/apache/james-project/pull/1559#issuecomment-1545962554
> We are missing documentations for the new webadmin endpoints:
I'll add it later on, though would be nice to clarify/confirm endpoints - do we want to have it directly or under `/server/<protocol>?
> I would also say that we lack some tests. Did you think of a potential testing strategy for this changeset?
I think the most crucial would be testing the reload itself. I see that in `protocols-library` there are already tests using resources keystore - one possibility would be to have second keystore, update `MemoryFileSystem` with the new one, initiate certificate reload and check the result. Another opportunity would be generating certificates on the fly, but it's more difficult with newer JDK versions it's difficult.
Testing whole path (rest->server->protocol connection) could be slightly more problematic as it would require certain duplication to cover all protocols (and I'm not sure that it's needed).
> Also I am unsure the feature of "reloading SSL" triggered per protocol is truely useful.
It could be (possiby having different certificates for different protocols) but I would say it's the edge case, however...
> I would rather propose a single endpoint that triggers the reload on all relevant servers: something like `POST /servers?reloadSSLCertificate`. This can be achieved with a multibinder on `AbstractServerFactory`.
>
> Finally keeping some orthogonality across components, and refraining from adding the webadmin dependency to the protcol-library, seems necessary to me. I am ready to provide help if need be to help sort out this issue.
...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 :-)
TBH, in our downstream project I used that approach and simply rely on injected `Set<AbstractServerFactory>` to reload certificates for all servers/protocols :-)
--
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
[GitHub] [james-project] chibenwa commented on pull request #1559: JAMES-3906 Add hot reloading/updating of the certificate: new interfa…
Posted by "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