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

Errant async_method_call in http router #157

Open
edtanous opened this issue Oct 20, 2020 · 14 comments
Open

Errant async_method_call in http router #157

edtanous opened this issue Oct 20, 2020 · 14 comments

Comments

@edtanous
Copy link
Contributor

edtanous commented Oct 20, 2020

In the HTTP router, there appears to be this async_method_call;

crow::connections::systemBus->async_method_call(

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.

@edtanous edtanous reopened this Dec 15, 2020
@kingzmm
Copy link

kingzmm commented Jan 14, 2021

@edtanous How to set up ldap service so that users in ldap can log in successfully

@edtanous
Copy link
Contributor Author

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.

@kingzmm
Copy link

kingzmm commented Jan 14, 2021

@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

@kingzmm
Copy link

kingzmm commented Jan 14, 2021

@edtanous How to detect whether the connection to the ldap service is successful

@edtanous
Copy link
Contributor Author

edtanous commented Jan 14, 2021

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.

@kingzmm
Copy link

kingzmm commented Jan 14, 2021

@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

@kingzmm
Copy link

kingzmm commented Jan 14, 2021

@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?

@edtanous
Copy link
Contributor Author

if you want to use LDAP, first you need to build a bmc with ldap support. That's outside the scope of this bug.

@kingzmm
Copy link

kingzmm commented Jan 14, 2021

@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

@edtanous
Copy link
Contributor Author

This is a discussion much better had on the openbmc mailing list. This bug is unrelated to your question.

@kingzmm
Copy link

kingzmm commented Jan 14, 2021

@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

@edtanous
Copy link
Contributor Author

Steps:

  1. Google "openbmc mailing list"
  2. Sign up for mailing list.
  3. Read rules for mailing list.
  4. Send email to mailing list, asking your questions.

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.

@kingzmm
Copy link

kingzmm commented Jan 14, 2021

@edtanous Thank you very much for your answer

@edtanous
Copy link
Contributor Author

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:

https://gerrit.openbmc.org/c/openbmc/bmcweb/+/56081

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

No branches or pull requests

2 participants