[GitHub] [tomcat] efge opened a new pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge opened a new pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox

efge opened a new pull request #406:
URL: https://github.com/apache/tomcat/pull/406


   The problem solved here is exemplified by https://stackoverflow.com/questions/64911070/clients-certificate-authentication-issue-in-tomcat-in-7-0-100.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox

efge commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-786648989


   (force-pushed to rebase)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

efge commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-790618988


   (force-pushed to rebase)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on a change in pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

michael-o commented on a change in pull request #406:
URL: https://github.com/apache/tomcat/pull/406#discussion_r587564027



##########
File path: java/org/apache/catalina/valves/SSLValve.java
##########
@@ -137,7 +149,13 @@ public void invoke(Request request, Response response) throws IOException, Servl
          *       separate lines, the CertificateFactory is tolerant of any
          *       additional whitespace.
          */
-        String headerValue = mygetHeader(request, sslClientCertHeader);
+        String headerValue;
+        String headerEscapedValue = mygetHeader(request, sslClientEscapedCertHeader);
+        if (headerEscapedValue != null) {
+            headerValue = URLDecoder.decode(headerEscapedValue, "ISO-8859-1");

Review comment:
       No, URLDecoder does *not* decode URIs. It has been designed for HTML forms only. Tomcat has a utility to do this properly. Moreover, it should be either UTF-8 or better the encoding from the `server.xml`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge commented on a change in pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

efge commented on a change in pull request #406:
URL: https://github.com/apache/tomcat/pull/406#discussion_r588412956



##########
File path: java/org/apache/catalina/valves/SSLValve.java
##########
@@ -137,7 +149,13 @@ public void invoke(Request request, Response response) throws IOException, Servl
          *       separate lines, the CertificateFactory is tolerant of any
          *       additional whitespace.
          */
-        String headerValue = mygetHeader(request, sslClientCertHeader);
+        String headerValue;
+        String headerEscapedValue = mygetHeader(request, sslClientEscapedCertHeader);
+        if (headerEscapedValue != null) {
+            headerValue = URLDecoder.decode(headerEscapedValue, "ISO-8859-1");

Review comment:
       Thanks yes I'm aware of what `URLDecoder` does but in this instance it was "good enough" given that we're decoding an ad hoc encoding done by Nginx that is a subset of what `URLDecoder` can decode. Also I had searched in the Tomcat codebase for a utility to do this but didn't find anything.
   
   I searched again deeper and finally found `UDecoder`/`UEncoder` which seem very old and have weird calling conventions but do the job.
   
   Regarding the charset I was using ISO-8859-1 because that's what's used a few lines below when converting the cert to a byte array, but I can use UTF-8 if you prefer. Again this is for an ad hoc Nginx encoding that encodes an ASCII cert anyway so the end result is the same.
   
   The PR has been updated with these changes.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge commented on a change in pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

efge commented on a change in pull request #406:
URL: https://github.com/apache/tomcat/pull/406#discussion_r588412956



##########
File path: java/org/apache/catalina/valves/SSLValve.java
##########
@@ -137,7 +149,13 @@ public void invoke(Request request, Response response) throws IOException, Servl
          *       separate lines, the CertificateFactory is tolerant of any
          *       additional whitespace.
          */
-        String headerValue = mygetHeader(request, sslClientCertHeader);
+        String headerValue;
+        String headerEscapedValue = mygetHeader(request, sslClientEscapedCertHeader);
+        if (headerEscapedValue != null) {
+            headerValue = URLDecoder.decode(headerEscapedValue, "ISO-8859-1");

Review comment:
       Thanks yes I'm aware of what `URLDecoder` does but in this instance it was "good enough" given that we're decoding an ad hoc encoding done by Nginx that is a subset of what `URLDecoder` can decode. Also I had searched in the Tomcat codebase for a utility to do this but didn't find anything.
   
   I searched again deeper and finally found `UDecoder`/`UEncoder` which seem very old and have weird calling conventions but do the job.
   
   Regarding the charset, I was using ISO-8859-1 because that's what's used a few lines below when converting the cert string to a byte array, but I can use UTF-8 if you prefer. Again this is for an ad hoc Nginx encoding that encodes an ASCII cert anyway so the end result is the same.
   
   The PR has been updated with these changes.
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on a change in pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

michael-o commented on a change in pull request #406:
URL: https://github.com/apache/tomcat/pull/406#discussion_r588501243



##########
File path: java/org/apache/catalina/valves/SSLValve.java
##########
@@ -137,7 +149,13 @@ public void invoke(Request request, Response response) throws IOException, Servl
          *       separate lines, the CertificateFactory is tolerant of any
          *       additional whitespace.
          */
-        String headerValue = mygetHeader(request, sslClientCertHeader);
+        String headerValue;
+        String headerEscapedValue = mygetHeader(request, sslClientEscapedCertHeader);
+        if (headerEscapedValue != null) {
+            headerValue = URLDecoder.decode(headerEscapedValue, "ISO-8859-1");

Review comment:
       Yes, I was referring to the UDecoder. I have nagged last year the usage of URLEncoder and Mark removed it. We should avoid it altogether for concistency. Since URLs are by default UTF-8, you should use UTF-8 unless there is a good reason for that.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

michael-o commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-791583579


   HTTPd BZ issue: https://bz.apache.org/bugzilla/show_bug.cgi?id=65169


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

efge commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-801276629


   (force-pushed to rebase and update correct section of changelog.xml)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge edited a comment on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

efge edited a comment on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-801276629


   (force-pushed to rebase and update correct section of changelog.xml after latest release)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o edited a comment on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

michael-o edited a comment on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-802077740


   Note that Joe Orton is working on HTTPd to make this possible too A new set of headers will contain certs in DER form encoded with Base 64.


--
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

michael-o commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-802077740


   Note that Joe Orton is working in HTTPd to make this possible too A new set of headers will contain certs in DER form encoded with Base 64.


--
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] michael-o commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

michael-o commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-803115444


   The change on HTTPd is now on trunk. It does not use the NGINX approach.


--
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

efge commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-803585111


   Hi. Anything prevent a committer from merging this PR (besides time)?


--
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

efge commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-804437149


   (force-pushed to rebase)


--
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] efge commented on pull request #406: Improve the SSLValve so it is able to handle the ssl_client_escaped_cert header from Nginx

GitBox
In reply to this post by GitBox

efge commented on pull request #406:
URL: https://github.com/apache/tomcat/pull/406#issuecomment-815291840


   (force-pushed to rebase and update correct section of changelog.xml after latest release)


--
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]