Solution for leaking secure variables in PR's

j.verdurmen's Avatar

j.verdurmen

25 Nov, 2015 09:26 PM

> Secure variables are not decrypted during pull request (GitHub only)
builds of public projects. This is made intentionally to avoid leaking your
credentials by submitting PR with malicious build script displaying
environment variables.

This is really a big problem as I cannot validate in a PR if someone has broke the Xamarin build. This makes the use of Appveyor really less beneficial.

Right now there are 3 "solutions"

1) Ask people to PR to another branch, and after merge, merge to master
2) Get a second "master, master2 and merge from "master"
3) Don't use secure variables and just expose the username and password in the build script

All of them are horrible :(

But I think I have the solution:

Proposal:

1. Don't decrypt secure variables in PR's (like now)
2. Replace the %...% (or other syntax) as a string replace in the .appveyor script before execution. (so don't add them as environment variables)
3. Maybe protect the .appveyor script from being changed for a PR (or don't decrypt if .appveyor is changed)

Now I can run `appveyor RegisterXamarinLicense -Email %xamarin_email% -Password %xamarin_password%` in a PR, because RegisterXamarinLicense won't print or leak the secure variables.

  1. Support Staff 1 Posted by Feodor Fitsner on 25 Nov, 2015 09:47 PM

    Feodor Fitsner's Avatar

    Now you can securely put your Xamarin credentials on project settings Xamarin tab and it will work for PRs too. Export to YML if you need to.

    - Feodor

  2. 2 Posted by j.verdurmen on 25 Nov, 2015 10:01 PM

    j.verdurmen's Avatar

    Hi Feodor,

    Thanks for response, but I don't fully understand it.

    The secure variables aren't needed anymore? (Then http://www.appveyor.com/docs/lang/xamarin needs a small update :))

  3. Support Staff 3 Posted by Feodor Fitsner on 25 Nov, 2015 10:04 PM

    Feodor Fitsner's Avatar

    Yes, secure variables are no longer needed (for Xamarin credentials) and yes, docs require to be updated.

    - Feodor

  4. 4 Posted by j.verdurmen on 25 Nov, 2015 10:11 PM

    j.verdurmen's Avatar

    That's great!

    I tested it in short, is it possible to also secure the email address?

    Thanks for the quick responses!

  5. Support Staff 5 Posted by Feodor Fitsner on 25 Nov, 2015 10:12 PM

    Feodor Fitsner's Avatar

    Yes, in appveyor.yml.

    - Feodor

  6. 6 Posted by j.verdurmen on 25 Nov, 2015 10:15 PM

    j.verdurmen's Avatar

    Final question,

    I think this won't work in forks? How do I fix the build script that Xamarin builds are not run in the fork? (e.g. `if define`)

    Using CMD right now.

  7. Support Staff 7 Posted by Feodor Fitsner on 25 Nov, 2015 10:18 PM

    Feodor Fitsner's Avatar

    Yes it won't work in forks if they are built under a different AppVeyor account. Configure credentials on project UI then.

    - Feodor

  8. 8 Posted by j.verdurmen on 25 Nov, 2015 10:21 PM

    j.verdurmen's Avatar

    Yes, I did that, but I have still in my build script a command to build with Xamarin.

    That will crash in the fork I think, so I like to improve the build script that it will skip the Xamarin build in forks..

  9. Support Staff 9 Posted by Feodor Fitsner on 25 Nov, 2015 10:29 PM

    Feodor Fitsner's Avatar

    Ah, I see what you mean. You can define some environment variable on main project's UI, say "main_repo" and then check it in build script. It won't be forked as it's not in appveyor.yml.

    - Feodor

  10. 10 Posted by j.verdurmen on 25 Nov, 2015 10:42 PM

    j.verdurmen's Avatar

    That's a great idea!

    I created the environment variable 'build_with_xamarin' for that :)

    Thanks for your help! Great support!

  11. Support Staff 11 Posted by Feodor Fitsner on 26 Nov, 2015 01:02 AM

    Feodor Fitsner's Avatar

    Great, I've also updated docs: http://www.appveyor.com/docs/lang/xamarin - thanks for the catch!

  12. j.verdurmen closed this discussion on 03 Apr, 2017 08:57 PM.

Comments are currently closed for this discussion. You can start a new one.

Keyboard shortcuts

Generic

? Show this help
ESC Blurs the current field

Comment Form

r Focus the comment reply box
^ + ↩ Submit the comment

You can use Command ⌘ instead of Control ^ on Mac

 

06 Jun, 2024 02:12 PM
06 Jun, 2024 03:49 AM
04 Jun, 2024 03:53 AM
30 May, 2024 04:03 PM
27 May, 2024 02:06 AM
20 May, 2024 09:52 PM