-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[ZEPPELIN-2598] Securing Zeppelin with OpenID Connect #2373
Conversation
@@ -86,7 +86,7 @@ | |||
<i ng-if="!navbar.connected" class="fa fa-circle server-disconnected" | |||
uib-tooltip="WebSocket Disconnected" tooltip-placement="bottom" style="margin-top: 7px; vertical-align: top"></i> | |||
<button ng-if="ticket" class="nav-btn dropdown-toggle" type="button" data-toggle="dropdown" style="margin:11px 5px 0 0; padding-left: 0px;"> | |||
<span class="username">{{ticket.principal}}</span> | |||
<span class="username">{{ticket.screenUsername}}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, buji
library uses the principal
field to store token information so we need to display proper user name instead of the whole content of the token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that going to change the output for cases other than buji?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a filter with:
if ($rootScope.ticket.principal.startsWith("#Pac4j")) {
I hope is reasonably enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not "out of the box" but zeppelin could now support this integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it only limited to having pac4j on the classpath and necessary configuration in shiro.ini? Or does it required some code changes in Zeppelin as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pac4j and maybe other dependencies on classpath and configuration is enough! We are integrating with Keycloak i.e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with following shiro.ini configuration with all the required jars on classpath. It is not working. I am getting following error
authentication token of type [class org.apache.shiro.authc.UsernamePasswordToken] could not be authenticated by any configured realms.
sessionManager = org.apache.shiro.web.session.mgt.DefaultWebSessionManager
securityManager.sessionManager = $sessionManager
oidcConfig = org.pac4j.oidc.config.OidcConfiguration
oidcConfig.discoveryURI = https://accounts.google.com/.well-known/openid-configuration
oidcConfig.clientId = 167480702619-8e1lo80dnu8bpk3k0lvvj27noin97vu9.apps.googleusercontent.com
oidcConfig.secret =MhMme_Ik6IH2JMnAT6MFIfee
oidcConfig.clientAuthenticationMethodAsString = client_secret_basic
oidcClient = org.pac4j.oidc.client.OidcClient
oidcClient.configuration = $oidcConfig
config = org.pac4j.core.config.Config
requireRoleAdmin = org.pac4j.core.authorization.authorizer.RequireAnyRoleAuthorizer
requireRoleAdmin.elements = admin
oidcSecurityFilter = io.buji.pac4j.filter.SecurityFilter
oidcSecurityFilter.config = $config
oidcSecurityFilter.clients = oidcClient
clients = org.pac4j.core.client.Clients
clients.callbackUrl = http://localhost:8086/api/callback
clients.clients = $oidcClient
config.clients = $clients
config.authorizers = admin:$requireRoleAdmin
pac4jRealm = io.buji.pac4j.realm.Pac4jRealm
pac4jSubjectFactory = io.buji.pac4j.subject.Pac4jSubjectFactory
securityManager.subjectFactory = $pac4jSubjectFactory
callbackFilter = io.buji.pac4j.filter.CallbackFilter
callbackFilter.defaultUrl = http://localhost:8086
callbackFilter.config = $config
securityManager.sessionManager.globalSessionTimeout = 86400000
shiro.loginUrl = /api/login
[urls]
/api/callback = callbackFilter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am setting up Google OpenID client, the login on Zeppelin should take me to Google account but it is opening Zeppelin Login form, what setup is needed in shiro.ini to correct this behavior?
zeppelin-server/pom.xml
Outdated
<artifactId>commons-lang3</artifactId> | ||
</exclusion> | ||
</exclusions> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add to the LICENSE file on these dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added license for pac4j
, buji
is again with Apache 2 shoul I add a specific file also for that one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to list license of those dependencies in zeppelin-distribution/src/bin-license/LICENSE
file.
licenses/LICENSE-pac4j-1.9.8
also need to be moved to zeppelin-distribution/src/bin-license/licenses
while those dependency will be included in binary package only.
pac4j-oidc brings some transitive dependencies. License of those dependencies also need to be taken care.
[INFO] +- org.pac4j:pac4j-oidc:jar:1.9.8:compile
[INFO] | +- com.nimbusds:oauth2-oidc-sdk:jar:5.24.2:compile
[INFO] | | +- javax.mail:mail:jar:1.4.7:compile
[INFO] | | | \- javax.activation:activation:jar:1.1:compile
[INFO] | | +- com.github.stephenc.jcip:jcip-annotations:jar:1.0-1:compile
[INFO] | | +- org.apache.commons:commons-collections4:jar:4.1:compile
[INFO] | | +- net.minidev:json-smart:jar:1.3.1:compile
[INFO] | | \- com.nimbusds:lang-tag:jar:1.4.3:compile (version selected from constraint [1.4.3,))
[INFO] | \- com.nimbusds:nimbus-jose-jwt:jar:4.26:compile
pom.xml
Outdated
<groupId>org.pac4j</groupId> | ||
<artifactId>pac4j-oidc</artifactId> | ||
<version>${pac4j.version}</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these are only needed for zeppelin-server, do we need these here?
conf/shiro.ini.template
Outdated
#securityManager.subjectFactory = $pac4jSubjectFactory | ||
#oidcSecurityFilter = io.buji.pac4j.filter.SecurityFilter | ||
#oidcSecurityFilter.config = $config | ||
#oidcSecurityFilter.clients = oidcClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you describe how to use this in https://github.com/apache/zeppelin/blob/master/docs/security/shiroauthentication.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to make some explanations in the docs, the fact is that both pac4j
and buji
are really highly configurable and most of configuration is provider dependent, this in theory result in free other integrations done by just changing shiro.ini
but I'm not such an expert ...
zeppelin-server/pom.xml
Outdated
<artifactId>commons-lang3</artifactId> | ||
</exclusion> | ||
</exclusions> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to list license of those dependencies in zeppelin-distribution/src/bin-license/LICENSE
file.
licenses/LICENSE-pac4j-1.9.8
also need to be moved to zeppelin-distribution/src/bin-license/licenses
while those dependency will be included in binary package only.
pac4j-oidc brings some transitive dependencies. License of those dependencies also need to be taken care.
[INFO] +- org.pac4j:pac4j-oidc:jar:1.9.8:compile
[INFO] | +- com.nimbusds:oauth2-oidc-sdk:jar:5.24.2:compile
[INFO] | | +- javax.mail:mail:jar:1.4.7:compile
[INFO] | | | \- javax.activation:activation:jar:1.1:compile
[INFO] | | +- com.github.stephenc.jcip:jcip-annotations:jar:1.0-1:compile
[INFO] | | +- org.apache.commons:commons-collections4:jar:4.1:compile
[INFO] | | +- net.minidev:json-smart:jar:1.3.1:compile
[INFO] | | \- com.nimbusds:lang-tag:jar:1.4.3:compile (version selected from constraint [1.4.3,))
[INFO] | \- com.nimbusds:nimbus-jose-jwt:jar:4.26:compile
pom.xml
Outdated
@@ -105,7 +105,9 @@ | |||
<commons.io.version>2.4</commons.io.version> | |||
<commons.collections.version>3.2.1</commons.collections.version> | |||
<commons.logging.version>1.1.1</commons.logging.version> | |||
<shiro.version>1.2.3</shiro.version> | |||
<shiro.version>1.3.2</shiro.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also update shiro version in zeppelin-distribution/src/bin-license/LICENSE
?
conf/shiro.ini.template
Outdated
#callbackFilter = io.buji.pac4j.filter.CallbackFilter | ||
#callbackFilter.defaultUrl = http://<zeppelin ip>:8080 | ||
#callbackFilter.config = $config | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we'll need something like #securityManager.realms = $pac4jRealm
here, isn't it?
conf/shiro.ini.template
Outdated
@@ -55,6 +55,33 @@ user3 = password4, role2 | |||
#zeppelinHubRealm.zeppelinhubUrl = https://www.zeppelinhub.com | |||
#securityManager.realms = $zeppelinHubRealm | |||
|
|||
### A sample for configuring OIDC Pac4J managed login(i.e. Keycloak) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could separate config here in similar way as in documentation, by separating oidc, clients, callbackFilter configuration by new line
i've just tried to login using the config info from online demo for oidc-client in https://demo.c2id.com/oidc-client using modified config below
but getting exception
please let me know if you can see any apparent misconfiguration i did |
Hi, I have a question. Does this PR means, zeppelin can be OAuth client? For example github, google, ... |
@1ambda potentially yes @khalidhuseynov thanks for pointing out a public server I can test! I will go through later on B.t.w. I realized that the MVP to get this working is just upgrading shiro and the ui triggers, |
Then can we add documentation including examples? (e.g for Google OAuth, Github OAuth, ,..) That's because users are having trouble when they setup security things. So, it would be nice to include documentation like
|
@1ambda To be honest I'm not such an expert and I'm just integrating from a technical POV a solution that other experts in team found. I will love to have such documentation too, but I cannot realistically propose myself for writing it... sorry. |
do this minimal and cleaned up version needs anything else to be worked out in order to be merged? |
@@ -96,8 +96,8 @@ The following components are provided under Apache License. | |||
(Apache 2.0) Lucene Suggest (org.apache.lucene:lucene-suggest:5.3.1 - http://lucene.apache.org/lucene-parent/lucene-suggest) | |||
(Apache 2.0) Elasticsearch: Core (org.elasticsearch:elasticsearch:2.1.0 - http://nexus.sonatype.org/oss-repository-hosting.html/parent/elasticsearch) | |||
(Apache 2.0) Joda convert (org.joda:joda-convert:1.8.1 - http://joda-convert.sourceforge.net) | |||
(Apache 2.0) Shiro Core (org.apache.shiro:shiro-core:1.2.3 - https://shiro.apache.org) | |||
(Apache 2.0) Shiro Web (org.apache.shiro:shiro-web:1.2.3 - https://shiro.apache.org) | |||
(Apache 2.0) Shiro Core (org.apache.shiro:shiro-core:1.3.2 - https://shiro.apache.org) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixcheung already updated
It turns out that the Pac4JPrincipal provided by buji is returning
CommonProfile.getId() and not the getUsername() or getDisplayName() values
for getName().
https://github.com/bujiio/buji-pac4j/blob/master/src/main/java/io/buji/pac4j/subject/Pac4jPrincipal.java
I believe the intent of the Principal.getName() method is to return a
human-friendly name, not a computer-friendly identifier. I created an
issue on the Buji github project to see if they are open to changing it.
https://github.com/bujiio/buji-pac4j/issues/61
…On Fri, Jun 9, 2017 at 6:50 AM, Andrea Peruffo ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In zeppelin-web/src/components/navbar/navbar.html
<#2373 (comment)>:
> @@ -86,7 +86,7 @@
<i ng-if="!navbar.connected" class="fa fa-circle server-disconnected"
uib-tooltip="WebSocket Disconnected" tooltip-placement="bottom" style="margin-top: 7px; vertical-align: top"></i>
<button ng-if="ticket" class="nav-btn dropdown-toggle" type="button" data-toggle="dropdown" style="margin:11px 5px 0 0; padding-left: 0px;">
- <span class="username">{{ticket.principal}}</span>
+ <span class="username">{{ticket.screenUsername}}</span>
@volumeint <https://github.com/volumeint> I like your proposal and
implemented it, unfortunately it turns out that for example with Keycloak
from "getName" I got instead the UUID
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2373 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/Aa_ih9guSoye9v10b1bqHjOCfWJ-ULxeks5sCSORgaJpZM4NpYAh>
.
|
I sincerely think that this implementation is enough to unlock the usage of pac4j, buji (due to the rest of updates sent there). Please let try to finalize this in this week |
ping |
I just submitted a pull request to buji-pac4j to make the value of Principal.getName() configurable via shiro.ini. We just have to wait for it to be accepted and released. I will provide some documentation on integrating with one of the social OAuth providers after I clean up my zeppelin pull request. An ounce of code can save a pound of documentation. |
@andreaTP , a bit strange why Jenkins succeeds, running "mvn clean package -DskipTests" fails with the below errors introduced in this pull request. Can you have a look? Thanks.
|
@necosta checked again but it works for me and on CI ... I have had this problem once try to fetch and pull and see if this solves |
@andreaTP , sorry, I forgot to add. You need to test on top of latest master. nokia:keycloak is 45 commits behind apache:master . That could explain why it's not failing here. Thanks. |
rebased on top of master and fixed style issues, this PR is taking too long for reviewers... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks reasonable to me. I'd suggest adding on documentation on how to use all these but that could come a bit later.
}) | ||
}, function (errorResponse) { | ||
// Handle error case | ||
let redirect = errorResponse.headers('Location') | ||
if (errorResponse.status === 401 && redirect !== undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't redirect be error code 3xx instead of 4xx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the redirect could not be performed automatically, since we are doing our requests with ajax.
In this case the answer is 401 https://github.com/pac4j/pac4j/blob/master/pac4j-core/src/main/java/org/pac4j/core/client/IndirectClient.java#L88
LGTM |
Hi @SarunasG , actually I'm facing the very same situation here ... |
Hi! Please redirect your questions to [email protected] or open a JIRA if there is a bug? |
Is there any document or example to tell us how to use SSO function in zeppelin? |
@xixikaikai #2373 (comment) |
I think the link is broken. |
actually a more detailed description is available here: https://github.com/apache/zeppelin/pull/2552/files |
@andreaTP thanks, I am now trying keycloak with the oidc |
BTW can I use local keycloak for oidc connect to zeppelin? |
@xixikaikai sure, we are using keycloak in production |
hello I've put the following libs into my 0.7.3's zeppelin lib folder, |
|
Again I've added more jars to zeppelin-0.7.3, but the error occur once more |
|
@SarunasG @1ambda @khalidhuseynov @andreaTP Hello, is there any doc of details for me to add libs in 0.7.3 and by changing the shiro.ini config file to support open id? |
Hello, |
Hello @xixikaikai , Dependencies to be added in zeppelin-0.8.0 lib folder: I have added the shiro.ini conf and dependency in the attachment. |
### What is this PR for? Bump Up the Shiro version in zeppelin from 1.3.2 to 1.4.0. Bumping up the Shiro version will help in using the newer pac4j versions. This will also help in integrating Zeppelin and OpenID Connect using latest versions of pac4j. Pac4j also mentions that it is compatible with Shiro 1.4 version. ### What type of PR is it? Improvement ### What is the Jira issue? ZEPPELIN-3791 ### How should this be tested? **Basic User Authentication** - Use the attached Basic Shiro.ini file - Start Zeppelin with "bin/zeppelin-daemon.sh start" - Using browser open zeppelin UI - Try login using user1/password2 - Login successful - Try login using user1/wrong - Login fails as expected **Testing with KeyCloak - Based on Shiro + Pac4j + buij-pac4j** - Use the attached keycloakshiro.ini which uses the zeppelin realm and zeppelin-client - Copy the dependency jars to zeppelin lib folder (list of jars attached) - Configure keycloak with realms and users matching keycloakshiro.ini - Start Zeppelin with "bin/zeppelin-daemon.sh start". - Using browser open zeppelin UI - You will be redirected to Keycloak UI. - Try login using username/password registered in keycloak - success - Try login using user/wrongPassword - Login fails as expected For more details related to Zeppelin keycloak integration refer to [PR-2373](#2373) [dependency-jar.txt](https://github.com/apache/zeppelin/files/2415208/dependency-jar.txt) [keycloakshiro.ini.txt](https://github.com/apache/zeppelin/files/2415209/keycloakshiro.ini.txt) [shiro.ini.txt](https://github.com/apache/zeppelin/files/2415210/shiro.ini.txt) Author: G <[email protected]> Closes #3188 from ajaygk95/master and squashes the following commits: dab87e0 [G] ZEPPELIN-3791: Bump-Up shiro version from 1.3.2 to 1.4.0
@ajaygk95 , thanks for sharing - it worked for us! There are two not resolved questions still:
|
Hi @pmuntyanu , |
Hi @ajaygk95 , it worked! I even changed the |
I succeeded to make zeppelin SSO with keycloak, with roles & logout successfully.
For logout, you do not need to provide logout handler in shiro.ini, instead you need to:
With the above setting, when logout, it will redirect to keycloak logout address (appending zeppelin home address after "redirect_uri" automatically) and it will logout the session from keycloak successfully. After logout, it will then redirect to your zeppelin server home page, which will redirect you to keycloak login page. Of course, the above looks not that elegant, say:
|
I've tried many times following some of the configurations above, but for some strange reason, keep getting redirected to I'm really not sure where the |
@alexanderswerdlow please open JIRA at https://issues.apache.org/jira/browse/ZEPPELIN |
What is this PR for?
Integrating Open ID connect login into Zeppelin leveraging Shiro(already present) and Pac4J( that needs to be in the classpath).
Modifications done here should not affect any existing mechanisms but simply integrates and enable new once.
What type of PR is it?
[Improvement]
What is the Jira issue?
[ZEPPELIN-2598]
Questions: