[redhat] Change authentication method for RHEL #3380
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
1 similar comment
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
2 times, most recently
from
f5c8717 to
c3f8413
Compare
|
Some notes:
Traceback (most recent call last): This behaviour is copied from other similar approaches in other classes, but I can see that the last line may not be clear enough. I can change that for a simple message if needed.
|
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
2 times, most recently
from
d80f678 to
f6de503
Compare
|
I should learn about
` ` and I get this permanently. I bet I did an unsupported action :), just are we OK if above uncaught exceptions will pop up? (should not we just log them and abort the upload..?) (just a side note: nice work with the |
I'll check this. I'm interested to know if the problem goes away when you get a login into a new session, but it looks like you cannot upload sos anymore after this error?
I think the code should be robust enough to deal with the user explicitly removing session keys. So I'll rework the code to deal better with this and upload again. Thank you for testing! |
|
The tests on Ubuntu are repeatedly failing with e.g.: but the log archive does not contain any details (esp. traceback is missing). Can somebody with Ubuntu at hand check it? |
sos/keyring.py
Outdated
There was a problem hiding this comment.
This whole bit is rather over-engineered for what is ultimately needed - saving a short-lived expiring token. If there were other use cases at hand, perhaps this would be more viable, but there aren't. Beyond that, it adds an otherwise unnecessary dependency and operates on shell-out, which is something this project has actively and consciously tried to reduce in recent years in favor of native-python wherever possible (see our compression and decompression flows, for instance). A far more direct and more maintainable approach would be to simply save the token, salt it with whatever unique-to-the-host information we desire, and save it to disk in a well-known and secured location. These aren't massively sensitive keys we're dealing with.
There was a problem hiding this comment.
The original approach was to use local storage, but the Red Hat security team asked us to use session-keys via keyctl in both redhat-suppor-tool and sos, and that's the reason why we used keyctl.
Actually it looks like there's not much we can do after revoking. I.e.: The only way to add keys or do anything else with the keyring seems to be to log out and in again, so the session is re-established. But I think I can improve the error log when the revoke occurs. |
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
2 times, most recently
from
2195a0e to
dfa4638
Compare
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
from
dfa4638 to
74c5f90
Compare
|
I've done some tests with Ubuntu and snaps and after reproducing the issue that was failing the tests, I think I managed to solve the problem (at least, locally). |
|
One user experience question): Current flow is: So we ask for auth token before we even start to generate the report. If generating the report takes say 10 minutes, then the token might get expired ( |
[...]
This is a very good point. I think we have two, maybe three options: |
|
I was thinking on this to some extent and there are two opposite approaches: either move any upload querying steps after sosreport is generated (logically only then they are needed) - or insist on the approach "during interactive session (no The "move user interaction wrt upload to the end" is option b), the "ask for everything at the beginning" is the option a). No strong preference here, either a) or b) is good to me. Just b) should be coherent in either scenario, like asking for username/password for upload when basic auth is required - that would be extra nontrivial change. So I lean a bit to increasing the timeout here (and optionally do the user interaction change altogether later on, if we decide so?). |
I think there may be an easier solution. I'm testing it now. |
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
from
74c5f90 to
ac4d3da
Compare
|
New version pushed with:
|
|
Now, lets see if I can answer your points Jake:
You are completely right - this class is basically implementing a whole framework to use keyctl. Unfortunately, the requirements we got don't give us much room. Basically the main requirement is to use in-memory token storage instead of local storage, and we can do that only with keyctl. The problem with this is, there's no keyctl native-python implementation we can use, so we have to do everything from scratch. But also, it is a bit more than just saving a short lived expiring token. Since we have to use the Linux kernel keyring, we need to be able to save keys, update them, remove them, set timeouts, set permissions, search, etc. That we have no native way to do this, and we need to call shell commands instead, is very unfortunate.
If the unnecessary dependency that you mention is about having keyutils installed, that dependency is only enforced when an user wants to upload the report from a RHEL system. In any other scenario, the dependency doesn't exist.
That's part of the problem - as I mentioned, there's no native-python available. There was a keyctl project (https://pypi.org/project/keyctl/ ) that as far as I know, applies to python 2.7. If there's a native python keyctl that we can use available somewhere, that I've missed, I'm more than happy to discard this class.
We've been specifically requested not to store the tokens locally in the filesystem, and the alternative suggested was storing the tokens in memory via the Linux Kernel keyring. I acknowledge that this is not the best implementation possible, but also that's the best we can do with the restrictions we've been imposed when it comes to managing tokens and uploading sos reports to the Red Hat Customer Portal. Hopefully this is more a temporary solution while we get some native keyctl in python, and when that happens I'll send a PR removing the whole class. |
|
As this is a large item I will need to set aside some time to review it later. |
There was a problem hiding this comment.
My specific concerns follow below, however I am still not wanting to add more shell-out reliance for the reasons I've stated previously.
Shell-out by nature is fragile and prone to breaking, at least moreso than if a proper API was leveraged for something like this. If usingkeyctl is a hard requirement from some external (to sos that is) team, then that team should provide an alternative that isn't simply painting over the lack of proper support for the requested function.
sos/policies/distros/redhat.py
Outdated
| _("The option --upload-user has been deprecated in favour" | ||
| " of device authorization") |
There was a problem hiding this comment.
Add a specific note that this is for RHEL here, not sos generally.
sos/policies/distros/redhat.py
Outdated
| _("The option --upload-pass has been deprecated in favour" | ||
| " of device authorization") |
There was a problem hiding this comment.
Ditto, this deprecation is RHEL-specific.
sos/policies/auth/keyring.py
Outdated
| # This class is based on the approach from redhat-support-tool, which in | ||
| # turn is based on https://github.com/tuxberlin/python-keyctl/tree/master |
There was a problem hiding this comment.
Please provide a reference to redhat-support-tool code for this.
There was a problem hiding this comment.
The code hasn't been merged yet, it's still under review, but as soon as it's done I'll reference it here.
sos/policies/auth/keyring.py
Outdated
| # This class is based on the approach from redhat-support-tool, which in | ||
| # turn is based on https://github.com/tuxberlin/python-keyctl/tree/master |
There was a problem hiding this comment.
After very brief reviewing, this is nearly a complete lift from python-keyctl. In many places it is a line for line copy - the identifiers, methods, flow, naming convention, etc... are largely identical.
I realize this tool is Open Source, but it seems disingenuous to simply say whatever follows is "based on something that is based on something else" when there is clearly more at play there. While this may not run afoul of the letter of the GPL, to me it walks a very fine line on the spirit of it. However, that is then complicated by the decision to slap a copyright line on this which I am uncomfortable with to say the least.
I am unwilling to move the project forward with this until this gets cleared up by @sosreport/team-red-hat
There was a problem hiding this comment.
Indeed, as I referenced before, https://github.com/tuxberlin/python-keyctl and https://pypi.org/project/keyctl/ are the original sources of the keyctl implementation in Python. It was written with Python 2.7 in mind, so we couldn't use the code directly in redhat-support-tool and in sos. The project has been abandoned for some time, but the code works for the most part - it doesn't make sense to reinvent the wheel, given this context.
Could you please clarify what do you mean when you say that there is clearly more at play there? The code has been adapted to this PR, and at the same time where it was taken from has been referenced. There has never been any intention to hide where the code was coming from originally, as you can see by my comments above. If there's a better way to reference the original code and author, I'm more than happy to change that - I'm still learning, and any suggestion will be very welcome, as always. For example, would adding the original creator in the copyright line be enough?
I look forward to further input from my colleagues in @sosreport/team-red-hat
As I mentioned previously, this is not supposed to be a permanent change - rather, it's a first step or stage, and we want to use sos builtins to change the functon _system() in keyring.py for something closer to the sos way and style. But it is going to take a bit of time implementing and testing to adapt what currently exists in sos code. On the other hand, I see this situation very similar to the upload_sftp() function in sos/policies/distros/init.py, where it's stated that: In our case the approach taken by the use of _system() inside keyring.py is "due to the lack of well maintained, secure, and generally widespread python libraries for keyctl, sos will need to shell-out to the system's local keyctl installation in order to handle these authorizations."
There are no alternatives to this, unfortunately. We've been investigating how to make this new requirement for uploads work in a sos way, and the only way for now is to take the old code for keyctl and adapt it (providing appropriate atribution to the original author and code) to our needs at the moment. Without this change, customers using RHEL won't be able to upload files to support. |
And we are here in really very similar situation like with And my gut feeling is, Also, from user perspective,
From a constructive perspective: what approach would be acceptable? So far we have heard just "not this way" pieces of feedback while we sometimes don't see a better approach. And all is for a request "store tokens via (*) I mean: the current PR is acceptable though maybe not ideal. If there is something better, let use it. If nobody sees a better solution, let use the current one. Also, another angle to view this PR: it is for a use case that affects just Red Hat support. All (three) people involved in this PR from Red Hat support sees / acknowledges the implementation as good enough. No harm outside Red Hat can happen, right..? |
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
3 times, most recently
from
79ed451 to
778fa5a
Compare
|
Latest update - there's no keyring.py anymore. With this PR, we don't store tokens at all, and as a result users will need to request a new one every time they want to upload to Red Hat Customer Portal. |
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
from
778fa5a to
774d275
Compare
| # We may have a device token already if we attempted | ||
| # to upload via http but the upload failed. So | ||
| # lets check first if there isn't one. |
There was a problem hiding this comment.
Here we assume http upload is the default, and we can failover from http to sftp only (but not vice versa). That is correct assumption and I dont expect something changes here - just in (the purely speculative) case we change the default to "sftp, if fails then http", we would have to modify this code (or move else-where to upload_http).
ACK as is now, just a comment about hidden assumption the code relies on (and works fine, as I tested).
|
Just the two instances of redundant code, otherwise all good. Code looks good, all tests of all variants "let break this in the middle of that user story" passed. @TurboTurtle , could you please review it now? :) Thanks a lot in advance. |
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
from
774d275 to
5c2c73f
Compare
|
Marking for later review. |
The authentication method for RHEL uploads to the customer portal is changing in 2024 to Device Auth tokens, from user/password basic authorization. To accomplish this, one new class is created: DeviceAuth (deviceauth.py), that takes care of managing OID token authentication. Closes: RH: SUPDEV-63 Signed-off-by: Jose Castillo <jcastillo@redhat.com>
jcastill
force-pushed
the
jcastillo-update-auth-redhat
branch
from
5c2c73f to
c1a0848
Compare
|
Could we get this into 3.7, please? What it is blocked by? |
FYI the feature has been successfully verified by a few independent people within Red Hat, in various scenarios we have figured out. So for the RedHat specific stuff, we can grant it will work well. The feedback about over-engineering and shell out has been accepted - the current proposal does not follow that approach. Just a different call flow (with working with tokens returned from RH API but not even storing them to the disk or other tool) is the matter of the PR. The changes behind the PR are of a medium size. Something the community has provided some feedback within a few weeks almost every time in past. Could you @arif-ali or @TurboTurtle review the PR or state if it is feasible to get it to 4.7 ? Or any other ETA of a review / feedback / whatever? I dont understand the silence here.. |
|
I simply do not have the same amount of time to spend on reviews as I previously had, as my professional priorities have been changed on me. I can probably look through this later this week. |
|
Fixed by dd2e395. |
The authentication method for RHEL uploads to the
customer portal is changing in 2024 to Device Auth
tokens, from user/password basic authorization.
To accomplish this, one new class is created:
DeviceAuth (deviceauth.py), that takes care of
managing OID token authentication.
Closes: RH: SUPDEV-63
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines