You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@james.apache.org by GitBox <gi...@apache.org> on 2021/11/16 09:59:23 UTC
[GitHub] [james-project] ottoka opened a new pull request #750: JAMES-3672 : Support TLS authentication via client certificate
ottoka opened a new pull request #750:
URL: https://github.com/apache/james-project/pull/750
In order to limit access to trusted partners/users only, James should support TLS with certificate-based client authentication.
For this purpose, TLS configuration is extended with the desired authentication mode (none, optional, required), and the associated trust store to validate any received client certificates. Example:
```
<tls socketTLS="true" startTLS="false">
...
<clientAuth required="true">
<truststore>file://conf/truststore</truststore>
<truststoreType>JKS</truststoreType>
<truststoreSecret>yoursecret</truststoreSecret>
</clientAuth>
</tls>
```
This is implemented mostly in `AbstractConfigurableAsyncServer` and associated Netty infrastructure.
T-Shirt size M.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751082468
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/AbstractSSLAwareChannelPipelineFactory.java
##########
@@ -64,6 +66,12 @@ public ChannelPipeline getPipeline() throws Exception {
if (enabledCipherSuites != null && enabledCipherSuites.length > 0) {
engine.setEnabledCipherSuites(enabledCipherSuites);
}
+ if (Boolean.TRUE.equals(clientAuth)) {
+ engine.setNeedClientAuth(true);
+ }
+ if (Boolean.FALSE.equals(clientAuth)) {
+ engine.setWantClientAuth(true);
+ }
Review comment:
Actually this is a tri-state logic: null=none=no client auth, false=want=optional client auth, true=need=mandatory client auth. So clientAuth can be null here, and in that case we must not call either of the engine.setXXXClientAuth(true).
Admittedly this is a somewhat ugly API in the JDK, there is no good way around it. I can rename clientAuth to clientAuthRequired to maybe make things a bit clearer.
--
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] ottoka edited a comment on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka edited a comment on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-971434219
> Question: Is client authentication also applied upon STARTTLS or is it omly applies to plain TLS?
The point is that the TLS negotiation will fail and prevent the connection if the client provides an unknown/invalid certificate, or none at all (in need mode, required=true). So you could use it with StartTLS as well if that is what you want to happen. Admittedly it makes more sense in a private mail network where you can use straight SMTPS on port 465 from the start.
(There is no good use right now for the optional client auth cases, i.e. required=false or skipping StartTLS. Maybe a future extension could somehow make the client certificate chain available to matchers and mailets, so they can e.g. allow relaying only on connections with trusted client certificates.)
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754069352
##########
File path: protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
##########
@@ -29,15 +29,17 @@
private final SSLContext context;
private final boolean starttls;
private final String[] enabledCipherSuites;
+ private Boolean clientAuth;
- private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites) {
+ private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites, Boolean clientAuth) {
Review comment:
Even with too states enums brings in strong typing thus better express the intent and should rather be used.
(Refs: Clean code book, chapter I don't remember but clearly boolean arguments should be discouraged.)
--
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 #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-971558332
> @chibenwa Do you have any insights on the algorithm and provider properties I mentioned above?
Yes and I forgot to reply on this.
This sounds like legacy to me, and I am in favor of removing the two options from the documentation.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754287030
##########
File path: server/apps/distributed-app/docs/modules/ROOT/pages/configure/ssl.adoc
##########
@@ -84,6 +84,32 @@ Please note `JKS` keystore format is also supported (default value if no keystor
</tls>
....
+
+=== Client authentication via certificates
+
+When you enable TLS, you may also configure the server to request a client certificate for authentication:
+
+....
+<tls socketTLS="false" startTLS="true">
+ <keystore>file://conf/keystore</keystore>
+ <keystoreType>JKS</keystoreType>
+ <secret>yoursecret</secret>
+
+ <clientAuth required="true">
+ <truststore>file://conf/truststore</truststore>
+ <truststoreType>JKS</truststoreType>
+ <truststoreSecret>yoursecret</truststoreSecret>
+ </clientAuth>
+</tls>
+....
+
+With `required="true"`, the server refuses any client connections that do not include a client certificate.
+
+With `required="false"`, the servers requests an optional client certificate, but also accepts connections without one.
Review comment:
Fixed in latest push. I decided to keep the three states in an enum because that's how TLS works, but removed the option for WANT from the configuration. Now its either NEED with `<clientAuth>...</clientAuth>` or NONE without this XML element.
--
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] Arsnael commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
Arsnael commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751889911
##########
File path: server/protocols/protocols-library/src/test/resources/testServerDefaults.xml
##########
@@ -0,0 +1,2 @@
+<testerver enabled="true">
Review comment:
```suggestion
<testserver enabled="true">
```
Idem for all the other xml files :)
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751938097
##########
File path: server/protocols/protocols-library/src/test/resources/testServerDefaults.xml
##########
@@ -0,0 +1,2 @@
+<testerver enabled="true">
Review comment:
Sigh, copy and paste strikes again. Thanks for spotting 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] ottoka edited a comment on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka edited a comment on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-970111530
BTW: When I created the unit test for this, I noticed that apparently there is no code that uses the `algorithm` and `provider` settings. Did I miss something? Are the documentation and samples outdated? Did the relevant code get lost at some point? I am confused. Could someone more experienced with the James code base have a look please?
--
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 #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-971537574
`Maybe a future extension could somehow make the client certificate chain available to matchers and mailets, so they can e.g. allow relaying only on connections with trusted client certificates.`
Yes sure, carrying information from the protocols to the mailet context can easily be done via mail attributes if needed.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751938097
##########
File path: server/protocols/protocols-library/src/test/resources/testServerDefaults.xml
##########
@@ -0,0 +1,2 @@
+<testerver enabled="true">
Review comment:
Sigh, copy and paste strikes again. Thanks for spotting this. Fixed in latest push.
--
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] Arsnael merged pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
Arsnael merged pull request #750:
URL: https://github.com/apache/james-project/pull/750
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r753983837
##########
File path: protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
##########
@@ -29,15 +29,17 @@
private final SSLContext context;
private final boolean starttls;
private final String[] enabledCipherSuites;
+ private Boolean clientAuth;
- private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites) {
+ private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites, Boolean clientAuth) {
Review comment:
Maybe an enum would be better than a Boolean.
Especially an enum would enable to explicit the usage of `null` that is unclear to me.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754287030
##########
File path: server/apps/distributed-app/docs/modules/ROOT/pages/configure/ssl.adoc
##########
@@ -84,6 +84,32 @@ Please note `JKS` keystore format is also supported (default value if no keystor
</tls>
....
+
+=== Client authentication via certificates
+
+When you enable TLS, you may also configure the server to request a client certificate for authentication:
+
+....
+<tls socketTLS="false" startTLS="true">
+ <keystore>file://conf/keystore</keystore>
+ <keystoreType>JKS</keystoreType>
+ <secret>yoursecret</secret>
+
+ <clientAuth required="true">
+ <truststore>file://conf/truststore</truststore>
+ <truststoreType>JKS</truststoreType>
+ <truststoreSecret>yoursecret</truststoreSecret>
+ </clientAuth>
+</tls>
+....
+
+With `required="true"`, the server refuses any client connections that do not include a client certificate.
+
+With `required="false"`, the servers requests an optional client certificate, but also accepts connections without one.
Review comment:
Fixed in latest push. I decided to keep the three states in an enum because that's how TLS works, but removed the option for WANT from the configuration. Now its either NEED with `<clientAuth>...</clientAuth>` ot NONE without this XML element.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754285597
##########
File path: protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
##########
@@ -29,15 +29,17 @@
private final SSLContext context;
private final boolean starttls;
private final String[] enabledCipherSuites;
+ private Boolean clientAuth;
- private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites) {
+ private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites, Boolean clientAuth) {
Review comment:
Fixed in latest push.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754048931
##########
File path: server/apps/distributed-app/docs/modules/ROOT/pages/configure/ssl.adoc
##########
@@ -84,6 +84,32 @@ Please note `JKS` keystore format is also supported (default value if no keystor
</tls>
....
+
+=== Client authentication via certificates
+
+When you enable TLS, you may also configure the server to request a client certificate for authentication:
+
+....
+<tls socketTLS="false" startTLS="true">
+ <keystore>file://conf/keystore</keystore>
+ <keystoreType>JKS</keystoreType>
+ <secret>yoursecret</secret>
+
+ <clientAuth required="true">
+ <truststore>file://conf/truststore</truststore>
+ <truststoreType>JKS</truststoreType>
+ <truststoreSecret>yoursecret</truststoreSecret>
+ </clientAuth>
+</tls>
+....
+
+With `required="true"`, the server refuses any client connections that do not include a client certificate.
+
+With `required="false"`, the servers requests an optional client certificate, but also accepts connections without one.
Review comment:
It is one of the options you have in the TLS handshake protocol. The idea is that servers could allow different levels of access whether they decided to authenticate or not.
Right now this does not make much sense in James. But someone could implement a way to access the client certificate (if any) from the protocol session, e.g. to implement SASL EXTERNAL authentication mode. Or even from mailets to do whatever.
Now that I think about it, it is probably easier to just remove the option for now; if someone needs it later they can easily add it again.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751082871
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/BasicChannelUpstreamHandler.java
##########
@@ -215,6 +215,14 @@ protected ProtocolSession createSession(ChannelHandlerContext ctx) throws Except
if (enabledCipherSuites != null && enabledCipherSuites.length > 0) {
engine.setEnabledCipherSuites(enabledCipherSuites);
}
+ Boolean clientAuth = secure.getClientAuth();
+ if (Boolean.TRUE.equals(clientAuth)) {
+ engine.setNeedClientAuth(true);
+ }
+ if (Boolean.FALSE.equals(clientAuth)) {
+ engine.setWantClientAuth(true);
+ }
Review comment:
see above re tri-state logic
--
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] ottoka commented on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-979399677
In the latest push I went with the second option. `Encryption` now has a method `createSSLEngine()` which applies the settings, and the object is passed through the protocol/implementation stack to wherever an engine needs to be created. This avoids the temporal coupling anti-pattern you mentioned. As a nice benefit it also should make things easier in case someone needs more settings to apply to the resulting SSLEngine.
--
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 #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-975626728
> No problem, I hope you enjoyed your vacation :-)
A lot!
(Sorry still picky on this one, Netty layer is a bit legacy, thus changes there are not the easiest one to conduct out!)
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751091170
##########
File path: src/site/xdoc/server/config-ssl-tls.xml
##########
@@ -109,6 +109,29 @@
</tls>
</source>
+ <p>When you enable TLS, you may also configure the server to request a client certificate for authentication:</p>
Review comment:
sure thing.
--
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] ottoka commented on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-971584558
OK, I will remove the comment from the unit test.
What about the documentation fix, seems to me like this should be a separate issue/PR? Should I create one, or will you do 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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754379970
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/BasicChannelUpstreamHandler.java
##########
@@ -215,6 +216,14 @@ protected ProtocolSession createSession(ChannelHandlerContext ctx) throws Except
if (enabledCipherSuites != null && enabledCipherSuites.length > 0) {
engine.setEnabledCipherSuites(enabledCipherSuites);
}
+ ClientAuth clientAuth = secure.getClientAuth();
+ if (ClientAuth.NEED.equals(clientAuth)) {
+ engine.setNeedClientAuth(true);
+ }
+ if (ClientAuth.WANT.equals(clientAuth)) {
+ engine.setWantClientAuth(true);
+ }
+ LOGGER.info("start SMTP with TLS client auth need {} want {}", engine.getNeedClientAuth(), engine.getWantClientAuth());
Review comment:
Right, and it was supposed to be debug.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754386559
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/NettyServer.java
##########
@@ -122,6 +122,7 @@ protected ChannelPipelineFactory createPipelineFactory(ChannelGroup group) {
maxCurConnectionsPerIP,
group,
secure != null ? secure.getEnabledCipherSuites() : null,
+ secure != null ? secure.getClientAuth() : null,
Review comment:
Right, just did it this way to mimic the line above.
--
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] ottoka commented on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-975532717
No problem, I hope you enjoyed your vacation :-)
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751048756
##########
File path: server/protocols/protocols-library/src/test/resources/testServerPlain.xml
##########
@@ -0,0 +1,9 @@
+<testerver enabled="true">
+ <jmxName>testserver-custom</jmxName>
+ <bind>0.0.0.0:25</bind>
Review comment:
```suggestion
<bind>0.0.0.0:0</bind>
```
Bonding a non fixed port is cleaner as it ensures the build can run several time concurrently on the same computer.
Port can then be retrieved programmatically in the tests.
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/AbstractSSLAwareChannelPipelineFactory.java
##########
@@ -64,6 +66,12 @@ public ChannelPipeline getPipeline() throws Exception {
if (enabledCipherSuites != null && enabledCipherSuites.length > 0) {
engine.setEnabledCipherSuites(enabledCipherSuites);
}
+ if (Boolean.TRUE.equals(clientAuth)) {
+ engine.setNeedClientAuth(true);
+ }
+ if (Boolean.FALSE.equals(clientAuth)) {
+ engine.setWantClientAuth(true);
+ }
Review comment:
```suggestion
if (clientAuth) {
engine.setNeedClientAuth(true);
} else {
engine.setWantClientAuth(true);
}
```
Looks simpler?
##########
File path: server/apps/distributed-app/docs/modules/ROOT/pages/configure/ssl.adoc
##########
@@ -84,6 +84,29 @@ Please note `JKS` keystore format is also supported (default value if no keystor
</tls>
....
+When you enable TLS, you may also configure the server to request a client certificate for authentication:
Review comment:
```suggestion
=== Client authentication via certificates
When you enable TLS, you may also configure the server to request a client certificate for authentication:
```
We might like to have a dedicated (sub) section dedicated to client authentication...
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/BasicChannelUpstreamHandler.java
##########
@@ -215,6 +215,14 @@ protected ProtocolSession createSession(ChannelHandlerContext ctx) throws Except
if (enabledCipherSuites != null && enabledCipherSuites.length > 0) {
engine.setEnabledCipherSuites(enabledCipherSuites);
}
+ Boolean clientAuth = secure.getClientAuth();
+ if (Boolean.TRUE.equals(clientAuth)) {
+ engine.setNeedClientAuth(true);
+ }
+ if (Boolean.FALSE.equals(clientAuth)) {
+ engine.setWantClientAuth(true);
+ }
Review comment:
Idem
```suggestion
if (clientAuth) {
engine.setNeedClientAuth(true);
} else {
engine.setWantClientAuth(true);
}
```
?
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751086356
##########
File path: server/protocols/protocols-library/src/test/resources/testServerPlain.xml
##########
@@ -0,0 +1,9 @@
+<testerver enabled="true">
+ <jmxName>testserver-custom</jmxName>
+ <bind>0.0.0.0:25</bind>
Review comment:
OK. This is supposed to be a configuration test only, so the address is never used anyway. I would have liked to have something like 1.2.3.4:5 here, but since the bind address is unfortunately not testable, I might as well keep it all zeroes.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754050152
##########
File path: protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
##########
@@ -29,15 +29,17 @@
private final SSLContext context;
private final boolean starttls;
private final String[] enabledCipherSuites;
+ private Boolean clientAuth;
- private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites) {
+ private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites, Boolean clientAuth) {
Review comment:
Agreed, enums kind of are the Java way to express tri-state or multi-state logic. But as discussed below, I will remove the unused optional authentication feature for now, so this will become a simple `boolean`.
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754331583
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/BasicChannelUpstreamHandler.java
##########
@@ -215,6 +216,14 @@ protected ProtocolSession createSession(ChannelHandlerContext ctx) throws Except
if (enabledCipherSuites != null && enabledCipherSuites.length > 0) {
engine.setEnabledCipherSuites(enabledCipherSuites);
}
+ ClientAuth clientAuth = secure.getClientAuth();
+ if (ClientAuth.NEED.equals(clientAuth)) {
+ engine.setNeedClientAuth(true);
+ }
+ if (ClientAuth.WANT.equals(clientAuth)) {
+ engine.setWantClientAuth(true);
+ }
+ LOGGER.info("start SMTP with TLS client auth need {} want {}", engine.getNeedClientAuth(), engine.getWantClientAuth());
Review comment:
How can we assume from within a Neety project that this would only be used for SMTP?
As far as I am aware of, `BasicChannelUpstreamHandler` is also used for POP3.
```suggestion
LOGGER.info("start {} with TLS client auth need {} want {}", protocol.getName(), engine.getNeedClientAuth(), engine.getWantClientAuth());
```
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/NettyServer.java
##########
@@ -122,6 +122,7 @@ protected ChannelPipelineFactory createPipelineFactory(ChannelGroup group) {
maxCurConnectionsPerIP,
group,
secure != null ? secure.getEnabledCipherSuites() : null,
+ secure != null ? secure.getClientAuth() : null,
Review comment:
```suggestion
secure != null ? secure.getClientAuth() : ClientAuth.NONE,
```
?
--
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] ottoka edited a comment on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka edited a comment on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-975650330
> My recommendation is to have the Encryption.getContext method applying the cypher suite and client auth extra configuration before returning it to the caller, thus the caller can safely be ignorant to these details. Ideally, getting read of the getters on cyphers and client auth should also be possible. (I would highly welcome such a refactoring)
Thing is, you have to apply cipher suites and client auth to the final SSLEngine or SSLSocket, not the SSLContext from which they are created (as I already said, its a weird API).
I could wire the client auth setting through to `IMAPServer` and `ManageSieveServer` though, as it already is with cipher suites. Alternatively, maybe introduce `Encryption.createSSLEngine()` (which applies the settings), and pass the Encryption object wherever we have context+ciphers now. Any thoughts?
--
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 #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-975175892
Sorry I am back from 01 week of vacation so I am having a new, deeper look at this work ;-)
--
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] ottoka commented on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-970111530
BTW: When I created the unit test for this, I noticed that apparently there is no code that uses the <algorithm> and <provider> settings. Did I miss something? Are the documentation and samples outdated? Did the relevant code get lost at some point? I am confused. Could someone more experienced with the James code base have a look please?
--
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] ottoka commented on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-971548356
@chibenwa Do you have any insights on the `algorithm` and `provider` properties I mentioned above?
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751051185
##########
File path: src/site/xdoc/server/config-ssl-tls.xml
##########
@@ -109,6 +109,29 @@
</tls>
</source>
+ <p>When you enable TLS, you may also configure the server to request a client certificate for authentication:</p>
Review comment:
Idem a distinct sub-section could be nice.
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754372780
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/NettyServer.java
##########
@@ -122,6 +122,7 @@ protected ChannelPipelineFactory createPipelineFactory(ChannelGroup group) {
maxCurConnectionsPerIP,
group,
secure != null ? secure.getEnabledCipherSuites() : null,
+ secure != null ? secure.getClientAuth() : null,
Review comment:
Even better with Optionals
```suggestion
Optional.ofNullable(secure).map(s -> s.getClientAuth()).orElse(NONE),
```
--
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] ottoka commented on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-975650330
> My recommendation is to have the Encryption.getContext method applying the cypher suite and client auth extra configuration before returning it to the caller, thus the caller can safely be ignorant to these details. Ideally, getting read of the getters on cyphers and client auth should also be possible. (I would highly welcome such a refactoring)
Seems like a sensible solution to me. But should I do the refactoring in this PR, or is it too far beyond its scope and should be a separate PR?
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754291061
##########
File path: protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
##########
@@ -46,13 +48,15 @@ public static Encryption createTls(SSLContext context) {
*
* @param enabledCipherSuites
* or <code>null</code> if all Ciphersuites should be allowed
+ * @param clientAuth
+ * specifies certificate based client authentication mode
*/
- public static Encryption createTls(SSLContext context, String[] enabledCipherSuites) {
- return new Encryption(context, false, enabledCipherSuites);
+ public static Encryption createTls(SSLContext context, String[] enabledCipherSuites, ClientAuth clientAuth) {
+ return new Encryption(context, false, enabledCipherSuites, clientAuth);
}
public static Encryption createStartTls(SSLContext context) {
- return createStartTls(context, null);
+ return createStartTls(context, null, null);
Review comment:
Why handle null case now that we have an enum? Can't we pass `NONE` instead?
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754299411
##########
File path: protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
##########
@@ -46,13 +48,15 @@ public static Encryption createTls(SSLContext context) {
*
* @param enabledCipherSuites
* or <code>null</code> if all Ciphersuites should be allowed
+ * @param clientAuth
+ * specifies certificate based client authentication mode
*/
- public static Encryption createTls(SSLContext context, String[] enabledCipherSuites) {
- return new Encryption(context, false, enabledCipherSuites);
+ public static Encryption createTls(SSLContext context, String[] enabledCipherSuites, ClientAuth clientAuth) {
+ return new Encryption(context, false, enabledCipherSuites, clientAuth);
}
public static Encryption createStartTls(SSLContext context) {
- return createStartTls(context, null);
+ return createStartTls(context, null, null);
Review comment:
Oops, missed this one.
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r753984896
##########
File path: server/apps/distributed-app/docs/modules/ROOT/pages/configure/ssl.adoc
##########
@@ -84,6 +84,32 @@ Please note `JKS` keystore format is also supported (default value if no keystor
</tls>
....
+
+=== Client authentication via certificates
+
+When you enable TLS, you may also configure the server to request a client certificate for authentication:
+
+....
+<tls socketTLS="false" startTLS="true">
+ <keystore>file://conf/keystore</keystore>
+ <keystoreType>JKS</keystoreType>
+ <secret>yoursecret</secret>
+
+ <clientAuth required="true">
+ <truststore>file://conf/truststore</truststore>
+ <truststoreType>JKS</truststoreType>
+ <truststoreSecret>yoursecret</truststoreSecret>
+ </clientAuth>
+</tls>
+....
+
+With `required="true"`, the server refuses any client connections that do not include a client certificate.
+
+With `required="false"`, the servers requests an optional client certificate, but also accepts connections without one.
Review comment:
What is the benefit of `required=false` ?
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r754302438
##########
File path: protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
##########
@@ -46,13 +48,15 @@ public static Encryption createTls(SSLContext context) {
*
* @param enabledCipherSuites
* or <code>null</code> if all Ciphersuites should be allowed
+ * @param clientAuth
+ * specifies certificate based client authentication mode
*/
- public static Encryption createTls(SSLContext context, String[] enabledCipherSuites) {
- return new Encryption(context, false, enabledCipherSuites);
+ public static Encryption createTls(SSLContext context, String[] enabledCipherSuites, ClientAuth clientAuth) {
+ return new Encryption(context, false, enabledCipherSuites, clientAuth);
}
public static Encryption createStartTls(SSLContext context) {
- return createStartTls(context, null);
+ return createStartTls(context, null, null);
Review comment:
Aaand fixed.
--
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] ottoka commented on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-975634813
> Sorry still picky on this one, Netty layer is a bit legacy, thus changes there are not the easiest one to conduct out!
That's fine, anything that improves code quality is a good thing. And cooperation on such issues is why we do all this in the first place, right? :-)
--
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 change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
chibenwa commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r753983837
##########
File path: protocols/api/src/main/java/org/apache/james/protocols/api/Encryption.java
##########
@@ -29,15 +29,17 @@
private final SSLContext context;
private final boolean starttls;
private final String[] enabledCipherSuites;
+ private Boolean clientAuth;
- private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites) {
+ private Encryption(SSLContext context, boolean starttls, String[] enabledCipherSuites, Boolean clientAuth) {
Review comment:
Maybe an enum would be better than a Boolean.
Especially an enum would enable to explicit the usage of `null` that is unclear to me.
This might allow NOT to require the `if(Boolean.TRUE.equals(clientAuth)` stuff.
Let me suggest:
```
public enum ClientAuth {
NEEDED,
WANTED,
NONE
}
```
--
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] ottoka edited a comment on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka edited a comment on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-975634813
> Sorry still picky on this one, Netty layer is a bit legacy, thus changes there are not the easiest one to conduct out!
That's fine, anything that improves code quality is a good thing. And cooperation on such issues is why we do all this in the first place, right? :-)
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751088182
##########
File path: server/apps/distributed-app/docs/modules/ROOT/pages/configure/ssl.adoc
##########
@@ -84,6 +84,29 @@ Please note `JKS` keystore format is also supported (default value if no keystor
</tls>
....
+When you enable TLS, you may also configure the server to request a client certificate for authentication:
Review comment:
Sure, I will update the documentation accordingly.
--
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] ottoka commented on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-971434219
> Question: Is client authentication also applied upon STARTTLS or is it omly applies to plain TLS?
The point is that the TLS negotiation will fail and prevent the connection if the client provides an unknown/invalid certificate, or none at all (in need mode, required=true). So you could use it with StartTLS as well if that is what you want to happen. Admittedly it makes more sense in a private mail network where you can use straight SMTPS on port 465 from the start.
--
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] ottoka commented on a change in pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka commented on a change in pull request #750:
URL: https://github.com/apache/james-project/pull/750#discussion_r751107193
##########
File path: protocols/netty/src/main/java/org/apache/james/protocols/netty/AbstractSSLAwareChannelPipelineFactory.java
##########
@@ -64,6 +66,12 @@ public ChannelPipeline getPipeline() throws Exception {
if (enabledCipherSuites != null && enabledCipherSuites.length > 0) {
engine.setEnabledCipherSuites(enabledCipherSuites);
}
+ if (Boolean.TRUE.equals(clientAuth)) {
+ engine.setNeedClientAuth(true);
+ }
+ if (Boolean.FALSE.equals(clientAuth)) {
+ engine.setWantClientAuth(true);
+ }
Review comment:
See latest push.
--
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] ottoka edited a comment on pull request #750: JAMES-3672 : Support TLS authentication via client certificate
Posted by GitBox <gi...@apache.org>.
ottoka edited a comment on pull request #750:
URL: https://github.com/apache/james-project/pull/750#issuecomment-970111530
BTW: When I created the unit test for this, I noticed that apparently there is no code that uses the `algorithm` and `provider` settings. Did I miss something? Are the documentation and samples outdated? Did the relevant code get lost at some point? I am confused and a bit concerned. Could someone more experienced with the James code base have a look please?
--
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