Skip to content

Conversation

@hkropp
Copy link
Contributor

@hkropp hkropp commented Nov 3, 2016

What is this PR for?

This PR adds PAM authentication support based on the introduced Shiro security implementation. With PAM support system users have immediate access to a secured Zeppelin instance.

What type of PR is it?

Feature

Todos

  • - Create PAM realm
  • - Create test for PAM authentication
  • - Test with running Zeppelin instance

What is the Jira issue?

ZEPPELIN-1611

How should this be tested?

PamRealmTest executes an automated test if the environment variables PAM_USER and PAM_PASS are set. This should be set to system username and password.
The test also includes a main function to manually execute the test. Setting the environment variables for example on MacOS for your IDE use launchctl setenv PAM_USER user and launchctl setenv PAM_PASS xxxxx, the test can then be run from your IDE.

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes

@bzz
Copy link
Member

bzz commented Nov 3, 2016

Thank you for contribution!

There are few things need to be taken care here:

  1. make sure the code adhere project styleguide - i.e whildcard imports in Java are discouraged, etc

  2. make sure that licences for all new runtime dependencies are compatible, and were added added to ./zeppelin-distribution/src/bin_license/LICENSE

  3. AFAIK right now new tests will not be run on TravisC infrastructure as they require some env vars to be set, but no modifications to .travis.yml. Do you think it's worth adding them to one CI profile?

  4. In PR description

    Does this needs documentation? Yes

    Do you plan to provide documentation at ./docs in this PR as well?

@hkropp
Copy link
Contributor Author

hkropp commented Nov 3, 2016

Thanks for your feedback @bzz

I applied the changes required for 1. and 2.

  1. Concerning Travis:
    It would need to be a username and password local to the machine the test is being executed, further it might need to be user that can potentially ssh to the machine as the /etc/pam.d/sshd is being used. For now I think it would be fine to consider this as a manually test.
  2. I'll be happy to provide the documentation. Does this have to be a new PR or can a PR be of multiple types?

@hkropp
Copy link
Contributor Author

hkropp commented Nov 3, 2016

Provided documentation as well.

@bzz
Copy link
Member

bzz commented Nov 4, 2016

👍 for docs in same PR, it looks great.

On CI, I think that's relatively minor and if that's too complicated to configure - we can can opt out for manual test run+instruction in docs.

Double-checking that we have all Licenses for dependencies (and transitive dependencies) logged is important though.

BTW, are you sure that all those commits belong to this branch?

 @anthonycorbacho   [ZEPPELIN-1586] Add security check in NotebookRestApi  …          80c5360
 @astroshim [ZEPPELIN-1585] Testcase for PySparkInterpreter.  …           3f03aa3
 @cloverhearts  [hotfix] does not showing notebooklist on navbar  …           990cc86

@hkropp
Copy link
Contributor Author

hkropp commented Nov 4, 2016

Cool!

Just to be clear the test will simply be ignored with assumeTrue so should be fine without any additional effort. How to use the test itself is documented in the test class with comments.

Concerning the licenses we should be fairly save. libpam4j mainly dependence on JNA and maven. JNA version >4 is available as Apache and actually was already introduced with selenium dependency in this project.

I rebased and pulled in a way that created a little mess here. What I could do is create a new branch with cherry-picked changes and created a new PR based on this, or? Let me know?

I think the Travis build failed because of issue with the other changes, or?

Copy link
Member

@Leemoonsoo Leemoonsoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hkropp for the great contribution.
I have tested this patch and it works well.

As @bzz commented, this PR includes commits not belongs to this contribution. You can try rewrite history or create a new pullrequest and cherry pick.

```
[main]
pamRealm=org.apache.zeppelin.realm.PamRealm
pamRealm.service=sshd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add configuration into conf/shiro.ini.template?

This PR adds [PAM](https://en.wikipedia.org/wiki/Pluggable_authentication_module) authentication support based on the introduced Shiro security implementation. With PAM support system users have immediate access to a secured Zeppelin instance.

Feature

* [x] - Create PAM realm
* [x] - Create test for PAM authentication
* [x] - Test with running Zeppelin instance

[ZEPPELIN-1611](https://issues.apache.org/jira/browse/ZEPPELIN-1611])

`PamRealmTest` executes an automated test if the environment variables `PAM_USER` and `PAM_PASS` are set. This should be set to system username and password.
The test also includes a main function to manually execute the test. Setting the environment variables for example on MacOS for your IDE use `launchctl setenv PAM_USER user` and `launchctl setenv PAM_PASS xxxxx`, the test can then be run from your IDE.

* Does the licenses files need update? Yes
* Is there breaking changes for older versions? No
* Does this needs documentation? Yes
@hkropp
Copy link
Contributor Author

hkropp commented Nov 7, 2016

Thank you @bzz and @Leemoonsoo , I rewrote history and added the shiro.ini.template. I think we should be fine here now, or? Please let me know, if you have further remarks.

Just so you are aware, I believe this currently does not support the resolution/listing of users and groups in UI for example for notebook authorization. Usernames and groups will stay empty with this. I created ZEPPELIN-1631 as a followup. I already have an idea on how to solve this and might be able to contribute this within next weeks.

@Leemoonsoo
Copy link
Member

Tested and LGTM.
Merge if there's no further discussion.

@asfgit asfgit closed this in f866d23 Nov 8, 2016
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.

3 participants