Defer or disable TLS client authentication #2269
Conversation
534cb31 to
0025cd1
Compare
abh3
left a comment
There was a problem hiding this comment.
In addition to the requested changes. Since "Note this only works with clients that support the post-handshake authentication option in TLS 1.3 (these include curl 7.62 or later and the curl version shipped with RHEL8 or later)." the obvious question is what happens if you set this option but the client doesn't support it? Failure,? Odd error? Since we have to support a wide mix of clients , getting into a situation where sometimes this works and sometimes not is not particularly appealing. What is the mitigation here?
472b0ee to
e358ac0
Compare
0025cd1 to
d4dabda
Compare
|
The short answer is not yet. Actually, Http uses the TlsContext for certain things. What you are seeing is that it is passing the SSL context to the xrtractor plugins which was code written before we had a TLSContext class. It was unfortunate but something we could not eliminate until a major release. So, that was what we planned to do to finally centralize OpenSsl code dependencies.
________________________________
From: ***@***.*** ***@***.***> on behalf of Brian P Bockelman ***@***.***>
Sent: Wednesday, July 24, 2024 6:48 AM
To: xrootd/xrootd ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [xrootd/xrootd] Defer or disable TLS client authentication (PR #2269)
@bbockelm commented on this pull request.
________________________________
In src/XrdHttp/XrdHttpProtocol.cc<#2269 (comment)>:
@@ -672,6 +709,49 @@ int XrdHttpProtocol::Process(XrdLink *lp) // We ignore the argument here
}
+
+#if OPENSSL_VERSION_NUMBER >= 0x10100010L
+ if (postheaderwait) {
+ postheaderwait = false;
+ if (SSL_verify_client_post_handshake(ssl) != 1) {
This is all manipulation of the SSL socket so the natural place for this would be to live in XrdTlsSocket.
However, there's zero use of XrdTlsSocket in XrdHttp - it utilizes the SSL context directly. Introducing and using it for only this small part of the handshake doesn't seem practical. If we bring XrdTlsSocket in, it wants to own the SSL context object and manipulates its internals.
So, short of a reimplementation of the guts of the XrdHttpProtocol object to use XrdTlsSocket, is there any way to provide a containment mechanism?
—
Reply to this email directly, view it on GitHub<#2269 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA7NRDWXNKSGJXNRCL5L36DZN6WDVAVCNFSM6AAAAABH4JPA4CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJWHAYDKNJUGQ>.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>
…________________________________
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-DEV list, click the following link:
http://listserv.slac.stanford.edu/scripts/wa.exe?SUBED1=XROOTD-DEV&A=1
|
7e12e96 to
b89a1ba
Compare
d4dabda to
aaac36e
Compare
aaac36e to
f6e06a8
Compare
1930490 to
db61ad6
Compare
53d0726 to
1d7b0e0
Compare
|
(Note: converting the PR back to "draft" status until Emma has a chance to re-test) |
|
Summary of in-person discussion:
|
26a4692 to
5612ef0
Compare
|
@abh3 - requested changes were completed. |
73e31bf to
4fbec14
Compare
|
As mentioned a few weeks back, I'm quite happy to share that it seems like we made the needed transition with this patch out-of-tree, greatly simplifying this to be an on/off and dropping the use of the TLS 1.3 extension. Accordingly, as this is pretty much a rewrite (particularly, removing the most controversial pieces), I'm dismissing the old reviews and re-requesting a fresh review. |
4fbec14 to
e635b6b
Compare
|
Hi @bbockelm, we will review it and consider it for 5.8. Thanks! |
e635b6b to
f977fab
Compare
amadio
left a comment
There was a problem hiding this comment.
There are still some things which I don't understand. Maybe it's due to the refactoring, but it seems to me that the setting should be passed in the constructor of the TLS context and afterwards it will not change, so nothing should be really needed in XrdHttpProtocol.cc. We should have tls.clientauth option implemented similarly to xrd.tls and xrd.tlsca, that is, independent of the protocol.
|
@amadio - thanks for the review! I just wanted to ACK I saw it and will try to get to this over the weekend. |
f977fab to
a8290ca
Compare
|
@amadio - in the latest revision, I overhauled the patch so setting the context's configuration each time was no longer needed and to include integration tests that the patch was working. Life is better, of course, with test coverage. |
a8290ca to
18bf0f5
Compare
|
@amadio and I discussed the PR. One last thing to check -- when client auth is disabled but the client still tries to present a certificate, what happens? If the server can still parse & verify the certificate when it has been presented one unprompted, then we should allow this and rename the knob something like "disable client certificate request". Reading the OpenSSL docs, I don't think this is possible -- but would be nice if it was! Otherwise, what we have is about the best we can do. |
8f63016 to
db0dc46
Compare
|
@amadio - Performed the investigation requested above. There's no way for the client to "send unprompted" the certificate (it's a protocol violation). Thus, there's no way to disable the server from prompting for a client certificate but verifying one if the client sends it regardless. Per our conversation last month, I think this is the best we can do. I think it's ready to merge for the next feature release. |
db0dc46 to
58c47a1
Compare
58c47a1 to
c0f3908
Compare
All comments have been addressed, looks good to me to go into XRootD 5.9.0.
The X.509 client auth request is fairly disruptive for communities that are browser-based (since this is an extremely uncommon flow for users -- and most of them interpret the certificate request as an error message) and not using X.509 client auth. This provides a boolean (defaulting to "on") that allows the administrator to turn off X.509 client auth when not needed / desired. Also included is test coverage for when the boolean is on and off.
c0f3908 to
dba01f6
Compare
When users visit XRootD-based HTTP servers in the browser, they get a pop-up request for selecting a client certificate. This is fairly disruptive for users not working in X.509-based environments -- and completely unnecessary if X.509 authentication is not in use!
This PR adds two new options,
tlsclientauthandtlsrequiredprefix, that control when the HTTPS server may request a client certificate. Note that this only makes sense for the HTTPS protocol and hence isn't part of the generic XrdTls configuration.http.tlsclientauth [on|off|defer]Controls the use of client certificate authentication only for the HTTPS protocol. When on (default), the HTTPS protocol server will always request a client certificate from the client. When off, it will never request a certificate. When set to defer, it will only request a client certificate is it is in a path specified by
https.tlsrequiredprefix.It is not an error if the client does not provide a certificate.
http.tlsrequiredprefix /prefixThis option, which can be specified multiple times, specifies a prefix that requires the use of TLS client certificate authentication. If a HTTP request is made for a path starting with this prefix and
http.tlsclientauthis in defer mode, then the HTTP server will request a client certificate from the client before a response is made.Note this only works with clients that support the post-handshake authentication option in TLS 1.3 (these include curl 7.62 or later and the curl version shipped with RHEL8 or later).