New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
spnego_gssapi: implement TLS channel bindings for openssl #7196
spnego_gssapi: implement TLS channel bindings for openssl #7196
Conversation
Run |
ce3cde8
to
7bf94d3
Compare
I've fixed the style problems and also fixed the |
This has been lingering for much too long. Can you explain, in layman terms, exactly what this PR brings and why? |
Without channel binding SPNEGO can be attacked by reusing an authentication happening over http (in cleartext) to authenticate over https. For example if there is a git client doing When the clients support channel binding the server can simple refuse GSSAPI authentication without channel binding (and refuse GSSAPI authentication over unencrypted http). The server can now distinguish between authentication data for In addition, the server will notice if the client's idea of the server's public key is incorrect (e.g. because the client is using This way of using channel binding data for SPNEGO/GSSAPI is what e.g. Microsoft Edge will do also when connecting to a https server and sending GSSAPI authentication data. |
Note: Here is a bug report regarding implementing something similar in firefox https://bugzilla.mozilla.org/show_bug.cgi?id=1179722 and the implementation in chromium https://codereview.chromium.org/1408433006/. |
Hello @steffen-kiess !
Apologies for the beginner question, but which package exactly do I need verify version of to test this patch?
It looks like I am primarily running 1.17. Will upgrading a specific package like |
I think upgrading only a single package from What I did (on
Then it should be possible to run the compiled
|
Hello @steffen-kiess , I just wanted to let you know that I've been able to verify this patch is working and solving the problem. Unfortunately in my scenario it required obscene amount of debugging, but that was rather an underlying issue with the computer apparently not having joined the AD network correctly, and some SSSD and Kerberos configuration that required changing. Also I ended up having to do:
Without I will now try start fresh with a new VM and see exactly which of these configuration changes are required and what the minimum-level amount of configuration is needed to get it working within the network (for example I ended up installing krb5 1.19 as system lib instead of linking against it via ENV var, I wanna see if it's possible without doing that), but that is not really related to this PR. Thank you so much for the work on this PR! ❤️ Trying to google about information on "Ubuntu SSSD secure context curl" gives very limited information, debugging in this area is hard, and software support in Linux-world has been lacking. It took me several days to even find this PR, so would love to see this merged in. |
@@ -4570,6 +4570,72 @@ static void ossl_disassociate_connection(struct Curl_easy *data, | |||
} | |||
} | |||
|
|||
static CURLcode ossl_get_tls_server_end_point(struct Curl_easy *data, | |||
int sockindex, char **binding, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C:\projects\curl\lib\vtls\openssl.c(4574,51): error C2220: the following warning is treated as an error [C:\projects\curl\lib\libcurl.vcxproj]
C:\projects\curl\lib\vtls\openssl.c(4574,51): warning C4100: 'sockindex': unreferenced formal parameter [C:\projects\curl\lib\libcurl.vcxproj]
C:\projects\curl\lib\vtls\openssl.c(4573,65): warning C4100: 'data': unreferenced formal parameter [C:\projects\curl\lib\libcurl.vcxproj]
AppVeyor is complaining about unused variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should only happen if #if OPENSSL_VERSION_NUMBER > 0x10100000L
is not satisfied. I can add (void)
casts to ignore the parameters in the #else
branch.
It seems like it is no longer possible to view the Build log from Azure on why it is failing, probably because the build was over 6 months ago. |
I can try rebasing the branch, then there should be a new build. |
7a883aa
to
0ad6f58
Compare
Ok, I've rebased to the current master, fixed the |
This ended up being so big of a pain; it had consumed so much support time that IT eventually chose to simply disable the requirement for Secure Channel Binding... Although, they are likely to want to turn this back on in a few years time. It would therefore be great if this PR could be merged so it would eventually start trickle down into a release, hopefully in time to make it into Ubuntu 22 if possible. |
@Foorack upvote it, or try to resolve the merge conflicts |
Hello @DemiMarie Wow, it's been so long that I had forgotten this issue. Altough while they temporarily disabled the requirement for CSB, they will likely want to enable it again in the future, so it would still be appreciated for this to be merged as Channel Binding becomes more industry adopted. Where do you want me to vote? Simple thumbs up on the main post? |
@steffen-kiess Can you please try rebase when you have time? |
This requires krb5 >= 1.19 because otherwise channel bindings won't be forwarded through SPNEGO.
0ad6f58
to
2047725
Compare
I've rebased the PR. (Basically no change, except that mesalink support was dropped.) |
Hi all, Have you progressed on it? |
@Neustradamus I recommend reacting with thumbs-up to the PR |
Actually I am thankful for the email notification, as it reminded me of this project. This issue temporarily stopped being a priority for me as IT disabled Enforced Secure Channel Binding (SCB) on the specific server, although this is an issue that will come back in the future. I am currently overloaded right now, but could possibly start look at this again next month. The test environment is to set up two identical servers, one with SCB and one without, and then try reproduce the issue both on this PR-branch and on master branch. |
@SGA-max-faxalv, @Foorack: Thanks for your comment! :) I have originally done a ticket here: #9226 about the RFC 9266: Channel Bindings for TLS 1.3 support. TLS Channel Bindings uses:
About GNU SASL and SCRAM: |
I'm sorry but I see very little desire from users for this and the PR has now fallen behind and has lots of merge conflicts. Closing. |
@bagder: Little desire? It is sure if you do not want, it will not help! TLS Channel Binding is important for security! Have you seen the list here? TLS Channel Binding is supported for example by GNU SASL, GnuTLS, Cyrus SASL/IMAPD, PostgreSQL, Exim, ...
What do you think about this feature and this PR? Thanks in advance. |
This feature is important if curl should be able to talk to a web server which enforces secure channel binding on Kerberos auth (recommended by Microsoft!). I will start priorizing setting up a dev environment. |
@Neustradamus @steffen-kiess This has been re-based (did not apply cleanly due to refactoring in the last 2 years) and proposed as a new PR. #13098 |
This requires krb5 >= 1.19 because otherwise channel bindings won't be
forwarded through SPNEGO.