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

RFC: kickstart: only call authselect if explicitly requested #1410

Closed
wants to merge 1 commit into from

Conversation

jlebon
Copy link
Contributor

@jlebon jlebon commented Mar 27, 2018

The system image might have a curated version of nsswitch.conf that we
don't want to overwrite. Rather than indeterministically calling it
based on whether fprintd is in the tree, let's just only call it if it
was explicitly asked for in the kickstart.

See also: https://github.com/pbrezina/authselect/issues/48.

@jlebon
Copy link
Contributor Author

jlebon commented Mar 27, 2018

(Untested for now.)

@jlebon
Copy link
Contributor Author

jlebon commented Mar 27, 2018

Tested as fixing FAW 28 Beta.

@cgwalters
Copy link
Contributor

The whole situation where authselect is sort of designed for an admin to explicitly invoke, but sometimes we invoke it behind their back in mysterious ways feels pretty broken.

The code here was added for a reason though - you're basically reverting it. And then people clicking through interactive workstation installs then won't get fprintd enabled.

What would seem a lot more sane to me is that if fprintd is installed it's enabled via some sort of drop-in automatically, no "authselect" required. But authselect could be used to explicitly disable it if installed.

@AdamWill
Copy link
Contributor

This doesn't feel right from the PoV of actually doing what this code was originally intended for, which was: enabling fingerprint authentication by default. Heck, that's still written in a comment line in your code:

# Enable fingerprint option by default (#481273).

but it doesn't actually do that any more, does it? It'd only do it if it happened to be using a kickstart with an authconfig line, so not "by default" at all. The conditional also looks to be clearly wrong in the new context - why would be be checking whether this is an 'automated' install or not, in that context? Basically, the whole "move that part into the if security_proxy.Authconfig: conditional" part of the PR looks entirely bogus to me.

In an ideal world I think I'd want to just drop this chunk entirely (i.e. just the first part of the PR) and figure out a better way to enable fingerprint auth by default, if we think we still want that. It seems weird in the first place that this was ever done by having anaconda run authconfig in the installed system root.

The system image might have a curated version of `nsswitch.conf` that we
don't want to overwrite. Rather than indeterministically calling it
based on whether `fprintd` is in the tree, let's just only call it if it
was explicitly asked for in the kickstart.
@jlebon
Copy link
Contributor Author

jlebon commented Mar 27, 2018

Right, this was meant as a partial revert of #1327. I.e. we still do run authselect if a line is present in the kickstart. Agreed that we shouldn't mysteriously enable fingerprinting if authconfig is present. I updated the patch to drop that part! ⬆️

@jlebon
Copy link
Contributor Author

jlebon commented Mar 27, 2018

Dropping this since we have a nicer workaround in https://pagure.io/workstation-ostree-config/pull-request/81. We can follow-up with the details of better fprintd integration with authselect somewhere else.

@jlebon jlebon closed this Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants