Quadlet - new unit type .login #26507
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
ygalblum
force-pushed
the
quadlet-login
branch
2 times, most recently
from
004b64f to
b0edc68
Compare
pkg/systemd/quadlet/quadlet.go
Outdated
|
|
||
| stringKeys := map[string]string{ | ||
| KeyCertDir: "--cert-dir", | ||
| KeyPassword: "--password", |
There was a problem hiding this comment.
This is insecure so we should not do that as it is not any better than before, it is the same issue as I described in #26484 (comment).
Security relevant stuff should never be passed as on cli argument. I think what this must do is a pass --password-stdin. Then in the systemd unit configure
StandardInput=data
StandardInputText=password
And lastly I am not sure on the perms of current generated files but we should make sure they are 600 so no other user could read the password from there.
There was a problem hiding this comment.
This is insecure
While I understand your comment, Quadlet already does something similar in .image files: https://docs.podman.io/en/latest/markdown/podman-systemd.unit.5.html#creds.
I don't expect users to use it with real registries. But, do we need to block it? We can add a warning in the man page.
I think what this must do is a pass
--password-stdin
I don't see the benefit here. Isn't the password still leaked just under a different key? What am I missing?
And lastly I am not sure on the perms of current generated files but we should make sure they are 600 so no other user could read the password from there.
Currently, all service files are created with 644. Is your comment about permissions specific to the Password key or as a whole to all .login units?
There was a problem hiding this comment.
Yes but again this is horrible security practise so we must discourage it. The login unit should be a solution and not do the exact same error again. The issue is that running podman login --password mypw leaks the password to all users who can view the process list which by default is everyone.
My solution is more secure because the password is never passed via cli argument but rather stdin which is "private". The permissions on the .login file are up to the user who creates them and only something quadlet could warn. But the permissions on the generated -login.service file is up to us so we must ensure the service file is only accessible by the same user because the service also has the password in clear text.
There was a problem hiding this comment.
Not sure I follow why using StandardInputText is better. Doesn't it just mean that instead of:
[Login]
Password=password
The .login unit will have:
[Login]
PasswordSTDIN=yes
[Service]
StandardInput=data
StandardInputText=password
Which also means that it will be copied to the .service file
From what I can see, the actual podman login command does not show in the journal. So, it's not leaked:
Jun 25 08:28:47 lima-default systemd[1001]: Starting image_test_t1-wmsnex2o-login.service...
Jun 25 08:28:47 lima-default image_test_t1-wmsnex2o-login[894538]: Login Succeeded!
Jun 25 08:28:47 lima-default systemd[1001]: Finished image_test_t1-wmsnex2o-login.service.
There was a problem hiding this comment.
Any user can just run ps auxww and see all the process currently running and does the password is leaked to all users. For a short lived command such as podman login it is hard to actually hit the right window to see it but if you try hard enough and run ps in a loop you might get lucky. Certainly not a secure.
There was a problem hiding this comment.
OK. So, I'll remove the support for Password and replace it with PasswordSTDIN and document the requirement for setting StandardInputText. Quadlet will not verify the existence of StandardInputText when PasswordSTDIN is set because it can be set in a drop-in.
As for permissions, the question is on the generated .service files. Do you think that all .service files generated for .login units should have 600? Only the ones with PasswordSTDIN? Not change the implementation at all and warn the user in the doc?
There was a problem hiding this comment.
To be clear you don't have to remove support for "Password" as quadlet field. What I try to say here is that we (quadlet) internally map the Password to mean set --password-stdin and
[Service]
StandardInput=data
StandardInputText=<value of Password field>
The user should not need to worry about the implementation details but to me it is very important to not accidentally leak an important secret as cli arg.
There was a problem hiding this comment.
I've adjusted the code, the tests and the documentation with your recommendations
| # Use the same directory for all quadlet files to make sure later steps access previous ones | ||
| mkdir $quadlet_tmpdir | ||
| # Shutdown the login service to logout and let the dependency kick in | ||
| service_cleanup $login_service inactive |
There was a problem hiding this comment.
there is the podman login --authfile $authfile --get-login $registry command you could run to check if you are logged into the registry. which should fail as the service stop should run logout
There was a problem hiding this comment.
Thanks. I added checked around starting and stopping of the login service (directly and indirectly)
|
|
||
| Path of the authentication file. | ||
|
|
||
| This field is mandatory when linking between an `AuthFile` key of a different unit and the `.login` unit. |
There was a problem hiding this comment.
would it make sense to instead default to %t/<unit-name>?
There was a problem hiding this comment.
Good question, I'm not sure.
Not setting one, means that the login information is stored in the default file. So, units can depend on on the .login unit (in the Wants and After) without setting AuthKey.
But, maybe it is cleaner
ygalblum
force-pushed
the
quadlet-login
branch
3 times, most recently
from
110eafe to
84997b1
Compare
Implement a new unit for podman login Add support for AuthFile to container and kube units Allow linking between AuthFile and a login unit Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
|
|
||
| This field is mandatory when linking between an `AuthFile` key of a different unit and the `.login` unit. | ||
|
|
||
| This is equivalent to the `--authfile` option. |
There was a problem hiding this comment.
| This is equivalent to the `--authfile` option. | |
| This is equivalent to the Podman `--authfile` option. |
In some places, you have Podman for a line like this, in others, you do not. I prefer having Podman for each. However, your call, I'd just ask for consistency.
|
|
||
| ### `LogoutOnStop=` (defaults to `false`) | ||
|
|
||
| When set to `true` the logout will be called when the service is stopped |
There was a problem hiding this comment.
| When set to `true` the logout will be called when the service is stopped | |
| When set to `true`, the logout will be called when the service is stopped. |
|
|
||
| This key contains a list of arguments passed directly to the end of the `podman login` command | ||
| in the generated file. It can be used to access Podman features otherwise unsupported by the generator. Since the generator is unaware | ||
| of what unexpected interactions can be caused by these arguments, is not recommended to use |
There was a problem hiding this comment.
| of what unexpected interactions can be caused by these arguments, is not recommended to use | |
| of the unexpected interactions that can be caused by these arguments, it is not recommended to use |
| this option. | ||
|
|
||
| The format of this is a space separated list of arguments, which can optionally be individually | ||
| escaped to allow inclusion of whitespace and other control characters. |
There was a problem hiding this comment.
It would be nice to add some examples at the bottom of this page. Not a necessity.
|
|
||
| ### `Secret=` | ||
|
|
||
| Use a podman secret for the credentials |
There was a problem hiding this comment.
| Use a podman secret for the credentials | |
| Use a podman secret for the credentials. |
Does this PR introduce a user-facing change?
Yes