-
Notifications
You must be signed in to change notification settings - Fork 50
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
MbedTLS 1.1.1 causing issues for authentication on Azure #250
Comments
I just tagged v1.1.2 (like 5 minutes ago) with a fix for a an issue introduced in v1.1.1. Could you pull and try again to see if the issue persists? |
ok, pinging @vtjnash here; I'm currently on vacation, but can help dig in and try to figure out what's going on once I get back (next week). |
Not a big rush since using |
Hi, Just a small update given the release of MbedTLS 1.1.3 (Thanks to @mbauman for pointing me to the new release). It looks like we have the same issue. On the left is mbedtls 1.1.0 where we see no errors thrrown, in the middle is with mbedtls 1.1.3 where we see the same IOError, and on the right is mbedtls 1.1.2.
|
Do you have local changes to AzSessions? I tried your example above with my own account, and just got back generic "invalid_client" messages due to the lack of a client_secret, so it seemed that this would not be able to get a token with that authentication flow. |
@vtjnash I do not have any local changes to AzSessions other than changing the edit |
Okay, I found there is a checkbox for this (https://stackoverflow.com/questions/45609432/how-do-i-resolve-the-error-aadsts7000218-the-request-body-must-contain-the-foll) It looks like this is an unhandled race condition in HTTP.jl with the ConnectionPool, but we can also reduce the duration of the race window so that it is harder to hit that bug, which helps to make this unlikely: JuliaWeb/HTTP.jl#901 |
@samtkaplan, can you confirm that upgrading HTTP.jl (v1.2.1) fixes the issue here? I realize this got auto-closed, but just wanted to follow up with you. |
@quinnj Thanks so much for checking in. I really appreciate all of the work. Unfortunately, it looks like there is still an issue. Perhaps, Interesting, the issue seems to be slightly different now in that it manifests more quickly than it did before. |
Hmmm, yeah, so this error should be benign; i.e. your requests should keep working. This is more reporting that a connection we were thinking of reusing ended up getting closed. @vtjnash, what do you think of changing function monitor_idle_connection(c::Connection)
try
if eof(c.io) ;@debugv 3 "💀 Closed: $c"
close(c.io)
end
catch ex
@try Base.IOError close(c.io)
ex isa Base.IOError || rethrow()
end
nothing
end |
That sounds good. Additionally, another reliability feature I think is missing here is checking whether it is still in the right state before calling eof or close. If the object already is being reused or closed, it should not do anything here. A simple age counter that increments on every use, and passed as an argument here, would help do that. (And would allow implementing it more accurately to the comment, which says it would close if any data gets received) |
#907) * Make monitor_idle_connections less noisy; follow up on JuliaLang/MbedTLS.jl#250 * fix import
Hi, I just wanted to give a small update from my side. With MbedTLS 1.1.3 and HTTP master, things are working great. I guess we can close this. Do you know when the next version of HTTP.jl will be released? I'm guessing that this commit that is not yet in a release was the final one required to close this issue (although as you noted that particular "error" message should be benign) . No rush, just curious. Many thanks!! |
Just made a new HTTP.jl 1.3.0 release today that includes the commit. Thanks for the help tracking these issues down! |
Hi,
I'm noticing that MbedTLS 1.1.1 is causing issues when authenticating on Azure using the AzSessions.jl package. In particular, I am noticing that the AzSessions.jl retry logic is being hit more than expected with a
Base.IOError
thrown. The trace indicates that the problem may be coming from the MbedTLSssl_write
method. I suppose that this may be related to #247?Here is a simple code to test (Please note that
sessions
is using device code flow for authentication):The error is triggered when AzSessions uses its OAuth2 refresh-token to retrieve a new OAuth2 access-token. Note that the refresh-token logic is only triggered about once per hour (when the access-token expires). Below is a screen-shot where on the left we are using MbedTLS 1.1.1, and on the right we are using MbedTLS 1.1.0.
Notice that on the right, we hit the
IOError
causing the AzSessions retry logic to be invoked, where-as on the right (with MbedTLS 1.1.0) we don't hit the retry logic.I have not done much to dig into the possible root cause, but I thought I would share my current observation.
Thanks!
Sam
The text was updated successfully, but these errors were encountered: