Whitelisting jwt and enabling decryption of whitelisted addons

[sebv]

Core changes required for the JWT token PR.

[sourishkrout]

@joshk looks like this will do the job for us from a travis-core perspective. Let us know if you have questions. Thanks!

[BanzaiMan]

Can we stay away from postfix conditionals? Makes the logic harder to follow.

[sebv]

I was following the pattern of lines 123 and 124. I don't mind to change but probably should be a global thing.

[BanzaiMan]

That's a good point. I thought about this a bit more, and I think the difficulty (for me) comes from using the same conditional twice in postfix: X if A && B; Y if A. The previous structure seems better to me.

[sebv]

The config has changed between the 2, it's not actually the same condition.

[sebv]

@BanzaiMan if you want to clean it up, you need to move if config[:addons] to the delete_addons method.
I tried to do that, but it triggers an issue in the specs when :source is null. Wasn't sure about the best way to fix that (not a ruby programmer).

[santiycr]

@BanzaiMan thoughts? Do you think this is good enough to merge? We can go ahead to modify delete-addons if you think that's the only way to go, but would prefer to avoid that if you think there are other options.

[joshk]

I agree with Hiro that the previous structure was better.
I find this bit of code a little hard to follow sorry.

[BanzaiMan]

@santiycr @sebv Could you rework this part of logical flow before I merge this? A more straightforward

if config[:addons]
  if addons_enabled?
    config[:addons] = decrypt_addons(config[:addons])
  else
    delete_addons(config)
    config[:addons] = decrypt_addons(config[:addons])
  end
end

seems more readable to me, even though config[:addons] = … appears twice in the if statement.

[BanzaiMan]

Further thought… Do we really want all other whitelisted addons to decrypt the configurations when addons_enabled? is falsey?

[halkeye]

@BanzaiMan
If i'm reading it correctly, addons_enabled? is falsey if its a pull request.

If we want to update this check to only decrypt the jwt token on PRs, then we need a new list of decryptable whitelisted plugins?

That seems like the safest solution.

[sebv]

@joshk I've just rebased everything, would it be possible to work on merging beginning of Jan.

[santiycr]

@sebv, @joshk seems like this one got dropped. If we rebased this again, could we get it over the goal line before master moves again? :)

[sebv]

ping @Josk @BanzaiMan. Would be good to get this done, users are waiting:

https://support.saucelabs.com/entries/25614798-How-can-we-set-up-an-open-source-account-that-runs-tests-on-people-s-pull-requests-?

[BanzaiMan]

@sebv @santiycr Yes, let's get started with rebasing! Thank you!! <3

[sebv]

@BanzaiMan just rebased.

[samccone]

hey @BanzaiMan this would be amazing to get in, over on https://github.com/tastejs/todomvc we would loveeeee to have this :)

[jayphelps]

[samccone]

[BanzaiMan]

Thank you! I will take a look soon. :-D

[joshk]

The team is in town next week. I'll make sure we have a chance to chat about the PR :)

[samccone]

👋 @joshk wondering if you had any feedback from your team meetings?

[ErisDS]

Am also interested to know if this is likely to land soon.

[geekdave]

Wondering if I should wait for this to land in Travis production? Or roll out a solution like https://github.com/cvrebert/savage ?

Any timeline estimates @joshk ?

[BanzaiMan]

This (along with travis-ci/travis-build#356) has been deployed to staging. My limited test seems reasonable (works as far as I can tell), and I would like to have a larger set of users test it.

To use this in staging, follow the following steps:

1. Enable a repository at https://staging.travis-ci.org/profile (You can build against only one of Travis environment, so if you are currently using Travis CI production, i.e., https://travis-ci.org, you need to switch back to it when you are done with the experiment.)

2. Encrypt the variable against the staging API endpoint:

   travis encrypt -r REPO/OWNER -e https://api-staging.travis-ci.org SUPER_SECRET_STUFF

3. Create a PR against it. When you try dumping SUPER_SECRET_STUFF, you should see JWT-encoded string instead of the plain text value (https://staging.travis-ci.org/BanzaiMan/travis_staging_test/builds/430639#L101-L102):

   eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJzbHVnIjoiQmFuemFpTWFuL3RyYXZpc19zdGFnaW5nX3Rlc3QiLCJwdWxsLXJlcXVlc3QiOjksImlhdCI6MTQzMjgzNzA4OH0.7GBV_50n04rTwBHzxTpBMFBqWAkojdlsW5yMTnrzQ_Y

The staging environment may be used for other tests, so if you suspect it's not working, please check with me if the correct code is deployed.

[samccone]

Hi @BanzaiMan my orgs are currently not showing up on the staging server

the just.a.test url is not helping much 👅

Looking forward to testing this out on TodoMVC, thanks again for the merge on this

[BanzaiMan]

@samccone Which organization is not showing up? On staging, I see that you have access to "twosigma" and "tastejs" organizations.

[samccone]

Tastejs. Want me to try again?

[BanzaiMan]

@samccone Do you see anything in https://staging.travis-ci.org/profile/tastejs? (Staging is currently used for testing another PR, so JWT addon does not work.)

[samccone]

Shows up now! 👪 ping me when I can try again 💏. Excited to land this

[geekdave]

Hi @BanzaiMan - I'm late to the party here, but just wondering if the use case of running a SauceLabs Pull Request build with secure env variables is a working use case now? The docs here say otherwise: http://docs.travis-ci.com/user/pull-requests/#Security-Restrictions-when-testing-Pull-Requests

[halkeye]

@BanzaiMan I've rebased both patches so they should now work against master again

Also added a new list of addons that are safe to decrypt since thats what one of the comments mentioned.

Let me know if there's anything else.