Quadlet - new unit type .login #26507
Conversation
openshift-ci
bot
added
release-note
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
There was a problem hiding this comment.
It is likely fine. Though I guess there is the gotcha that one cannot use unit specific specifiers like %n as this would get resolved to a different name in the contianer unit where --authfile would be added. Though stuff like %t would till be fine.
There was a problem hiding this comment.
My thought was that if the .login file doesn't specify an AuthFile then login info will be stored in the default file. So, the .container does not need to specify an AuthFile either, but it will need to specify the dependecy.
As for %n, you are correct. I'll add it to the doc
ygalblum
force-pushed
the
quadlet-login
branch
4 times, most recently
from
84997b1 to
7d8e147
Compare
|
|
||
| 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. |
|
@TomSweeneyRedHat I made the requested changes and added an example |
| ".login": { | ||
| Extension: ".login", | ||
| Order: 1, | ||
| ServiceFileMode: os.FileMode(0600), |
There was a problem hiding this comment.
maybe a small comment here why this is special could help.
pkg/systemd/quadlet/quadlet.go
Outdated
| ".pod": 5, | ||
| SupportedExtensions = map[string]ExtensionInfo{ | ||
| ".container": { | ||
| Extension: ".container", |
There was a problem hiding this comment.
Why is Extension a field if the key has already the same value? I don't see where this would be needed and liek this it just feels like it is wasting a bit of memory.
There was a problem hiding this comment.
Initially I thought we might get the struct and pass it around and then the Extension can be used. Since there's no usage of it, you're right and I removed it.
ygalblum
force-pushed
the
quadlet-login
branch
4 times, most recently
from
843c621 to
192b3cf
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>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, 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 |
|
@containers/podman-maintainers PTAL |
There was a problem hiding this comment.
I am new to this, but at a first glance, I don’t see any point to this. (I suppose this relates to #26484 ? the PR does not explicitly say.)
Most importantly, podman login is not magic and AFAIK there’s no need at all to run it on every boot, or regularly. As long as the credentials work, the AuthFile never needs to change.
In particular, for deployments like that, it should not be necessary to make any requests to the registry (and therefore, it should not be necessary to deal with TLS configuration).
Create an AuthFile, make it constant, deploy it with your application, done.
The discussion so far seems to center around keeping the password in plain text, and triggering a login on every boot. I think that’s generally inferior to just maintaining a pre-created AuthFile.
So why does a special login unit need to exist? Is it that Quadlet / auto-update finds it much easier to refer to systemd units than to mere files? (Something like that could well be the case, I am ignorant of the problem space. But then, fine, create a .login unit that only contains a parameter with an AuthFile path, and does not trigger any action.)
Or is that intended to explicitly trigger the registry login check, in order to stop early, and prevent creation of the actual container systemd units entirely?
[Hypothetically, a unit to generate an AuthFile might be interesting if there were some way to store credentials in a way that is protected from malicious access… but, eh, it’s unclear what “protected” means — what are the privilege boundaries? And systemd already contains a credentials mechanism. Anyway, this seems to not be that.
Design—wise, I find the “special Cases If the name of the AuthFile ends with .login” generally worrisome and aesthetically unpleasant.
Please just add a new option with independent semantics. We are not going to run out of option names; but we might break a user who happens to like naming AuthFiles myProject.login.
| | AuthFile=/etc/registry/auth\.json | --authfile=/etc/registry/auth\.json | | ||
| | CertDir=/etc/certs | --cert-dir=/etc/certs | | ||
| | LogoutOnStop=true | Add ExecStopPost to log out of the registry when the unit is stopped | | ||
| | Password=mypassword | --password-stdin and set StandardInputText | |
There was a problem hiding this comment.
I don't think Quadlet should block users from doing what podman allows them to
In many cases, the AuthFile is stored in a temporary FS and as a result logging in is required. Also note, this is not specific to boot time, it's upon demand.
I don't know what you mean by "deployments like that"
Yes, decoupling is a large part of it. But, your suggestion requires the user to do two things. This is even worse. BTW you could say the same about many other Quadlet units. Why do you need a
Quadlet users not only understand this design, they've learned to expect it. Cross referencing between Quadlet units is done this way all over. That's how |
Are you saying that the primary source is in a temporary storage (in that case, it probably can still use an auth-file format; just make a copy), or that the local system’s login state is in temporary storage (in that case, no need to log in again; at worst, just make a copy of an AuthFile-formatted input file — or better, point at the authoritative persistent storage directly without having a temporary-storage state)?
I think there needs to be a fundamental separation between sensitive-but-essentially-public-to-the-whole-company deployment configuration, and secrets; and it is irresponsible to design things that recommend erasing that separation.
*shrug* I still think it’s a bad idea, and the best time to stop with a bad idea is now. But it might be more productive for me to wander away again and to let Quadlet-experienced maintainers review this. |
|
The argument just use Authfile everywhere was my first impression too #26484 (comment) I think it is a fair point to say we don't need this unit, a user just has to run login themselves and ship the authfile next to the quadlet files like they would do for the login unit so I guess yeah technically there is no strong requirement for this. I am definitely totally ok with this approach here though I think there is one catch where this could be an advantage @mtrmac. If you set an authfile the password must be in clear text in said file on the persistent disk. With the login unit there is no strict requirement that the password is in clear text in said unit. The password could be passed as systemd creds (doesn't seem to work today due systemd/systemd#31213) but I could see that working eventually or we ad the Maybe it would be good to have some more feedback on this here from the others. |
Yes; at that point there would be a benefit to adding a new feature. [Well, better, the whole authfile could be passed as systemd creds, couldn’t it? Quadlet might still want a separate unit for the loading of the auth file from systemd credentials, so that the feature does not have to be added to every single other Quadlet unit; *shrug*, I know ~nothing about Quadlet and I have no opinion on that.] One general point to think about, “auth file” is really a much better point of abstraction than a “login” or a “password”, because an authfile can contain a whole set of credentials, e.g. for all mirrors. And using an auth file ~automatically scopes the credentials only for the service that uses the auth file, without polluting global state like a |
I think you are right and maybe it is even easier to do that. AFAIK systemd hands us a file so I would assume one could already just point the Authfile= key in each unit to that. As for sharing yeah I guess if we want to abstract then yes otherwise to is likely only two lines anyway.
Yeah I guess that is a valid point. The login unit would not really work in the use case of needing two different credentials. So you convinced me, users should be providing the authfile themselves. |
While this is true, it's relevant only in specific cases. For most cases, a single image is used.
No one forces users to use a Quadlet file (or Quadlet at all, for that matter). It's about facilitating the different options. To me it's simpler to ship a In terms of security, both options are not really secure. In both cases the credentials are stored in a known place in plain text. |
I would, conceptually, expect a single service (a single “set of privileges) to share an AuthFile, for however many images, repositories, mirrors, or registries, are involved.
The way I think about it, shipping a |
yeah the more I think about this I agree. Also simple is relative on the target audience. Maybe some users would prefer it, hard to know? In terms of podman maintainer hat not having a feature in podman makes our lives easier so if there is no significant new value then maybe it is not the best to commit to supporting that long term.
You can just all well update the authfile inplace, directly. |
|
Just to be explicit, because I’m not 100% sure we are all talking about the same
|
Not sure I understand what you mean by this.
I don't see anyone doing this. |
I don’t see how 1. is really more convenient, but I am not working on operations and I wouldn’t know. |
|
The difference that I see
|
|
I think $myApp.authfile can be treated about the same as $myApp.login: ship a template file (well, an empty file would be best here). And use whatever distribution mechanism is in place to deploy the half-a-dozen Sure, the |
|
I think I'm missing something in what you're saying. How can one ship an authfile? It has to be created using the user's credentials. Maybe you're thinking about closed OS images (e.g. fleets or something like that) |
|
I don’t know. How can one ship a |
|
When I said "ship a As you wrote the |
|
I’m naive about these things. In my mental model, all of those Users work in that git repo, or whatever it is. Paths on the final machine don’t matter, the deployment scripts handle that. Or, alternatively, all the In none of these situations are users thinking about the paths at application deployment time. The paths are always a design-time concern, just like systemd unit names. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Does this PR introduce a user-facing change?
Yes