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

Reef: mgr/dashboard: Simplify authentication protocol #55689

Merged
merged 3 commits into from Feb 23, 2024

Conversation

mcv21
Copy link
Contributor

@mcv21 mcv21 commented Feb 21, 2024

This is a backport of #54710

Which is a partial fix for https://tracker.ceph.com/issues/63529

While it's not a total fix, it will enable the dashboard in reef on systems with a recent PyO3 (e.g. Debian Bookworm, Arch), so I think is a useful backport for reef.

Conflicts:
All three commits cherry-picked cleanly with no conflicts.

@mcv21 mcv21 requested a review from a team as a code owner February 21, 2024 11:50
@mcv21 mcv21 requested review from afreen23 and nizamial09 and removed request for a team February 21, 2024 11:50
@github-actions github-actions bot added this to the reef milestone Feb 21, 2024
Copy link
Member

@nizamial09 nizamial09 left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of it. I wanted to do this for a while but been busy with other stuffs..
lgtm apart from one comment. Was there any conflict you faced when you did the cherry-picking?

src/pybind/mgr/dashboard/tox.ini Outdated Show resolved Hide resolved
@mcv21
Copy link
Contributor Author

mcv21 commented Feb 22, 2024

Thank you for taking care of it. I wanted to do this for a while but been busy with other stuffs.. lgtm apart from one comment. Was there any conflict you faced when you did the cherry-picking?

No, there were no conflicts cherry-picking the three commits from the original PR.

By removing the dependency to PyJWT we also remove the dependency to the cryptographic library which
in the dashboard module will create a crash. In newer implementations of the library PyO3 is used to run
rust code in order to encrypt with Elliptic Curves. This is never used in the dashboard communication so
a much simpler implementation where we only use the hmac sha256 algorithm to create the signed JWT message
could be used.

Fixes: https://forum.proxmox.com/threads/ceph-warning-post-upgrade-to-v8.129371
Signed-off-by: Daniel Persson <mailto.woden@gmail.com>
(cherry picked from commit c616a9d)
Move the JWT requirement to the test requirements file. Also remove JWT from ceph specification and debian build.

Signed-off-by: Daniel Persson <mailto.woden@gmail.com>
(cherry picked from commit c1ea66f)
Seemed that the test dependencies was separated in two different requirements files
one for the testing and one for linting. Added the JWT dependency in the linting file
as well.

Signed-off-by: Daniel Persson <mailto.woden@gmail.com>
(cherry picked from commit 06765e6)
@mcv21
Copy link
Contributor Author

mcv21 commented Feb 22, 2024

jenkins retest this please

@mcv21
Copy link
Contributor Author

mcv21 commented Feb 22, 2024

@nizamial09 I don't know why the Pull Request Triage job is failing ("Could not resolve to a ProjectV2 with the number 2.") but I think that's a CI thing rather than a problem with the MR.

@nizamial09
Copy link
Member

@nizamial09 I don't know why the Pull Request Triage job is failing ("Could not resolve to a ProjectV2 with the number 2.") but I think that's a CI thing rather than a problem with the MR.

you can ignore that failure (i remember fixing it in the main branch before and probably missed to backport it in other branches). once the other required checks like make check and api tests are passed, I'll merge this. thank you!

@mcv21
Copy link
Contributor Author

mcv21 commented Feb 22, 2024

@nizamial09 I'm not sure about the dashboard tests - I don't have permissions to view the images from the dashboard test job, and the e2e error log looks like a docker issue rather than a problem with this MR...

@nizamial09
Copy link
Member

I'm not sure about the dashboard tests - I don't have permissions to view the images from the dashboard test job, and the e2e error log looks like a docker issue rather than a problem with this MR...

the failures are known. so its okay. I'll trigger a shaman run just to make sure we are on the safe side here

@nizamial09
Copy link
Member

nizamial09 commented Feb 22, 2024

shaman build passed ✔️

@nizamial09 nizamial09 merged commit 33d8bef into ceph:reef Feb 23, 2024
10 of 13 checks passed
bazaah added a commit to bazaah/aur-ceph that referenced this pull request Mar 16, 2024
Removes the (limited) runtime usages of python-jwt, and therefore,
python-cryptography from the mgr dashboard module.

This should restore dashboard functionality, if used alongside a
patched pyo3 for python-bcrypt.

Upstream-Ref: ceph/ceph@33d8bef
References: ceph/ceph#55689
References: https://tracker.ceph.com/issues/63529
References: #20
References: https://git.st8l.com/luxolus/pyo3/commit/44d6919168621b46e4e884130ca43338655b020c
bazaah added a commit to bazaah/aur-ceph that referenced this pull request Mar 18, 2024
Removes the (limited) runtime usages of python-jwt, and therefore,
python-cryptography from the mgr dashboard module.

This should restore dashboard functionality, if used alongside a
patched pyo3 for python-bcrypt.

Upstream-Ref: ceph/ceph@33d8bef
References: ceph/ceph#55689
References: https://tracker.ceph.com/issues/63529
References: #20
References: https://git.st8l.com/luxolus/pyo3/commit/44d6919168621b46e4e884130ca43338655b020c
bazaah added a commit to bazaah/aur-ceph that referenced this pull request Mar 23, 2024
Removes the (limited) runtime usages of python-jwt, and therefore,
python-cryptography from the mgr dashboard module.

This should restore dashboard functionality, if used alongside a
patched pyo3 for python-bcrypt.

Upstream-Ref: ceph/ceph@33d8bef
References: ceph/ceph#55689
References: https://tracker.ceph.com/issues/63529
References: #20
References: https://git.st8l.com/luxolus/pyo3/commit/44d6919168621b46e4e884130ca43338655b020c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
3 participants