pam: Always Honor PAM_TTY if set #722
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #722 +/- ##
==========================================
- Coverage 83.43% 83.18% -0.26%
==========================================
Files 83 88 +5
Lines 8689 8975 +286
Branches 74 74
==========================================
+ Hits 7250 7466 +216
- Misses 1111 1164 +53
- Partials 328 345 +17 ☔ View full report in Codecov by Sentry. |
3v1n0
force-pushed
the
pam-tty-check-fix
branch
3 times, most recently
from
7ef2707 to
a7726d0
Compare
pam/internal/adapter/utils.go
Outdated
| tty, err := os.OpenFile(pamTty, os.O_RDWR, 0600) | ||
| if err != nil { | ||
| return nil, func() {}, err | ||
| } |
There was a problem hiding this comment.
This will fail if PAM_TTY is set to $DISPLAY, which according to https://manpages.ubuntu.com/manpages/noble/en/man3/pam_set_item.3.html was done in the past:
PAM_TTY
The terminal name prefixed by /dev/ for device files. In the past, graphical X-based
applications used to store the $DISPLAY variable here, but with the introduction of
PAM_XDISPLAY this usage is deprecated.
Should we print a warning instead?
There was a problem hiding this comment.
Ah, we even just print a debug message in L105
pam/internal/adapter/utils.go
Outdated
| isTerminalTtyOnce.Do(func() { | ||
| tty, cleanup, err := getPamTty(mTx) | ||
| if err != nil { | ||
| log.Debugf(context.TODO(), "Failed to open PAM TTY: %s", err) |
There was a problem hiding this comment.
Shouldn't this be a warning?
There was a problem hiding this comment.
I didn't want to be too pedantic here, since it's not a big deal on pam.ErrBadItem in other failures, but I can probably filter the errors we don't care out...
| if tty == nil { | ||
| tty = os.Stdin | ||
| } | ||
| defer cleanup() |
There was a problem hiding this comment.
I would prefer returning nil in getPamTty and doing
if cleanup != nil {
defer cleanup()
}
here, because it's less surprising IMO
pam/internal/adapter/utils.go
Outdated
|
|
||
| // We check the fd could be passed to x/term to decide if we should fallback to stdin | ||
| if tty.Fd() > math.MaxInt { | ||
| return nil, cleanup, fmt.Errorf("unexpected large PAM TTY fd: %d", tty.Fd()) |
There was a problem hiding this comment.
Maybe do the cleanup here? It seems a bit unexpected that the caller needs to do cleanup when an error is returned.
In case a PAM tty is used, we should do our terminal checks against that instead of the stdin value, since they may differ
In case PAM_TTY is set we should use that to check if we can run the interactive CLI UI, and if it's set we should use it as the tea input
3v1n0
force-pushed
the
pam-tty-check-fix
branch
from
a7726d0 to
1fbcc79
Compare
In case a PAM tty is used, we should do our terminal checks against that
instead of the stdin value, since they may differ.
Do this check also to ensure that we can run the interactive CLI UI.
A follow-up of #720 (comment)
UDENG-5760