PR#182: Enhancements to OTP-handling UX - fedora-packager

fedora-packager

#182 Enhancements to OTP-handling UX

Merged 2 months ago by sgallagh. Opened 2 months ago by sgallagh.

sgallagh/fedora-packager OTP_ux_enhancements into main

Add help documentation about $FKINIT_OTP variable

Stephen Gallagher • 2 months ago

8122b45

fkinit: Allow to pass the OTP token via $FKINIT_OTP

Miro Hrončok • 2 months ago

aecb137

fkinit: Use herestring for pass/OTP

Stephen Gallagher • 2 months ago

45376f1

src/fkinit

file modified

+7 -5

		`@@ -3,7 +3,7 @@`
		`# Created by argbash-init v2.10.0`
		`# ARG_OPTIONAL_SINGLE([user],[u],[Fedora account name],[$USER])`
		`# ARG_OPTIONAL_BOOLEAN([staging],[],[Use the staging infrastructure])`
		`- # ARG_HELP([Acquire a Kerberos ticket-granting ticket for Fedora])`
		`+ # ARG_HELP([Acquire a Kerberos ticket-granting ticket for Fedora],[If the environment variable \$FKINIT_OTP is set, it will be read for the one-time password instead of prompting for it.])`
		`# ARGBASH_GO()`
		`# needed because of Argbash --> m4_ignore([`
		`### START OF CODE GENERATED BY Argbash v2.10.0 one line above ###`
		`@@ -37,8 +37,9 @@`
		`printf '%s\n' "Acquire a Kerberos ticket-granting ticket for Fedora"`
		`printf 'Usage: %s [-u\|--user <arg>] [--(no-)staging] [-h\|--help]\n' "$0"`
		`printf '\t%s\n' "-u, --user: Fedora account name (default: '$USER')"`
		`- printf '\t%s\n' "--staging: Use the staging infrastructure (off by default)"`
		`+ printf '\t%s\n' "--staging, --no-staging: Use the staging infrastructure (off by default)"`
		`printf '\t%s\n' "-h, --help: Prints help"`
		`+ printf '\n%s\n' "If the environment variable \$FKINIT_OTP is set, it will be read for the one-time password instead of prompting for it."`
		`}`


		`@@ -59,8 +60,9 @@`
		`-u*)`
		`_arg_user="${_key##-u}"`
		`;;`
		`- --staging)`
		`+ --no-staging\|--staging)`
		`_arg_staging="on"`
		`+ test "${1:0:5}" = "--no-" && _arg_staging="off"`
		`;;`
		`-h\|--help)`
		`print_help`
		`@@ -104,9 +106,9 @@`
		`kinit -n @$domain -c FILE:$armorcache`

		`F_PASSWORD=$(systemd-ask-password "FAS password:")`
		`- F_OTP=$(systemd-ask-password "FAS OTP (leave blank if not configured):")`
		`+ F_OTP=${FKINIT_OTP:-$(systemd-ask-password "FAS OTP (leave blank if not configured):")}`

		`- echo -n ${F_PASSWORD}${F_OTP} \| kinit -T FILE:$armorcache $_arg_user@$domain >/dev/null`
		`+ kinit -T FILE:$armorcache $_arg_user@$domain <<< "${F_PASSWORD}${F_OTP}" >/dev/null`
		`unset F_PASSWORD`
		`unset F_OTP`

sgallagh commented 2 months ago

The first patch replaces the use of echo with a bash "here string" to avoid putting the password and OTP into the process table. This will avoid a potential attack vector on a shared system.

The second patch resolves ticket #180 and will skip prompting for the OTP if it has been passed as the environment variable $F_OTP to the process.

churchyard commented 2 months ago

Thanks.

One thing I don't like is that the environment variable is unnecessarily concise. That's why I went with FKINIT_OPT in my PR. It makes it clear that the variable is related to the fkinit program.

sgallagh commented 2 months ago

Oops, I didn't actually even realize that was a PR; I thought it was an issue and missed the patch entirely. I'll review it over there.

2 new commits added

Allow passing the OTP via an environment variable.
fkinit: Use herestring for pass/OTP

2 months ago

2 new commits added

Allow passing the OTP via an environment variable.
fkinit: Use herestring for pass/OTP

2 months ago

sgallagh commented 2 months ago

Updated with @churchyard 's changes and some documentation added to --help.

2 new commits added

Allow passing the OTP via an environment variable.
fkinit: Use herestring for pass/OTP

2 months ago

sgallagh commented 2 months ago

... And now also a fix to escape the $ in the help text so it actually prints the string $FKINIT_OTP.

churchyard commented on line 39 of src/fkinit 2 months ago

I believe the if should not be here. What happens if the environment variable is set?

Edited 2 months ago by churchyard

sgallagh commented 2 months ago

I believe the if should not be here. What happens if the environment variable is set?

Huh... I could have sworn I deleted that. You're right, that's superfluous. Fixed.

2 new commits added

Allow passing the OTP via an environment variable.
fkinit: Use herestring for pass/OTP

2 months ago

churchyard commented 2 months ago

Now that the actual change is identical to mine, would you consider actually cherry-picking my commit which includes quite an extensive commit message, and amending your help changes on top?

Or I can do that instead in my PR.

sgallagh commented 2 months ago

Well, there's still a difference in the help message, but yeah; I can split that to a separate commit and pull your original.

3 new commits added

Add help documentation about $FKINIT_OTP variable
fkinit: Allow to pass the OTP token via $FKINIT_OTP
fkinit: Use herestring for pass/OTP

2 months ago

sgallagh commented 2 months ago

OK, I've pulled your original patch and commit message into this set.

churchyard commented 2 months ago

Thank you. Looks good to me.

Pull-Request has been merged by sgallagh

2 months ago

pemensik commented 2 months ago

Errm. Isn't passing of secrets via environment variable considered a bad and insecure practice? I mean, it can be read from /proc/PID by any process of the user or superuser. I think in general piping secrets into the destination is a better variant. I mean, openssh-askpass prints password always to stdout.

It seems to me passing the command to run to get the password would be generally better variant.

pemensik commented 2 months ago

I mean, OTP pin is not hard secret, but still it is a part of access passphrase, right?

sgallagh commented 2 months ago

I mean, OTP pin is not hard secret, but still it is a part of access passphrase, right?

I considered that, but the attack surface is miniscule; while a race does exist where someone might be able to use the OTP before it is passed to kinit, it's an extremely small window. I suppose we could add an admonition against using that feature on a multi-user system to the help text, but the convenience of it to be able to extract the TOTP from a wallet or ykman is high enough to risk the race on a single-user system, I think.

If you're concerned about the race, just use the systemd-ask-password interface.