Skip to content

[JENKINS-47113] Populate the authorities after a successful authentication to Github #87

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

Merged

Conversation

Wadeck
Copy link
Contributor

@Wadeck Wadeck commented Nov 1, 2017

  • when the user logged in using Github login, we trigger the loggedIn event in order to populate the LastGrantedAuthorities
  • also when he just uses the Github token, we trigger the authenticated event (but not the loggedIn)
  • store the access token when the user authenticated by Github token or login to populate the future calls using API Token.
  • add an embedded Jetty server in the test to check the behavior when talking with GitHub.

See JENKINS-47113.

Proposed changelog entries

  • After a successful login using GitHub login page, the security groups are populated when using CLI

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees @daniel-beck

- add failing test to reproduce the case
- after using the github auth, we retrieve the groups normally but for next auth with ApiToken, we do not receive the groups
- add fireLoggedIn after GithubAuth
- add fireAuthenticated after GithubToken
@ghost
Copy link

ghost commented Nov 1, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

//
// wc = j.createWebClient();
// // in case of GithubToken there is no session so we retrieve the same result
// makeRequestWithAuthCodeAndVerify(encodeBasic(aliceLogin, aliceApiRestToken), "alice", Arrays.asList("authenticated", "org-a", "org-a*team-b"));
Copy link
Member

Choose a reason for hiding this comment

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

Should the commented out code be deleted? It looks like that code is in testBob_usingGithubLogin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not the same. In such case the commented code is related to the TODO in the use of GitHub token that should or should not trigger the loggedIn event.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok that makes sense.


@Issue("JENKINS-47113")
@Test
public void testGroupPopulation() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

I'd consider moving this stuff to an @Before method and adding @Test to testAlice_usingGithubToken and testBob_usingGithubLogin, and maybe renaming those methods to testLoginUsingGithubApiToken and testLoginUsingGithubOauth?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For that point I followed some previous comments from Jesse, telling me it's better to have a single test in order to avoid double Jenkins startup.
But in this plugin, there's not a lot of tests, so you're right I think, will do the change 👍

SecurityListener.fireAuthenticated(new GithubOAuthUserDetails(token.getName(), github.getAuthorities()));

//TODO do we want to trigger a loggedIn event when using GitHub token ?
// SecurityListener.fireLoggedIn(token.getName());
Copy link
Member

@dwnusbaum dwnusbaum Nov 1, 2017

Choose a reason for hiding this comment

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

SecurityListener#loggedIn JavaDoc says:

Fired when a user has logged in via the web UI.

So the question is whether this code is called from the UI, right? I'll do some local testing, would be good to decide either way and remove the TODO before merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the use of GitHub login, it's when we use the process showing you the GitHub login page etc. It's the case of "Web UI" authentication.
But in the case of GitHub token, we are in the same case as for the API Token in core, it's not trough Web UI but by REST calls, hence my TODO.

FTR I proposed changes in the JavaDoc of loggedIn in #3074 from "via the web UI" to "notion of cache / storage of authentication"

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the link to the proposed JavaDoc changes. I looked through other SecurityRealm implementations (listed here), but didn't find any that called loggedIn for a token-based authentication. The main reason to call fireLoggedIn would be to populate the last granted authorities, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Essentially yes, but as you mentionned it's not a behavior that is really desired. I will remove the comments 👍

- add a TODO on the GithubAPITest that use Ignore but should use Assume
@samrocketman
Copy link
Member

What happens to the granted authorities when a user's access is revoked from an organization or team but they don't log in through the web UI?

@daniel-beck
Copy link
Member

What happens to the granted authorities when a user's access is revoked from an organization or team but they don't log in through the web UI?

This is acting as a cache, so previous access remains. The user record in Jenkins can be deleted, this will remove the associations (even if the user might reappear due to e.g. occurring in SCM).

@samrocketman
Copy link
Member

samrocketman commented Nov 7, 2017

The user record in Jenkins can be deleted, this will remove the associations (even if the user might reappear due to e.g. occurring in SCM).

Are you referring to User.get('username').delete() when you say delete the user record? In general, I know there's large organizations which use this OAuth as a form of authorization. I don't think it would be good behavior if a user is granted authorizations after they've been revoked.

Think of the following scenario from an organization using GitHub Enterprise:

  • A user is revoked from a team or organization but they still require access to other organizations and teams in Jenkins.

I do not think this change will be accepted if it doesn't properly match the user's state of access with GitHub.

Idea: Why not store the GitHub API token generated from a user login during the OAuth process with the user record? This could then be used to determine authorizations and even take advantage of the caches provided for GitHub organizations and teams in the plugin. When the Jenkins API token is used; it could use the cached authorization token from the user login to query GitHub directly for organizations and teams.

@Wadeck
Copy link
Contributor Author

Wadeck commented Nov 7, 2017

The case you mentionned currently did not work either. With current version if you use Github Token or Login you will have the correct authorization but with Jenkins API Token, you will simply be authenticated.

Idea: Why not store the GitHub API token generated from a user login during the OAuth process with the user record? This could then be used to determine authorizations and even take advantage of the caches provided for GitHub organizations and teams in the plugin. When the Jenkins API token is used; it could use the cached authorization token from the user login to query GitHub directly for organizations and teams.

From my PoV this will be a security issue. Storing token that could be retrieved is a bad idea from a security perspective.

@samrocketman
Copy link
Member

samrocketman commented Nov 7, 2017

The case you mentionned currently did not work either. With current version if you use Github Token or Login you will have the correct authorization but with Jenkins API Token, you will simply be authenticated.

Yes if using a Jenkins API token, but a user is not granted additional authorization that they don't have. Granting authorizations which have been revoked in GitHub is not desired behavior. The case I mentioned works if the user is using a GitHub personal access token to interact with the Jenkins API.

From my PoV this will be a security issue. Storing token that could be retrieved is a bad idea from a security perspective.

I don't really see much difference from plugins configuring tokens for authorization with GitHub. Other GitHub applications store this token to interact with GitHub on the user's behalf (of which the user consents). As long as the stored token is secured using the credentials API (or at the very least hudson.util.Secret) AND it is not retrievable by non-admins I don't really see the security concern. Otherwise, one could argue using Jenkins for credential storage would be a security concern.

@stephenc
Copy link
Member

stephenc commented Nov 7, 2017

As long as the stored token is secured using the credentials API (or at the very least hudson.util.Secret) AND it is not retrievable by non-admins

The credentials api is not a match for storage of such details.

I would recommend storing the oauth token in Secret or using a similar mechanism with a realm specific key derived from the instance master key.

jglick
jglick previously approved these changes Nov 7, 2017
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

I guess? Do not understand well enough to review seriously.

* https://developer.github.com/v3/orgs/
* https://developer.github.com/v3/orgs/teams/
*/
private static class MockGithubServlet extends DefaultServlet {
Copy link
Member

Choose a reason for hiding this comment

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

Can also try using WireMock. Cf. discussion in jenkinsci/jenkins-test-harness#39 and linked PRs.

@jglick
Copy link
Member

jglick commented Nov 7, 2017

From out-of-band discussion: I suspect that the root problem here is that JENKINS-20064 should never have been using SecurityListener to begin with, or at least not mandating it. Rather, a SecurityRealm should be in charge of supplying granted authorities for a user ID in all access modes including REST/CLI. LastGrantedAuthoritiesProperty may be a decent default implementation for a typical password-based realm but this plugin may prefer to use a more finely tuned policy.

AbstractPasswordBasedSecurityRealm.createSecurityComponents already uses ImpersonatingUserDetailsService so why does User.impersonate rewrap the result (whether the realm wants it or not)? This seems suspicious.

Copy link
Member

@samrocketman samrocketman left a comment

Choose a reason for hiding this comment

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

Blocking because of security issue. Authorizations should not be granted to a user when authorized access is revoked in GitHub.

@Wadeck
Copy link
Contributor Author

Wadeck commented Nov 8, 2017

@samrocketman you're right, I will propose a modification in that way: i.e. storing the oauth token in Secret as mentioned by Stephen and not triggering the loggedIn event for the moment. Ideally we desire to have such event triggered for audit / log purpose.

@samrocketman
Copy link
Member

samrocketman commented Nov 8, 2017

@Wadeck I appreciate you opening this change and starting this dialogue. I agree the Jenkins token was a bit of a black sheep. Hopefully, this gets resolved and other forms of auth in Jenkins benefit as well (such as SSH keys).

…r in a UserProperty.

- also the loggedIn event trigger is removed to avoid cache problem with LastGrantedAuthorities
@Wadeck
Copy link
Contributor Author

Wadeck commented Nov 14, 2017

@samrocketman new version proposed: we store the access token in a UserProperty and use it to retrieve the real information when the user is authenticated by API Token per example.
I removed the loggedIn event trigger since it's problematic with the LastGrantedAuthorities like discussed. Can you review it ? 🙏

@stephenc can you just check the usage of Secret in order to be sure I'm not making mistakes.

@jglick I investigated your point and I agree the implementation of LastGrantedAuthorities overcome the SecurityListener initial responsability. The idea of providing some caching feature for the SecurityListener could be interesting.

@Wadeck Wadeck requested a review from samrocketman November 14, 2017 14:05
@Wadeck Wadeck changed the title [JENKINS-47113] Fire loggedIn event when using Github login [JENKINS-47113] Store the access token after a successful authentication to GitHub Nov 14, 2017
@Wadeck Wadeck changed the title [JENKINS-47113] Store the access token after a successful authentication to GitHub [JENKINS-47113] Populate the authorities after a successful authentication to Github Nov 14, 2017
@samrocketman
Copy link
Member

I shall review this evening after work hours. I'll also build and play with it. Thanks!

@samrocketman
Copy link
Member

Sure, I'll get that information for you this evening after work.

@samrocketman
Copy link
Member

samrocketman commented Dec 6, 2017

Expand the following details to see plugin versions

Jenkins version and plugins versions
Jenkins version 2.73.1
ace-editor 1.1
ant 1.7
antisamy-markup-formatter 1.5
authentication-tokens 1.3
blueocean 1.2.4
blueocean-autofavorite 1.0.0
blueocean-bitbucket-pipeline 1.2.4
blueocean-commons 1.2.4
blueocean-config 1.2.4
blueocean-dashboard 1.2.4
blueocean-display-url 2.1.0
blueocean-events 1.2.4
blueocean-git-pipeline 1.2.4
blueocean-github-pipeline 1.2.4
blueocean-i18n 1.2.4
blueocean-jira 1.2.4
blueocean-jwt 1.2.4
blueocean-personalization 1.2.4
blueocean-pipeline-api-impl 1.2.4
blueocean-pipeline-editor 1.2.4
blueocean-pipeline-scm-api 1.2.4
blueocean-rest 1.2.4
blueocean-rest-impl 1.2.4
blueocean-web 1.2.4
bouncycastle-api 2.16.2
branch-api 2.0.11
build-timeout 1.18
cloudbees-bitbucket-branch-source 2.2.3
cloudbees-folder 6.1.2
credentials 2.1.16
credentials-binding 1.13
display-url-api 2.0
docker-commons 1.8
docker-workflow 1.13
durable-task 1.14
email-ext 2.60
external-monitor-job 1.7
favorite 2.3.0
git 3.5.1
git-client 2.5.0
git-server 1.7
github 1.28.0
github-api 1.89
github-branch-source 2.2.3
github-oauth 0.28-SNAPSHOT (private-ed74c068-sam)
gradle 1.27.1
handlebars 1.1.1
htmlpublisher 1.14
icon-shim 2.0.3
jackson2-api 2.7.3
jira 2.4.2
jquery-detached 1.2.1
junit 1.21
ldap 1.17
mailer 1.20
mapdb-api 1.0.9.0
matrix-auth 1.7
matrix-project 1.11
mercurial 2.1
momentjs 1.1.1
pam-auth 1.3
pipeline-build-step 2.5.1
pipeline-github-lib 1.0
pipeline-graph-analysis 1.5
pipeline-input-step 2.8
pipeline-milestone-step 1.3.1
pipeline-model-api 1.2.1
pipeline-model-declarative-agent 1.1.1
pipeline-model-definition 1.2.1
pipeline-model-extensions 1.2.1
pipeline-rest-api 2.9
pipeline-stage-step 2.2
pipeline-stage-tags-metadata 1.2.1
pipeline-stage-view 2.9
plain-credentials 1.4
pubsub-light 1.12
resource-disposer 0.8
scm-api 2.2.2
script-security 1.34
sse-gateway 1.15
ssh-credentials 1.13
ssh-slaves 1.21
structs 1.10
subversion 2.9
timestamper 1.8.8
token-macro 2.3
variant 1.1
windows-slaves 1.3.1
workflow-aggregator 2.5
workflow-api 2.22
workflow-basic-steps 2.6
workflow-cps 2.41
workflow-cps-global-lib 2.9
workflow-durable-task-step 2.15
workflow-job 2.14.1
workflow-multibranch 2.16
workflow-scm-step 2.6
workflow-step-api 2.13
workflow-support 2.15
ws-cleanup 0.34

By this point the versions of Jenkins and plugins are out of date. I can try upgrading and cut a new build of your pull request.

@samrocketman
Copy link
Member

samrocketman commented Dec 6, 2017

  • ❌ Testing Jenkins CLI with API token [FAILED]

I'll try to be as exact as possible. Even with the latest build and latest version of Jenkins and plugins, the issue still remains.

Versions of Jenkins and plugins tested

Setup

From the this page https://github.com/samrocketman/jenkins-bootstrap-github-oauth/releases/tag/pr-87-30d61ed

Download your choice of RPM or DEB. It contains a packaged version of Jenkins and every plugin I had installed during testing.

Download the github-oauth.hpi (or build your own). That binary is specifically from commit 30d61ed.

Provision your own VM which supports RPM or DEB packages. It should have at least 6GB of RAM and 2 CPUs are recommended.

Alternatively, if you prefer vagrant then follow the instructions in the jenkins bootstrap README.

If you install the package Jenkins can be started as a service with the following command:

/etc/init.d/jenkins start

Configure Jenkins

When Jenkins is listening on http://localhost:8080/ or wherever you set up Jenkins. The following needs to be done.

  1. Configure the stable GitHub Authentication Plugin according to the wiki.
  2. Log into Jenkins using your GitHub account.
  3. Install your custom github-oauth.hpi from the plugin manager advanced page and visit /restart in the web UI to restart Jenkins to finish upgrading the plugin.
  4. Visit your user configuration page to get the Jenkins API token. See little red box in attached screenshot.

jenkins-api-token-screenshot

Testing Jenkins CLI

Assuming you're using http://localhost:8080/ for Jenkins run the following commands to test Jenkins CLI.

curl -LO http://localhost:8080/jnlpJars/jenkins-cli.jar
java -jar jenkins-cli.jar -s http://localhost:8080/ -http -auth samrocketman:ab2c1dd55cd00fbac7560979ac1281ee who-am-i

Note: If Jenkins CLI prompts for your private key password; then this is a bug. Temporarily rename your ~/.ssh/id_rsa file when testing. You can rename it back when done testing.

@Wadeck
Copy link
Contributor Author

Wadeck commented Dec 6, 2017

@samrocketman after lots of efforts I reproduce your problem. I used a GitHub account with not sufficiant amount of org/team to be able to reproduce initially, so using local Jenkins, Vagrant one or even within a VM, the results is always the same ~15s to have the response.

The root cause seems to be a simple timeout on the CLI. When you are using a GitHub account with logs of org / groups, the current implement will loop on each org / then each team to verify the user belongs to them.
In addition, due to the CLI http mode, three requests are made (1) GET /, 2) ?
POST /cli, 3) POST /cli with body ~ "who-am-i"), and so the token verification must be done thrice, hence the timeout.
The CLI has a timeout around one minute.

I will try to find a way to improve the performance (caching ?) without having the same problem as we encountered with LastGrantedAuthorities.

@samrocketman
Copy link
Member

samrocketman commented Dec 6, 2017

That makes sense. My account is a member of many organizations and teams.

Strangely, when I use the GitHub personal access token it seems to work. Good investigation @Wadeck.

@Wadeck
Copy link
Contributor Author

Wadeck commented Dec 6, 2017

@samrocketman after a bit more analysis on the code, the root cause must be found between GithubAuthenticationToken#new and GithubAuthenticationToken#getGrantedAuthorities. The constructor is used when you authenticate with your Github token and the getGrantedAuthorities when you use the API Token.

I suspect the code in the getGrantedAuthorities to be never used before, as I found a bug during this PR (line around 449, the team was passed instead of the team.getName()). I imagine that method use old code and so the "correct" way is just to have a common method between the constructor and that method to speed up the things (and be consistent). This will no additional caching normally.

I'll keep you informed ;)

(and yes also for me, with my GH token very quick and with API Token timeout)

@samrocketman
Copy link
Member

samrocketman commented Dec 6, 2017

@Wadeck that being said. Would you like me to merge this and you open a separate PR with the fix? As far as I can tell your code didn't actually break anything.

@Wadeck
Copy link
Contributor Author

Wadeck commented Dec 6, 2017

@samrocketman yeah good idea, in order to separate things.

@samrocketman
Copy link
Member

@oleg-nenashev would you mind reviewing again since you have a blocking review?

@jglick jglick dismissed stale reviews from oleg-nenashev and themself December 6, 2017 16:41

stale

@jglick jglick self-requested a review December 6, 2017 16:42
@Wadeck
Copy link
Contributor Author

Wadeck commented Dec 6, 2017

When this one is merged, @samrocketman you will be able to redo your testing process on #92 as it corrects the problem (at least for me)

@samrocketman
Copy link
Member

I'll check this out this evening after work. It will give other reviewers a chance to look before end of day. Thanks for your hard work on this @Wadeck. I know long PR reviews with lots of comments like this one can be cumbersome. I thank you for your patience.

@samrocketman samrocketman merged commit 7e13146 into jenkinsci:master Dec 7, 2017
@samrocketman
Copy link
Member

This pull request also resolves JENKINS-40204.

@samrocketman
Copy link
Member

@oleg-nenashev @jglick @stephenc @Wadeck @daniel-beck @dwnusbaum

Congratulations, you have earned a 👾 sticker as prize for participating in this pull request! A lot of issues were resolved.

@Wadeck Wadeck deleted the JENKINS-47113_FIRE_LOGGED_IN_EVENT branch December 7, 2017 08:12
@barryhagan
Copy link

An unfortunate side effect of this change is now every time my oauth token is updated the SCM Sync configuration plugin (https://plugins.jenkins.io/scm-sync-configuration) commits a change because the token is stored in the user's config.xml.

@Wadeck
Copy link
Contributor Author

Wadeck commented Feb 14, 2018

@barryhagan does it impact the data integrity or behavior of the plugin by not saving the change ?

If it's "just" some noise in the git repository, I see two possibilities:

  1. need to add a property excluder in the scm-sync-configuration plugin ?
  2. change behavior by only triggering a commit when the config files are changed manually ? (dunno if it's easily possible to know)

WDYT ?

In any case, you can file a ticket in JIRA for that plugin mentionning this PR.

@barryhagan
Copy link

@Wadeck, it is a noise problem.

The scm-sync-configuration plugin is doing file diffs on the config.xml and committing the changes to a github repo. It does not inspect individual persisted properties, nor should it be doing that.

In my opinion, the github-oauth-plugin should not be using a user property to store this type of cache data since that ends up in config.xml and the value changes each time a user authenticates. Transient cache data does not belong in a config file. I would suggest having it store the oauth token as a Secret in a different file in $JENKINS_HOME/users/:username.

Obviously it was convenient to use the user property interface to store this, and there are no rules about what can be stored that way, but I just don't think this data belongs in the user config file. The behavior of scm-sync-configuration is a symptom.

@samrocketman
Copy link
Member

samrocketman commented Feb 15, 2018

I disagree. Jenkins API tokens are stored in the same manner per user. The same thing would happen if one changes their Jenkins API token. This data is specific to users and, for now, should remain.

How frequent is it actually changing the API token? This theoretically shouldn't change all that often per login session. Otherwise tools like Travis CI wouldn't be able vote back to repositories using stored tokens from the user via the authorized app.

Needs more investigation.

@barryhagan
Copy link

I totally agree that Jenkins API tokens should be stored this way. They don't change unless you intend to change them. They are effectively a password.

The Github access token is being implicitly updated each time there is an oauth flow to create a session cookie for a user. That will cause numerous config changes on an even moderately active installation and now my configuration commit history is a mess. Updating the stored access token on every session login is not really necessary, but it certainly simplifies the lifetime management of that data.

I've reverted to the previous version for now.

@samrocketman
Copy link
Member

samrocketman commented Feb 15, 2018

Creating an issue is probably the best course of action. This way additional work can be tracked. There's articles on how to manage tokens better long term https://stackoverflow.com/questions/26902600/whats-the-lifetime-of-github-oauth-api-access-token

Would like an issue, though.

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.

8 participants