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

pam: trigger pam_authenticate on login #3966

Merged
merged 5 commits into from
Jul 10, 2020
Merged

pam: trigger pam_authenticate on login #3966

merged 5 commits into from
Jul 10, 2020

Conversation

awly
Copy link
Contributor

@awly awly commented Jul 2, 2020

This will trigger any "auth" PAM modules configured on the system for
teleport. For example, Duo 2FA prompt on each connection.
The module will be able to interact with the user (e.g. print prompts).

Also, make PAM env var propagation consistent for port forwarding
sessions.

Fixes #3929

@awly awly requested a review from russjones July 2, 2020 00:01
@awly
Copy link
Contributor Author

awly commented Jul 2, 2020

Tested with basic pam_exec.so module and Duo

@russjones
Copy link
Contributor

Can you add test coverage, we have a PAM module we use for testing that can be configured to test if Teleport calls the authentication function. https://github.com/gravitational/teleport/blob/master/modules/pam_teleport/pam_teleport.c

@webvictim
Copy link
Contributor

Some weird test stuff happening here:

Jenkins:

signal: killed
FAIL	github.com/gravitational/teleport/lib/pam	74.098s

It looks like Drone timed out and killed the container too.

@webvictim
Copy link
Contributor

Looks like a memory leak:

2020-07-02-141610__2247x495__maim

@awly
Copy link
Contributor Author

awly commented Jul 2, 2020

Looks like a memory leak:

I think it's because we're reading from stdin in some cases and in tests it's set to an infinite nop reader.

Added an io.LimitReader around it for sanity.

@russjones refactored the testing setup and added missing test cases.

@awly
Copy link
Contributor Author

awly commented Jul 2, 2020

Need to re-run make install in modules/pam_teleport on the build box to get things passing again

@awly awly force-pushed the andrew/pam-auth branch from 4fbe683 to 6de0f62 Compare July 2, 2020 21:19
@awly
Copy link
Contributor Author

awly commented Jul 2, 2020

ok, I fixed the buildbox build steps, but looks like drone uses a cached version of buildbox.
@webvictim any particular reason we don't use make -C build.assets test here: https://github.com/gravitational/teleport/blob/master/.drone.yml#L77-L85 ?
That would let drone pick up any new changes to buildbox.

@webvictim
Copy link
Contributor

webvictim commented Jul 2, 2020

The reason is mostly because Jenkins uses /gopath while Drone uses /go, so the Docker -v commands are different. We also use a shared Go build cache in Drone for speed which Jenkins doesn’t do. I can always modify the Makefile to hold the logic and just call those targets from Drone - the reason I didn’t initially is that for rapid iteration, it can be nice to be able to change the build code without having to cut new tags and backport fixes to release branches.

I am planning to modify the Drone pipeline very soon so that it pulls the buildbox image and rebuilds before running the tests.

With that said, this has actually got me wondering whether the shared build cache could be influencing the test failures in Drone...

@awly
Copy link
Contributor Author

awly commented Jul 7, 2020

Looks like #3982 will soon fix the problem.
I'll wait for it to land, thanks @webvictim!

webvictim added a commit that referenced this pull request Jul 8, 2020
build.assets/Makefile Outdated Show resolved Hide resolved
lib/pam/pam_test.go Show resolved Hide resolved
@awly awly force-pushed the andrew/pam-auth branch from 6de0f62 to c7f1e38 Compare July 8, 2020 21:24
@awly awly requested a review from benarent as a code owner July 8, 2020 21:24
@awly awly requested a review from webvictim July 8, 2020 21:25
webvictim added a commit that referenced this pull request Jul 8, 2020
@awly awly force-pushed the andrew/pam-auth branch from c7f1e38 to 9515494 Compare July 8, 2020 22:53
@benarent
Copy link
Contributor

benarent commented Jul 8, 2020

Do Teleport users need to set anything, or is this automatically on with?

  pam:
     enabled: true

@awly
Copy link
Contributor Author

awly commented Jul 8, 2020

@benarent automatically on with that config field, yes.
Users do need to configure a PAM auth policy to get any effects from this.

@webvictim
Copy link
Contributor

@awly #3982 is merged; I fixed the Dockerfile merge conflicts for you.

Looks like integration tests are failing.

@benarent
Copy link
Contributor

benarent commented Jul 9, 2020

👍 from me. I tested it with Duo and it worked perfectly. New Video Demo: https://share.getcloudapp.com/YEupwAAl

build.assets/Dockerfile Outdated Show resolved Hide resolved
Andrew Lytvynov added 2 commits July 9, 2020 15:42
This will trigger any "auth" PAM modules configured on the system for
teleport. For example, Duo 2FA prompt on each connection.
The module will be able to interact with the user (e.g. print prompts).

Also, make PAM env var propagation consistent for port forwarding
sessions.

Fixes #3929
- update PAM policies and module for "auth" step
- use pam_teleport.so from the repo directory instead of guessing
  OS-specific global path
- add tests covering all failure scenarios and generally refactor PAM
  tests
Andrew Lytvynov added 3 commits July 9, 2020 15:47
This removes the need for libpam-devel on the host and reliably compiles
pam_teleport.so in our CI pipeline.
As part of this, combine build.assets/pam/ and modules/pam_teleport to
avoid the need to sync them.
Updated expected module output.
@awly awly force-pushed the andrew/pam-auth branch from f61061a to 36c4e5e Compare July 9, 2020 22:49
@awly awly requested a review from webvictim July 9, 2020 22:50
@awly
Copy link
Contributor Author

awly commented Jul 10, 2020

Need approval from @klizhentas or @russjones on this one

@awly awly merged commit 78c2a31 into master Jul 10, 2020
@awly awly deleted the andrew/pam-auth branch July 10, 2020 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support External PAM Auth Modules
4 participants