Skip to content

Limit number of authentication attempts#6030

Open
ecrocombe wants to merge 8 commits intonpgsql:mainfrom
ecrocombe:main
Open

Limit number of authentication attempts#6030
ecrocombe wants to merge 8 commits intonpgsql:mainfrom
ecrocombe:main

Conversation

@ecrocombe
Copy link

This removes the while(true) loop in authentication process.
Replacing it with a maximum number of attempts read from MAX_AUTH_ATTEMPTS.

Ideally, we would want to know why authentication is not progressing or changing its state during the process, so this is kind of a stop gap to try and prevent holding up threads.

@ecrocombe
Copy link
Author

Stop gap for #6029

@vonzshik
Copy link
Contributor

This is not going to help much because timeout.CheckAndApply (which we call first thing in the loop) will throw if the timeout is elapsed (and in the original issue they have timeout equal to 15 seconds). This is why I'm still convinced that the problem is somewhere in between OpenSSL/.NET and not in npgsql.

@ecrocombe
Copy link
Author

Good spot, however limiting the attempts to 10 instead of it racing around for N seconds was the original goal of this PR.

But I digress, I understand this will not help much.

No hard feelings if this is denied.

@vonzshik
Copy link
Contributor

@roji I'll leave it up to you

@roji
Copy link
Member

roji commented Feb 27, 2025

I'm not sure I have the full context here... Do we know how many actual back and forths are typical in an auth flow, in other words, is 10 a good number, or should it be 100?

I'm generally not against having a maximum here - it's not a bad idea wherever there's an infinite loop, just in case... But we have to make sure we don't block any possible normal usages (where there are more than 10 roundtrips?). Also, if I understand correctly, this isn't meant to be a fix for the actual problem here in any case, right?

@ecrocombe
Copy link
Author

ecrocombe commented Feb 27, 2025

Do we know how many actual back and forths are typical in an auth flow, in other words, is 10 a good number, or should it be 100?

After reading #5006 , It seemed 4 was needed, so i thought i was generous with 10, but I'm not familiar enough to make that decision.

Also, if I understand correctly, this isn't meant to be a fix for the actual problem here in any case, right?

Correct, I intended this PR to be a means to remove suspect calls the Rfc2898DeriveBytes.Pbkdf2 as it would seem it caught the interest of dotnet/runtime. And I thought it was something i could easily make up in my time to save npgsql the trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants