-
Notifications
You must be signed in to change notification settings - Fork 135
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
Errant async_method_call in http router #157
Comments
@edtanous How to set up ldap service so that users in ldap can log in successfully |
Ideally we would use Pam in async mode. I’ve never had time to do that, but it on my list. The phosphor user manager client needs to make sure that the new user is added to dbus before the Pam call is allowed to complete. After that, bmcweb should have a record of it, and it should go through just fine. worst case, if we get a weird ordering problem (which I don’t think we would), we need to split the auth up and use an io_context::post to put the post-login activities after the messages come through and are processed. |
@edtanous The problem I have now is that my attempt to log in to the user in the ldap service fails, and I don’t know if my connection to the ldap service is successful |
@edtanous How to detect whether the connection to the ldap service is successful |
If the LDAP service fails to connect, the PAM conversation (in authenticate_pam) should fail, indicating the user isn't authenticated, and bmcweb should return 401. If bmcweb authenticates via PAM, and the ldap server disconnected, that's a bug in the pam implementation that needs fixed. |
@edtanous I did not see the verification of pam. I connected to the ldap service on the web. It seems to be through /redfish/v1/Managers/bmc/Truststore/Certificates/,I want to build ldap service now, can you give me some suggestions, thank you very much |
@edtanous Still have trouble asking, the web interface can automatically generate the ldap service, you don’t need to build the ldap server yourself, is this okay? |
if you want to use LDAP, first you need to build a bmc with ldap support. That's outside the scope of this bug. |
@edtanous I’m very sorry, I don’t know if my bmc supports ldap function, it should be reset to support bmc, so I just tried it, please ask, how to detect whether bmc has ldap function,thank you very much |
This is a discussion much better had on the openbmc mailing list. This bug is unrelated to your question. |
@edtanous I'm really sorry, I really don't have a clue about the ldap service, can I trouble you to share your general steps,Many thanks |
Steps:
Alternatively, you could also sign up for discord, where we have a server set up (look in the docs repo for details). Either of those are better forums to ask questions than this unrelated bug. |
@edtanous Thank you very much for your answer |
Note, the safety issues in this call did cause some problems recently. We still need to come up with a better solution to this. One possible fix that was at one point reverted is here: |
In the HTTP router, there appears to be this async_method_call;
bmcweb/http/routing.h
Line 1296 in 3fad0d5
This is causing a dbus method call for every request. To my understanding, this was added because of LDAP support needing to fill in an unknown users role. That's fine, but causing a performance impact to all requests, not just those systems relying on LDAP, or when LDAP is enabled is a problem, and needs resolved.
Other notable issues:
The routing layer/file should have nothing to do with looking up a users role. At a minimum this is the wrong place to put the code.
It forces a dependence on phosphor-user-manager and Dbus. bmcweb is used in contexts without dbus or phosphor-user-manager (ie, openrmc). While that project is a fork, I'd like to see them joined at some point, and the more openbmc-specific restrictions we impose at the framework level, the harder that becomes.
req, res, and rules are all captured by reference, which might lead to lifetime issues if the connection is closed while the UserRole lookup is occurring.
std::map is less efficient than could be used.
I have very little context on what this is needed for. Does anyone have any suggestions on how these could be resolved?
It should be noted, this was checked in as a "fix"
61dbeef
The code was arguably better before, as it didn't cause a performance impact to users not using ldap, but the commit message is not clear about why exactly we had to move to what is effectively a block call.
The text was updated successfully, but these errors were encountered: