Password stored in plain text #184

Closed
TrevorBasinger opened this Issue Aug 10, 2012 · 18 comments

Comments

Projects
None yet
4 participants
Contributor

TrevorBasinger commented Aug 10, 2012

I was poking around /data/system/accounts.db and noticed that the github passwords are being stored in plain text. I dove into the egit-github source and saw that this is being stored in running memory as a Base64 string with "BASIC" prefixing it. This is then sent in the headers of every request that is sent out.

The GitHubClient also allows authentication with oAuth2 tokens. It seems to me like that should be the preferred way of authenticating. Is there a reason why it hasn't been implemented otherwise?

If not, I'd be happy to put together a pull request that would remedy this.

Contributor

kevinsawicki commented Aug 10, 2012

The app could be switched to using oauth2, it would just require either embedding a browser widget to handle the token handshake or requiring that you enter your actual password once which is then used to obtain a token via the API that would be used for future API calls.

I believe the only current issue is that the raw urls for repositories such as this don't accept OAuth tokens for authentication which would affect how files are browsed in private repositories.

Contributor

TrevorBasinger commented Aug 15, 2012

Actually, I think that tokens can be used with private raw repositories if the URL is formatted correctly.

Example:
https://raw.github.com/github/android/master/README.md?login=<username>&token=<token>

I'll start working on that pull request.

Contributor

kevinsawicki commented Aug 15, 2012

I'm not sure the token param currently accepts OAuth2 tokens generated by the api.

Have you had success requesting a raw URL using an api generated oauth2 token?

Contributor

TrevorBasinger commented Aug 15, 2012

You would know better than me. I overlooked the fact that token may not be an access token. I haven't tested it yet, but after I do I'll report back.

Contributor

TrevorBasinger commented Aug 15, 2012

Okay, I've tested it and you were right. However, after a deeper dive into the GitHub API, I'm wondering if using raw.github.com urls are even necessary. The API delivers the raw contents of files in BASE64 encoding. The egit-github doesn't use raw urls either. So I have to ask, why did you mention raw urls in the first place?

Sorry if that's a dumb question. I just want to make sure I'm moving in the same direction you are.

Contributor

kevinsawicki commented Aug 15, 2012

I mentioned raw urls in the first place because the app currently uses them when opening a file in a commit and that would have to change if oauth2 was switched too.

I just wanted to make it clear what would need to be changed beside the login activity if you were going to open a pull request to use oauth2.

The blob API can definitely be used instead of the raw URLs.

Contributor

TrevorBasinger commented Aug 15, 2012

Ah okay, I understand now. I'll make a branch for oauth on my fork and get working on it.

Contributor

kevinsawicki commented Aug 15, 2012

Cool, sounds good, I apologize if my first comment about the raw urls was confusing.

Contributor

TrevorBasinger commented Aug 15, 2012

No worries! I just wanted to make sure I was clear so I didn't break something and look like an ass.

Contributor

TrevorBasinger commented Sep 3, 2012

I wanted to update this since it hasn't had any activity in a while. I've got code working for authorization and I'm about done making the switch from username/password to tokens. I just haven't had time to sit down and finish yet.

Contributor

kevinsawicki commented Sep 3, 2012

Cool, thanks for the update, feel free to open the pull request early and I can help out finishing it up.

xrg commented Sep 6, 2012

++ on the issue. Storing the github password on a mobile device, like that, is not a good idea IMHO.

I'm also eagerly awaiting the arrival of this.

Contributor

TrevorBasinger commented Sep 12, 2012

Thanks for your interest. I'll be making time to wrap this up tonight :)

I want to apologize to people for having to drag this one out. I'm normally
more punctual. I won't get into but life is crazy for me right now. I'm
still entirely committed to making this happen.

If I can't make time to get it done this week I'll be getting Kevin a pull
request of what I have. I just hate presenting incomplete work for someone
else to pick up the pieces.

Looking forward to being able to get you guys something.

Also Kevin, I was merging my changes into the current version of master and
noticed you had removed the jars and egit stuff in the src folder. I
haven't been able to get it to build since. I have gathered/jarred
everything I need except I'm having problems compiling the egit-github
jars. Something about a ProvisionException when I use maven to compile. I
can get you a stack trace later if you need it but I was wondering if there
is an easier way to gather what I need to get it compiling again. I'm
relatively new to using maven for large projects.

Btw, I haven't been using eclipse. I have been working out of tmux with
vim. Is it a necessity to use eclipse when building?

Thanks guys.

On Wed, Sep 12, 2012 at 11:20 AM, Ian Cordasco notifications@github.comwrote:

I'm also eagerly awaiting the arrival of this.


Reply to this email directly or view it on GitHubhttps://github.com/github/android/issues/184#issuecomment-8500238.

Contributor

kevinsawicki commented Sep 12, 2012

It should all build with a mvn clean install from the app folder. You definitely don't have to use Eclipse.

There shouldn't have ever been any jars in the src folder. I did remove from jars from the libs folder that were accidentally checked in. Those jars were only necessary for building from Eclipse so the Maven build should be unaffected.

Please let me know if you have problems with it.

@TrevorBasinger I didn't say that as a means towards rushing your or @kevinsawicki to getting this done. I simply wanted a way to follow the issue from my phone without having to try to clumsily navigate to watch it. I'd rather it be done right than fast.

Contributor

TrevorBasinger commented Sep 12, 2012

No worries @sigmavirus24, I didn't take it as such. I'm more or less just feeling guilty for not delivering anything yet and wanted to let whoever is interested know I'm still committed.

Also, @kevinsawicki mvn clean install worked. I'm not sure what my problem was before. Thanks for the assist.

Contributor

TrevorBasinger commented Sep 13, 2012

Alrighty, so here's my status update for the night. I spent most of my time tracing through the program to figure out it's flow. The Guice stuff is probably what is throwing me off the most right now. I'll commit what I have so far up to my fork after I get some sleep and see what I can get figured out tomorrow.

TrevorBasinger referenced this issue Sep 18, 2012

Closed

oauth #224

@kevinsawicki kevinsawicki added a commit that referenced this issue Oct 5, 2012

@TrevorBasinger @kevinsawicki TrevorBasinger + kevinsawicki Add OAuth support
Closes issue #75 and issue #184
13c6a7e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment