Closed Bug 1076172 Opened 5 years ago Closed 5 years ago

Make mozharness http authentication for developer mode to be easier to use repeteadly

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: armenzg, Assigned: kartikgupta0909, Mentored)

Details

(Whiteboard: [good first bug][easier-mozharness])

Attachments

(3 files, 3 obsolete files)

Hello contributor!

To get started:
In order to help in here you can start by checking out Mozharness:
 hg clone http://hg.mozilla.org/build/mozharness
 cd mozharness
See this wiki for more information:
https://wiki.mozilla.org/Auto-tools/Projects/Mozharness#Setup
At that point, you should aim to try running a b2g emulator job [1] and to be asked only once for http authentication.
Currently you get asked every time.

You can also try to fix bug 1075896 instead of this one.

= Overview =
A bit ago, we added a developer mode for mozharness, this allows developers to run tests in a similar way to what is done in the Release Engineering infrastructure (tbpl.mozilla.org).

One of these features, allows for developers to download files from locations which require LDAP authentication.
Unfortunately, every time a developer runs mozharness locally, the developer mode will ask you (every single time) for your credentials.
Since these days we have to type much longer passwords (due to new Mozilla security rules) it can be rather annoying to do it every time.

= What to fix =
It would be great if when we run mozharness with developer mode for the first time, we get prompted to *store* securely our LDAP credentials.
That way, any following run would not have to ask for the full credentials.

= A possible approach =
It would be great if we had a way to create a file with your http credentials under your home directory, however, we will need a pass key to unlock it.
On following runs, we would detect such file and be asked for a password to unlock it.

I'm open for other suggestions.

The code to be modified is in here [2].


[1]
armenzg-thinkpad mozharness hg:[default!] $ python scripts/b2g_emulator_unittest.py --cfg b2g/emulator_automation_config.py --test-suite reftest --this-chunk 1 --total-chunks 20 \
> --installer-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/emulator.tar.gz \
> --test-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/b2g-35.0a1.en-US.android-arm.tests.zip \
> --cfg developer_config.py
14:57:24     INFO - MultiFileLogger online at 20141001 14:57:24 in /home/armenzg/repos/mozharness
14:57:24  WARNING - When you use developer_config.py, we drop 'read-buildbot-config' from the list of actions.
14:57:24     INFO - Replacing url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/emulator.tar.gz -> https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/emulator.tar.gz
14:57:24     INFO - Replacing url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/b2g-35.0a1.en-US.android-arm.tests.zip -> https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/b2g-35.0a1.en-US.android-arm.tests.zip
14:57:24     INFO - Replacing url http://runtime-binaries.pvt.build.mozilla.org/tooltool/sha512/263f4e8796c25543f64ba36e53d5c4ab8ed4d4e919226037ac0988761d34791b038ce96a8ae434f0153f9c2061204086decdbff18bdced42f3849156ae4dc9a4 -> https://secure.pub.build.mozilla.org/tooltool/pvt/build/sha512/263f4e8796c25543f64ba36e53d5c4ab8ed4d4e919226037ac0988761d34791b038ce96a8ae434f0153f9c2061204086decdbff18bdced42f3849156ae4dc9a4
14:57:24     INFO - Replacing url http://runtime-binaries.pvt.build.mozilla.org/tooltool/sha512/0748e900821820f1a42e2f1f3fa4d9002ef257c351b9e6b78e7de0ddd0202eace351f440372fbb1ae0b7e69e8361b036f6bd3362df99e67fc585082a311fc0df -> https://secure.pub.build.mozilla.org/tooltool/pvt/build/sha512/0748e900821820f1a42e2f1f3fa4d9002ef257c351b9e6b78e7de0ddd0202eace351f440372fbb1ae0b7e69e8361b036f6bd3362df99e67fc585082a311fc0df
14:57:24     INFO - NOTICE: Files downloaded from outside of Release Engineering network require LDAP credentials.
Please enter your full LDAP email address:

[2]
http://hg.mozilla.org/build/mozharness/file/7030a62bf270/mozharness/mozilla/testing/testbase.py#l161
Whiteboard: [easier-mozharness][good first bug] → [good first bug][easier-mozharness]
I created this authenticated site to test the changes against (user: test; password: test):
http://people.mozilla.org/~armenzg/permanent/private_builds

Let me know if I can put smaller size builds in there for you to test.
Hey armenzg: I'd like to give this a shot. Feel free to assign it to me. I'm currently thinking of the following approach:

1) Store the username and the password in the home directory of the user (as you mentioned).
2) Protect the file using a key based challenge, more precisely:
    a) Have a set period of time for which the file is valid. (The time would be written into the file).
    b) After the expiration of the set time, credential re-entry is required.
    c) Upon successful re-entry of the credentials the file is updated with the new time.

3) (more of an improvement): Try to fit an ssh key based authentication into the process rather than basic username password challenge each time.

Does that sound good to you? And yes, could you put in some smaller test build into the directory you posted earlier?

Cheers,
simar

PS: I see this is also being tracked under bug 1075896. Any reason why they are separately placed?
Hi Simarpreet,
Feel free to ignore #2/#3 completely. I find more and more people around Mozilla being OK storing their credentials on their own machines without a pass-phrase of some sort.

Wrt to #3, it is LDAP credentials we're talking about so there's no key involved.

For #1, would you mind putting it under $HOME/.mozilla/credentials.cfg ?
Create the path if missing.

bug 1075896 is actually slightly different. Imagine that all downloadable files are publicly available then there would be no need to request LDAP credentials. Perhaps there's no need to fix that case since once this is fixed, we would only ever have to bother the user once. I'm happy to close that one once this is fixed.

For now, you can probably test it with this (it would only download and extract the file - see the last option):
python scripts/desktop_unittest.py --cfg unittests/linux_unittest.py --mochitest-suite plain-chunked --total-chunks 5 --this-chunk 1 --blob-upload-branch mozilla-central --download-symbols ondemand --cfg developer_config.py --installer-url http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64/1416311724/firefox-36.0a1.en-US.linux-x86_64.tar.bz2 --test-url http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64/1416311724/firefox-36.0a1.en-US.linux-x86_64.tests.zip --download-and-extract

Unfortunately trying to reach people.mozilla.com does not kick in the developer mode properly and tries another download code path which does not have http authentication.

I will try to fix it in bug 1087664. Meanwhile, once you give me your patch I will try to verify it against b2g jobs which do require ldap authentication.
Kartik (adding to CC field) is working on trying to accomplish the encryption of the credentials which we can pipe with this.

When you guys have work in progress please attach it to the bug and we can learn from each other.

Thank you both!
Attached file password.py (obsolete) —
This is the file for encrypting the password and decrypting it using a passphrase.
Attachment #8524821 - Flags: review?(armenzg)
Attachment #8524821 - Attachment mime type: text/x-python → text/plain
Attachment #8524821 - Flags: review?(armenzg)
I spoke with Kartik and we're going to only be focusing on storing the credentials in clear text.
Assignee: nobody → kartikgupta0909
Attached patch Not working (obsolete) — Splinter Review
Attachment #8533204 - Flags: review?
If you're having trouble making the code work is because we're hitting bug 1087664.

I have managed to run the following code by applying the change seen in [1] and using "test" and "test" for the credentials:
python scripts/desktop_unittest.py --cfg unittests/linux_unittest.py --mochitest-suite plain-chunked --total-chunks 5 --this-chunk 1 --blob-upload-branch mozilla-central --download-symbols ondemand --cfg developer_config.py --installer-url http://people.mozilla.org/~armenzg/permanent/private_builds/firefox-36.0a1.en-US.linux-x86_64.tar.bz2 --test-url http://people.mozilla.org/~armenzg/permanent/private_builds/firefox-37.0a1.en-US.linux-x86_64.tests.zip --download-and-extract


diff --git a/mozharness/mozilla/testing/testbase.py b/mozharness/mozilla/testing/testbase.py
--- a/mozharness/mozilla/testing/testbase.py
+++ b/mozharness/mozilla/testing/testbase.py
@@ -337,1 +337,1 @@ 2. running via buildbot and running the 
-        source = self.download_proxied_file(self.test_url, file_name=file_name,
+        source = self.download_file(self.test_url, file_name=file_name,
@@ -347,1 +347,1 @@ 2. running via buildbot and running the 
-        zipfile = self.download_proxied_file(url, parent_dir=dirs['abs_work_dir'],
+        zipfile = self.download_file(url, parent_dir=dirs['abs_work_dir'],
@@ -425,1 +425,1 @@ 2. running via buildbot and running the 
-        source = self.download_proxied_file(self.installer_url,
+        source = self.download_file(self.installer_url,
@@ -441,1 +441,1 @@ 2. running via buildbot and running the 
-        source = self.download_proxied_file(self.symbols_url,
+        source = self.download_file(self.symbols_url,
Comment on attachment 8533204 [details] [diff] [review]
Not working

Review of attachment 8533204 [details] [diff] [review]:
-----------------------------------------------------------------

Setting the review flag to myself.
Attachment #8533204 - Flags: review? → review?(armenzg)
Comment on attachment 8533204 [details] [diff] [review]
Not working

Review of attachment 8533204 [details] [diff] [review]:
-----------------------------------------------------------------

Kartik, even though I have marked the patch as r- it does not mean that is bad code but code that cannot land as-is.
We just need to address the issues mentioned below and request for another review request.
Once you get an r+, I will land the code for you.

Good job and thanks for your help!

::: mozharness/mozilla/testing/testbase.py
@@ +156,5 @@
>      def _get_credentials(self):
>          if not hasattr(self, "https_username"):
>              self.info("NOTICE: Files downloaded from outside of "
>                      "Release Engineering network require LDAP credentials.")
> +	    if (os.path.isfile("password.txt")):

Can you please place this under $HOME/.mozilla/credentials.cfg ?

Can you please place the value as a constant called "developer_mode_credentials"?

@@ +161,5 @@
> +	        fp = open("password.txt",'r')
> +		content = fp.read().splitlines()
> +		fp.close()
> +		self.https_username = content[1].split(':')[1].strip()
> +		self.https_password = content[1].split(':')[1].strip()

No need to split with the change recommended below.

@@ +167,5 @@
> +                self.https_username = raw_input("Please enter your full LDAP email address: ")
> +                self.https_password = getpass.getpass()
> +	        fp = open("password.txt","w+")
> +	        line1 = "Username: "+self.https_username+"\n"
> +	        line2 = "Password: "+self.https_password+"\n"

Could you please write to the file without "Username:"/"Password:"?
Attachment #8533204 - Flags: review?(armenzg) → review-
Attached patch Suggested changes made. (obsolete) — Splinter Review
Attachment #8524821 - Attachment is obsolete: true
Attachment #8533204 - Attachment is obsolete: true
Attachment #8533645 - Flags: review?(armenzg)
Comment on attachment 8533645 [details] [diff] [review]
Suggested changes made.

Changing the request to my mozilla account.
Attachment #8533645 - Flags: review?(armenzg) → review?(armenzg)
Comment on attachment 8533645 [details] [diff] [review]
Suggested changes made.

Review of attachment 8533645 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there!
Please bear with my picky-ness as I want this to be sustainable and be uniform to the rest of the code. Thank you so far!

::: mozharness/mozilla/testing/testbase.py
@@ +156,5 @@
>      def _get_credentials(self):
>          if not hasattr(self, "https_username"):
>              self.info("NOTICE: Files downloaded from outside of "
>                      "Release Engineering network require LDAP credentials.")
> +	    credential_path = os.path.expanduser("~/.mozilla/credentials.cfg")

Add under _get_credentials:
self.credentials_path = os.path.expanduser("~/.mozilla/credentials.cfg")

Using "self." will allow in the future other parts of the code to have access to it.

@@ +169,5 @@
> +                self.https_password = getpass.getpass()
> +	        fp = open(credential_path,"w+")
> +	        line1 = self.https_username+"\n"
> +	        line2 = self.https_password+"\n"
> +		fp.write("Developer mode credentials\n"+line1+line2)

Please don't write "Developer mode credentials".

You can do this instead:
fp.write("%s\n" % self.https_username)
fp.write("%s\n" % self.https_password)
Attachment #8533645 - Flags: review?(armenzg) → review-
Attachment #8533645 - Attachment is obsolete: true
Attachment #8533797 - Flags: review?(armenzg)
Comment on attachment 8533797 [details] [diff] [review]
Suggested changes made.

Review of attachment 8533797 [details] [diff] [review]:
-----------------------------------------------------------------

Ftr, this works if we access the right indexes (0 & 1):
                self.https_username = content[0].strip()
                self.https_password = content[1].strip()

I will land this.

Thanks a lot!

Let me know if bug 1109221 is a good one for you to own.
Attachment #8533797 - Flags: review?(armenzg) → review+
Attachment #8534300 - Flags: review?(armenzg)
Two issues I noticed with this:

1) The .mozilla directory does not exist for me, I had to create it manually. It would have been good to just create it in case it doesn't exist

2) Now my ldap credentials are being stored in cleartext in my user directory, with mode 644. On a multiuser system this means that anyone can read the file. I'd prefer mode 600 instead, or better yet use https://pypi.python.org/pypi/keyring to store it in the system keyring.
Comment on attachment 8534300 [details] [diff] [review]
The indices have been corrected.

Review of attachment 8534300 [details] [diff] [review]:
-----------------------------------------------------------------

We have already landed it. This is why Fallen found some issues.

http://hg.mozilla.org/build/mozharness/rev/b3a666ba7c9a
Comment on attachment 8534427 [details] [diff] [review]
Suggested changes regarding the file permissions have been implemented

Review of attachment 8534427 [details] [diff] [review]:
-----------------------------------------------------------------

Landed with some modifications.
Normally I would ask you to re-submitted an up-to-date patch as we sometimes can create out-of-date patches.
However, today I was already distracted so I took a chance to fix it on the spot.

::: mozharness/mozilla/testing/testbase.py
@@ +157,5 @@
>          if not hasattr(self, "https_username"):
>              self.info("NOTICE: Files downloaded from outside of "
>                      "Release Engineering network require LDAP credentials.")
> +	    if not os.path.exists(os.path.expanduser("~/.mozilla")):
> +	        os.makedirs(os.path.expanduser("~/.mozilla"))

I'm going to approve this little bit and the chmod step below.

I have landed the patch with a bit of modifications. You can see it in here:
http://hg.mozilla.org/build/mozharness/rev/30247a28bfd5

Before attaching new patches remember to use "hg pull -u"; that way you can produce patches that are up-to-date.

If you want to understand what I mean; try to do this:
hg pull
hg up -r 2f784af5a30c 
hg diff
hg up -r b3a666ba7c9a
hg diff
hg up -r tip
hg diff

The first diff will contain this attachment.
The second diff will contain what I would have expected.
The third diff should show you some conflicts.

armenzg@armenzg-thinkpad:~/repos/mozharness$ ls -l ~/.mozilla/credentials.cfg 
-rw------- 1 armenzg armenzg 44 Dec 10 14:25 /home/armenzg/.mozilla/credentials.cfg
Attachment #8534427 - Flags: review?(armenzg.bugzilla) → review+
Attachment #8534300 - Flags: review?(armenzg)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.