Skip to content
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

Implement "Partial success" authentication #542

Conversation

falemagn
Copy link
Contributor

The authentication callback can now return WOLFSSH_USERAUTH_PARTIAL_SUCCESS to indicate partial success, as per RFC 4252.

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@falemagn falemagn force-pushed the 378e73a_The_AuthTypesCb_can_now_return_partial_success branch from b96aee4 to 7ccb5ec Compare July 21, 2023 09:57
@JacobBarthelmeh JacobBarthelmeh self-assigned this Jul 21, 2023
@JacobBarthelmeh
Copy link
Contributor

ok to test

@@ -11763,7 +11767,7 @@ int SendUserAuthFailure(WOLFSSH* ssh, byte partialSuccess)
ret = WS_BAD_ARGUMENT;

if (ret == WS_SUCCESS) {
authSz = GetAllowedAuth(ssh, authStr);
authSz = GetAllowedAuth(ssh, authStr, &partialSuccess);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the partial success should be coming from the userAuthCb return value. Adding WOLFSSH_USERAUTH_PARTIAL_SUCCESS to the list of possible return values from the user auth instead of to the list of auth types. And then if it is returned, setting a new variable partialSuccess to 1 and passing it as the last argument to SendUserAuthFailure in the DoAuthRequest* functions instead of the current hard set '0' in wolfSSH master.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JacobBarthelmeh the advantage of handling it the current way is that the change is centralized in a few lines of code.

We can do it the way you suggest, but then it would conflict with PR #559. I suggest that to be merged first, then.

@ejohnstown
Copy link
Contributor

This isn't entirely correct. There are two callback functions used in this process. The user authentication callback and the user authentication type callback. You added a flag for use in the type callback, for setting the partialSuccess flag.

What is missing is a return value for the user authentication callback. Which return value are you using to indicate a partial success? If the callback returns WOLFSSH_USERAUTH_SUCCESS, the SendUserAuthFailure function isn't called. If the callback returns any existing failure, the flow leads down to SendUserAuthFailure() which will send the partial success as true to the client, but it never verifies the signature in the message with the public key.

I am going to add WOLFSSH_USERAUTH_PARTIAL_SUCCESS to the enumeration WS_UserAuthResults. It is new, existing code won't know it is available. The user authentication callback will know that the key is expected, and that it is keeping track of partial successes. In the case where you want both a password and public key to authenticate a user, the callback can return the partial success value. The library will check the signature as expected. Then it will send the failure message with partial success set. (It will also need to skip sending in the !hasSig case.)

I'm also going to update the password side.

@ejohnstown
Copy link
Contributor

I believe this is solved with PR #571. I'm going to close this PR.

@ejohnstown ejohnstown closed this Sep 5, 2023
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.

4 participants