-
Notifications
You must be signed in to change notification settings - Fork 14
Proposed Bug Fix: Sunset-user-auth-none-fix #8
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
Conversation
src/conn.rs
Outdated
| match p.method { | ||
| AuthMethod::None | AuthMethod::Unknown(_) => { | ||
| trace!("UserauthRequest None or Unknown method requested. sending a fail"); | ||
| serv.auth.resume_request(false, s)?; | ||
| } | ||
| _ => { |
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.
Specifics of auth handling should be in servauth.rs, not the dispatch loop here.
A "None" authmethod is allowed to return success - for example if you want to allow login without any password, and drop to some kind of restricted shell. The picow demo does that currently https://github.com/mkj/sunset/blob/main/embassy/demos/common/src/server.rs#L175
Is your code handling it the same way?
I will run a rustfmt pass over the repo - will make these diffs easier to read.
|
Ah I think I see the failure you mean, will figure what's going on. |
|
As you will find out, you need that your prev_event is a password or a Public key auth event. However that is not the case on a None auth. If you want to allow that, provided that the server is configured as no password required I suppose that you can have that checked. However that would be a global no password auth. Would it be better to add a field to check each user valid authentication? Do you have time to think about it or shall I give it a go? Apologies for clippy formatting the file. Next time will be clearer. |
|
I'm happy to have a look at it. I wonder if the debug assertion is spurious, will try and remember what the intent was. |
|
I've made a change, FirstAuth should have been allowed by that assertion too. a622398 |
|
Yes it does fix it. I will use the demo std to explore more the crate so thanks for that! |
Find fix at src/conn.rs:L413-421
There was a problem at the user authentication step, since a None or Unknown method were not handled. That made the test at event::ServFirstAuth fail in any course of action:
In all those cases the runner expected having completed a an authentication method such as password or PubKey.
In the proposed fix this is handled by sending a failed request when a None or Unknown AuthMethod are received. That message will include the available authentication methods as it was already arranged in the library.