Return PAM_MAXTRIES on too many auth failures#1425
Return PAM_MAXTRIES on too many auth failures#1425denisonbarbosa wants to merge 2 commits intomainfrom
Conversation
denisonbarbosa
force-pushed
the
exit-on-max-tries
branch
2 times, most recently
from
768207b to
1cd7c8b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1425 +/- ##
==========================================
+ Coverage 80.00% 86.17% +6.17%
==========================================
Files 20 113 +93
Lines 990 7372 +6382
Branches 0 111 +111
==========================================
+ Hits 792 6353 +5561
- Misses 198 963 +765
- Partials 0 56 +56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return m, sendEvent(startAuthentication{}) | ||
|
|
||
| case auth.Denied: | ||
| if authMsg == "Maximum number of authentication attempts reached" { |
There was a problem hiding this comment.
I do agree with the change, but I think we should use another authentication error, not rely on strings for this.
There was a problem hiding this comment.
In fact I wanted to do this myself since the start, but we did not support the error, and we never did it.
I think if we want to do it now, we should add another kind.
There was a problem hiding this comment.
I agree with the another error idea and considered it. However, since we can't ensure API compatibility between authd and the brokers, we can't do that yet.
We are planning to update the API soon, so IMO we should wait and add all the breaking changes in a single release, so maybe adding a //TODO here as a reminder to not forget about a new error code when updating the API?
There was a problem hiding this comment.
Mmh, I'm not sure if there's any benefit in doing multiple API changes at once. We need to implement the functionality for authd to choose the best version of the broker API once. Once that's done, we should be able to just bump the API version each time we make a change to it.
There was a problem hiding this comment.
The issue is that adding a new access mode will break this flow for users (on authd-{broker} edge versions). Now that authd is in the archive, we can't easily update it to follow broker-related changes, so we need to be more careful with what we merge in main (yes, it's a specific use case of someone using stable authd and edge brokers, but it still exists)
There was a problem hiding this comment.
We have indeed various needs for API changes, so we should definitely make a list (maybe open issues and tag them?) so and try to land them separately but maybe in a different release.
However, regarding this specific case I was hoping we have been better at handling the msg.access as apparently we are not handling the default case (or well I did handle it in gdm and native model, but it was not handled in the bubbletea case), and so adding a new kind may lead the CLI UI to just ignore it rather than just erroring out (and this would have been fine).
So... I'm still thinking... But I feel that we should:
- Prepare an API break where the broker is initialized with platform data (we need for various things), that also contains the daemon version
- If the broker is contacted using the new API (we can just use another dbus interface or versioned method, and support both) then new features such as this one can go
There was a problem hiding this comment.
The issue is that adding a new access mode will break this flow for users (on authd-{broker} edge versions).
Sure, it's a breaking API change, I don't disagree with that. My point is: We have to start dealing with breaking API changes, and once the functionality for that is in place, we should be able to bump the version whenever there is a breaking change, even if it's minor.
denisonbarbosa
force-pushed
the
exit-on-max-tries
branch
2 times, most recently
from
d4db55e to
202e857
Compare
adombeck
left a comment
adombeck
left a comment
There was a problem hiding this comment.
I can still reproduce the issues with SSH and sudo I described in
I did not check if the behavior with GDM or the login command changed.
adombeck
left a comment
adombeck
left a comment
There was a problem hiding this comment.
I can still reproduce the issues with SSH and sudo I described in
#1342 (comment)
My bad, I didn't realize I also had to rebuild the broker for the change to take effect. After doing that, it works like a charm. The only things that's a bit surprising is the error message from sudo:
$ sudo ls
[sudo error] Maximum number of authentication attempts reached
sudo: maximum 1 incorrect authentication attempts
I assume it would correctly say "3 incorrect attempts" if we would return PAM_AUTH_ERR on the first incorrect attempt instead of implementing the retry logic ourselves. I did not check if that would have any other effects on UX.
@denisonbarbosa did you consider the "Return PAM_AUTH_ERR after the first incorrect attempt" option I mentioned in #1342 (comment)? What was the outcome? IMO we should document in the commit message why we went with the other option instead. |
Some callers expect PAM_AUTH_ERR after an authentication failure in order to trigger a retry. We handle this in authd by using the Retry access mode instead and leaving it up to the broker to determine how many authentication failures are allowed. However, we used to return PAM_AUTH_ERR after the user failed to authenticate the max amount of times rather than PAM_MAXTRIES (which is what is expected by the callers to end the authentication flow), which would cause some adapters to keep trying to authenticate the user even after the broker sending the "maximum authentication attempts reached" message and that was a bad UX. Now, by returning PAM_MAXTRIES, the adapters correctly inform the users that they reached the maximum attempts and stop the authentication session.
We should add a default: case in order to avoid having more issues in the future should we decide to add more access modes.
denisonbarbosa
force-pushed
the
exit-on-max-tries
branch
from
202e857 to
4498dfb
Compare
adombeck
left a comment
adombeck
left a comment
There was a problem hiding this comment.
The change LGTM, but we need to handle the breaking API change. The official D-Bus doc has a section about API versioning. We should probably follow that and add a new com.ubuntu.authd.Broker2 interface, which calls the same methods but returns AuthdDeniedMaxTries, while the old interface should still return AuthdDenied.
Yeah, in fact I do remember me to suggest to start using a versioned name since the start, but we decided that However, if we do change the interface I think we should IMHO try to at least add few changes we have already in mind, more than just returning another value (that is more an "ABI" change than API one here), otherwise IMHO we can just use versioned methods (that's easier to handle). |
I disagree. As I mentioned in #1425 (comment), I don't think we should postpone breaking changes to make several at once. I implemented a PoC in #1446 to demonstrate how I think we should handle breaking changes. That should make it simple to add new interfaces even for small breaking changes. |
3 participants
We used to return PAM_AUTH_ERR, but this created problems in some adapters, since they would keep retriggering the authentication requests even after MAX_TRIES was reached. Returning PAM_MAXTRIES should stop the requests from coming.
UDENG-9459