Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1774954495 Merged into all branches. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o closed pull request #672: BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile URL: https://github.com/apache/tomcat/pull/672 -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
rmaucher commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1772689379 Ok, and I'll update the new OpenSSLContext to do things properly (eventually) since it would be better to use a memory BIO rather than a file BIO. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1772580776 I'd like to merge this weekend unless there will be objections after my change. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
rmaucher commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1770353017 The Java code is a lot simpler. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1769101843 Guys, I have now changed the code by reading the password file for OpenSSL in Java, instead of C. Please have a look 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1762134403 > > I'm a -0 on loading the password from native code. I would support "consistency" by _removing_ the existing BIO-loading of the cert, key, etc. in libtcnative if we wanted to go for that. > > I was saying earlier it would still need to use a different BIO type, so I was thinking "no benefit". Now that I think about it more, this is still better. I guess I'll do it eventually with the Panama code. I think this discussion should be moved to the dev mailing list. As far as I understand you both, you want to retain `setCertificateRaw()` only. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
rmaucher commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1762132365 > I'm a -0 on loading the password from native code. I would support "consistency" by _removing_ the existing BIO-loading of the cert, key, etc. in libtcnative if we wanted to go for that. > I was saying earlier it would still need to use a different BIO type, so I was thinking "no benefit". Now that I think about it more, this is still better. I guess I'll do it eventually with the Panama code. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1762123047 > I'm a -0 on loading the password from native code. I would support "consistency" by _removing_ the existing BIO-loading of the cert, key, etc. in libtcnative if we wanted to go for that. > > Honestly, libtcnative's days are numbered. Even having this conversation is not worth the time we are spending on it. This PR is not about libtcnative, but about Tomcat. If you consider, just like @rmaucher, the change in tcnative as unnecessary, express it in the other PR. Have a look at the actual changes which affect Tomcat. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
ChristopherSchultz commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1762120662 I'm a -0 on loading the password from native code. I would support "consistency" by _removing_ the existing BIO-loading of the cert, key, etc. in libtcnative if we wanted to go for that. Honestly, libtcnative's days are numbered. Even having this conversation is not worth the time we are spending on 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on code in PR #672:
URL: https://github.com/apache/tomcat/pull/672#discussion_r1358038098
##
java/org/apache/tomcat/util/net/SSLHostConfig.java:
##
@@ -796,9 +796,6 @@ public static String adjustRelativePath(String path) throws
FileNotFoundExceptio
newPath = System.getProperty(Constants.CATALINA_BASE_PROP) +
File.separator + newPath;
f = new File(newPath);
}
-if (!f.exists()) {
Review Comment:
I left the commits intentionally for those who like to test this PR in
action.
--
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: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
rmaucher commented on code in PR #672:
URL: https://github.com/apache/tomcat/pull/672#discussion_r1358035893
##
java/org/apache/tomcat/util/net/SSLHostConfig.java:
##
@@ -796,9 +796,6 @@ public static String adjustRelativePath(String path) throws
FileNotFoundExceptio
newPath = System.getProperty(Constants.CATALINA_BASE_PROP) +
File.separator + newPath;
f = new File(newPath);
}
-if (!f.exists()) {
Review Comment:
Yes ok.
--
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: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on code in PR #672:
URL: https://github.com/apache/tomcat/pull/672#discussion_r1358027078
##
java/org/apache/tomcat/util/net/SSLHostConfig.java:
##
@@ -796,9 +796,6 @@ public static String adjustRelativePath(String path) throws
FileNotFoundExceptio
newPath = System.getProperty(Constants.CATALINA_BASE_PROP) +
File.separator + newPath;
f = new File(newPath);
}
-if (!f.exists()) {
Review Comment:
Note the first line of the PR and the prefix on the commit:
NOTE: Disregard the [TEMPORARY] commits, they are for testing purposes only
and will not be merged.
--
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: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on code in PR #672:
URL: https://github.com/apache/tomcat/pull/672#discussion_r1358027078
##
java/org/apache/tomcat/util/net/SSLHostConfig.java:
##
@@ -796,9 +796,6 @@ public static String adjustRelativePath(String path) throws
FileNotFoundExceptio
newPath = System.getProperty(Constants.CATALINA_BASE_PROP) +
File.separator + newPath;
f = new File(newPath);
}
-if (!f.exists()) {
Review Comment:
Note the first line of the PR and the prefix on the commit.
--
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: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
rmaucher commented on code in PR #672:
URL: https://github.com/apache/tomcat/pull/672#discussion_r1358020057
##
java/org/apache/tomcat/util/net/SSLHostConfig.java:
##
@@ -796,9 +796,6 @@ public static String adjustRelativePath(String path) throws
FileNotFoundExceptio
newPath = System.getProperty(Constants.CATALINA_BASE_PROP) +
File.separator + newPath;
f = new File(newPath);
}
-if (!f.exists()) {
Review Comment:
Why remove 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: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1761201586 > For certificate (key) file, there's an attempt to always use PEMFile. When that fails, it uses the code path where it simply passes the file name, since that's what the native API has had since forever. It actually does not change the native code in a significant way since it would have to use a memory BIO rather than a file BIO, this is not a simple string like the password. So, if you are totally against adding the method overload in tomcat-native, please leave a comment in that specific PR. I still think, yes, you are partially right, but it does not hurt either, it just adds convenience. I'd like to know the opinion of other committers as well. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
rmaucher commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1761197570 For certificate (key) file, there's an attempt to always use PEMFile. When that fails, it uses the code path where it simply passes the file name, since that's what the native API has had since forever. It actually does not change the native code in a significant way since it would have to use a memory BIO rather than a file BIO, this is not a simple string like the password. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1761175235 > Ok trying again. So the code addition in tomcat-native simply uses a file BIO to load the contents of the file and use it as a password. So overall, I do not understand the benefit of the rather significant amount of changes over simply loading the password files in the Java code and calling tomcat-native as before. I don't understand why any third party users of tomcat-native are not able to do the same as well, there's very little value add here and API compatibility seems more useful. I understand your point. It is basically for consistency with the Java code. We do also pass the path of the certificate and key to OpenSSL while we could load it in Java and pass raw bytes. This is the same. Consistency. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
rmaucher commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1761170846 Ok trying again. So the code addition in tomcat-native simply uses a file BIO to load the contents of the file and use it as a password. So overall, I do not understand the benefit of the rather significant amount of changes over simply loading the password files in the Java code and calling tomcat-native as before. I don't understand why any third party users of tomcat-native are not able to do the same as well, there's very little value add here and API compatibility seems more useful. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
michael-o commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1761153823 > I don't get it overall. Since that's what ultimately happens (the tomcat-native patch simply does that), I think the content of the files should simply be loaded as the password in the Java code. While I don't fully understand your statement, please read mine: https://github.com/apache/tomcat-native/pull/20#issue-1941460224 Long term I'd like to use this in Spring API Gateway with Netty and libtcnative instead of SunJSSE and well as neo4j. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] BZ 66670: Add SSLHostConfig#certificateKeyPasswordFile and SSLHostConfig#certificateKeystorePasswordFile [tomcat]
rmaucher commented on PR #672: URL: https://github.com/apache/tomcat/pull/672#issuecomment-1761136665 I don't get it overall. Since that's what ultimately happens (the tomcat-native patch simply does that), I think the content of the files should simply be loaded as the password in the Java code. -- 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: dev-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
