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

Do not expose the testing IdP implementation #2072

Closed
matrss opened this issue Nov 7, 2023 · 9 comments · Fixed by #2289
Closed

Do not expose the testing IdP implementation #2072

matrss opened this issue Nov 7, 2023 · 9 comments · Fixed by #2289
Assignees
Labels
mscolab SAML2 feature of 9.0 improvements
Milestone

Comments

@matrss
Copy link
Collaborator

matrss commented Nov 7, 2023

- msidp = mslib.msidp.idp:main

Do we really want to expose the IdP that was implemented? I still consider this to be testing infrastructure that should be moved to tests/ and be used as a lightweight target in integration tests of the SP functionality only.

Originally posted by @matrss in #2069 (comment)

@matrss matrss added the SAML2 feature of 9.0 improvements label Nov 7, 2023
@nilupulmanodya
Copy link
Collaborator

@matrss When we move IdP tests/, it means we are no longer offering testing IdP and guidance to the user. When I think about the user's end, it can be a bit confusing, especially for users configuring first time with 3rd party IdP who are not familiar with configurations and use cases with SAML2 authentication. What are your insights on the user's perspective? If you think we no longer need further guidance, please assign me I will work on this issue.

@matrss
Copy link
Collaborator Author

matrss commented Jan 22, 2024

When we move IdP tests/, it means we are no longer offering testing IdP [...]

We would still offer it under tests/, but not as part of the published package e.g. installable from conda-forge. I consider this IdP implementation to be a lightweight one that would be easy to spin up in our tests (albeit we don't have any using it, yet), but not one we should expose into a production setting since it is limited in its functionality and not thoroughly checked for potential security issues.

[...] and guidance to the user. When I think about the user's end, it can be a bit confusing, especially for users configuring first time with 3rd party IdP who are not familiar with configurations and use cases with SAML2 authentication. What are your insights on the user's perspective?

I don't think the testing IdP implementation has that much value in this regard, anyway. If a mscolab server admin (other, less technical, users won't interact with this anyway, I'd assume) wants to integrate mscolab with a SAML IdP, then I'd expect the documentation integrating with keycloak (https://github.com/Open-MSS/MSS/blob/develop/docs/sso_via_saml_mscolab.rst) to be of more value, since that is closer to a real-world IdP they would encounter.

This is just my opinion though. Maybe @ReimarBauer has a different view, e.g. from integrating MSColab with the Helmholtz AAI?

@matrss matrss added this to the 9.0.0 milestone Feb 16, 2024
@ReimarBauer
Copy link
Member

ReimarBauer commented Feb 16, 2024

it is not helping a user when we have it under tests. This is something similiar to the werkzeug/flask web server. We have to add a big hint simliar to that example.

INFO: WARNING: This is a development server. Do not use it in a production deployment. Use a production e.g. keycloak server instead.

For the next major I want to keep it as it is. Of course with the message. On a further step we maybe can implement auth plugins.

@matrss
Copy link
Collaborator Author

matrss commented Feb 16, 2024

it is not helping a user when we have it under tests.

I don't think it is helping a user where it is now either, is my point. So why expose a msidp entrypoint in the package?

On a further step we maybe can implement auth plugins.

What? If you mean what I think you mean, then please not. There is no point in re-implementing an IdP here.

@ReimarBauer
Copy link
Member

You are right that it is only something for the developer, but with the same arguments there won't be a flask werkzeug wsgi server available.

"""
When running publicly rather than in development, you should not use the built-in development server ( flask run ). The development server is provided by Werkzeug for convenience, but is not designed to be particularly efficient, stable, or secure.
"""

So we should answer if it helps a developer and if it is not an entrypoint how the results of the documentation can be archieved.

@ReimarBauer
Copy link
Member

with auth plugins I meant more the server part lines, this did not scale when there now the next e.g. ldap needs routes etc to be used with us.

@ReimarBauer
Copy link
Member

ReimarBauer commented Feb 18, 2024

Anyway I think we need to refactor it to avoid problems

When it gets deployed the hard coded passwords have to become removed.

is there a reason PASSWD is different.

  • move the PASSWD section of idp_uwsgi.py to some place outside the deployed code and make it possible to import e.g. from a idp_settings in the PYTHONPATH
  • move the PASSWD section of idp.py to some place outside the deployed code and make it possible to import e.g. from a idp_settings in the PYTHONPATH
  • idp_user may become also a class into the idp_settings
  • the idp_settings can go to docs /samples/config/idp/ and into our testconfiguration.

When it is run by a user it should show a big hint, that it is not for production, and without the configuration it should not crash.

@matrss
Copy link
Collaborator Author

matrss commented Feb 19, 2024

with auth plugins I meant more the server part lines, this did not scale when there now the next e.g. ldap needs routes etc to be used with us.

Ah OK. I don't think this is much of a priority though, I am pretty sure LDAP wouldn't require these types of endpoints on our end and would instead rely purely on LDAP queries coming from our side. Other than SAML and OIDC, there is nothing relevant in this space, and an entire plugin system for two types of auth is overkill, IMO.

When it gets deployed the hard coded passwords have to become removed.

For testing purposes it doesn't matter, for anything else this shouldn't be used anyway. I don't see a purpose in making this configurable. A single hardcoded <username>:<password> combination would suffice for testing purposes and as soon as a password needs to be changed this is a clear sign that this IdP should no longer be used, IMO.

When it is run by a user it should show a big hint, that it is not for production

Yes.

@ReimarBauer
Copy link
Member

we don't need to move the userata yet, we have something similiar in the example seed database for mscolab.

At the moment an enterprise scanner annoyes me I change my mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mscolab SAML2 feature of 9.0 improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants