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

Handle missing IdP trait in PAM interpolation. #6558

Merged
merged 6 commits into from
May 6, 2021

Conversation

xacrimon
Copy link
Contributor

@xacrimon xacrimon commented Apr 22, 2021

This defaults the environment variable during PAM environment variable interpolation if it does not exist. The fallback value is meant to indicate a misconfiguration to notify admins of the issue.

Needs backport to branch/v6.

Fixes #6545

@xacrimon xacrimon added PAM Label related to Pluggable Authentication Module (PAM) Submethod. backport-required c-xa Internal Customer Reference labels Apr 22, 2021
@xacrimon xacrimon requested review from fspmarshall and quinqu April 22, 2021 18:41
@xacrimon xacrimon self-assigned this Apr 22, 2021
@russjones
Copy link
Contributor

@xacrimon I'm not sure setting a placeholder is the right answer here, what are our alternatives?

@xacrimon
Copy link
Contributor Author

xacrimon commented Apr 22, 2021

@xacrimon I'm not sure setting a placeholder is the right answer here, what are our alternatives?

There's three courses of action that are possible to take here

  • A) Bubble up the error which causes the SSH connection to fail. This should be included in error logs. This is the current behaviour.
  • B) Set a placeholder and log a warning. This is my proposed solution.
  • C) Don't define the variable at all in the PAM environment. My issue with this is it requires handling the potential nonexistance of these environment values within the PAM module which I feel is inconsistent and cumbersome. Not setting the environment value prevents you from distinguishing between a config that doesn't specify this value due to a node misconfiguration and a misconfigured IdP. I'd rather guarantee their existence to remove this ambiguity.

Option A is problematic because it kills all ability to connect to the node with the misconfigured IdP and due to this figuring out the error can be cumbersome as you need to access the node another way in order to find out what is going wrong.

@xacrimon xacrimon added this to the 6.2 "Buffalo" milestone Apr 23, 2021
@russjones russjones assigned r0mant and unassigned xacrimon Apr 28, 2021
@russjones
Copy link
Contributor

@fspmarshall @r0mant Can you review?

lib/srv/ctx.go Outdated Show resolved Hide resolved
lib/srv/ctx.go Outdated Show resolved Hide resolved
@xacrimon xacrimon requested a review from r0mant May 3, 2021 20:56
@russjones russjones requested a review from awly May 5, 2021 17:56
@xacrimon xacrimon merged commit 8886f8d into master May 6, 2021
@xacrimon xacrimon deleted the joel/handle-missing-trait branch May 6, 2021 20:38
xacrimon added a commit that referenced this pull request May 6, 2021
* Handle missing IdP trait in PAM interpolation.

* emit warning

* respond to feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-required c-xa Internal Customer Reference PAM Label related to Pluggable Authentication Module (PAM) Submethod.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PAM environmental variables break ssh login if corresponding value is NULL
4 participants