Merge lp:~mvo/aptdaemon/support-change-credentials-on-add-repo into lp:aptdaemon

[Michael Vogt]

This branch allows changing the credentials for a given machine without duplicating them.

When add_repository is called with a already existing repository that contains credentials this will
currently lead to /etc/apt/auth.conf to append the credentials to the file. This also means that if
the credentials change the first credentials will stay in the file and a duplicated "machine" token
is added.

This branch fixes the issue by cleaning the auth.conf from multiple duplicated machine tokens plus
it will allow updating credentials using the AddRepository call for already existing repositories.

If. e.g. auth.conf contains
machine example.com/foo
user bar
password baz

and AddRepository("deb http://foo:<email address hidden>/foo precise main") is called, this will updated
the credentials with this branch now.

[Anthony Lenton (elachuni)]

<achuni> mvo_: netrc.netrc(auth_conf_path).hosts returns a dict?
<achuni> mvo_: (looking at https://code.launchpad.net/~mvo/aptdaemon/support-change-credentials-on-add-repo/+merge/112098 )
<achuni> yep
<achuni> ah, Eric S. Raymond, why did you think lowercase class names were a good idea?
<mvo_> achuni: hehe
<achuni> mvo_: will l. 51 create the new file owned by the right user?
<achuni> (by root?)
<mvo_> achuni: good catch, but yes, aptdaemon (the backend) runs as root
<achuni> also, could somebody else create a file at /etc/apt/auth.conf.tmp owned by some different user, and have you move it onto /etc/apt/auth.conf on their behalf?
<mvo_> and the umask should be correct too to ensure its 0600, but let me double check that
<achuni> (ie, would it be necessary to check /etc/apt/auth.conf.tmp doesn't exist before opening it?)
<mvo_> achuni: good point again, I think the risk is tiny as the dir is root owned, but I could use a tempfile instead here
<achuni> mvo_: not sure it's necessary, dir perms might be enough here
<achuni> mvo_: jic, creating a tempfile in /tmp could mean it's on a different filesystem, and make the call to os.rename() less atomic
<mvo_> achuni: exactly that was my main reasoning to do it in the same place (plus less risk for tmpdir based attacks as /etc/apt should be non-user writable)
<achuni> right
<achuni> mvo_: if you go for a tempfile, it could be right there in /etc/apt/ maybe?
<mvo_> achuni: aha, that is a good idea
<achuni> mvo_: so, if netrc.hosts is a dict, what happens atm if you have duplicate entries in /etc/apt/ for a ppa, one is just skipped entirely?
<mvo_> achuni: yes, apt will pick the first one
<mvo_> achuni: apt just reads the file sequentially iirc and stops on the first match
<achuni> mvo_: looking at the code it seems it currently keeps the last auth line for each machine (at least in 2.7) I hope that doesn't contradict the symptoms
<achuni> mvo_: I don't think it's an issue, but another side effect of your branch is that it strips out all comments from auth.conf, if there were any
<mvo_> achuni: yeah, the comments are a issue, we currently don't use any there, but users may have put some there :/
<mvo_> achuni: the last auth line is probably the best one currently, or do I miss anything? its a bit tricky to decide if there are multiple ones
<achuni> mvo_: yep, I think the last one is best, as it seems to be the last one that was written with the current code
<mvo_> achuni: yes, I think so too
<achuni> mvo_: I hope that doesn't mean that we need to start looking for a different issue, because this should be working as is :)
<achuni> mvo_: branch looks good in general, I'll give my +1 as is
<achuni> mvo_: I'll let you decide what you want to do about the comments :)

review: Approve

[Michael Vogt (mvo)]

The important points (IMO) are:
- use NamedTemporaryFile() in the same dir as netrc, this is easy however NamedTemporary file uses mode 0600 by default (which we should probably too)
- all comments in auth.conf get lost, we don't write them and preserving them is tricky as the format is very loose, but it would be nice to keep them
- the order of the auth.conf is not preserved

[lp:~mvo/aptdaemon/support-change-credentials-on-add-repo]

849. By Michael Vogt on 2012-09-03

preserve order/comments in auth.conf when updating a existing record

[Michael Vogt (mvo)]

I addressed the points (2) and (3) now with the latest revision. I will work on addressing (1) next.

[lp:~mvo/aptdaemon/support-change-credentials-on-add-repo]

850. By Michael Vogt on 2012-09-03

use tempfile.NamedTemporaryFile() instead of hardcoding a .tmp

851. By Michael Vogt on 2012-09-03

aptdaemon/worker.py: surround machine/login with \n

852. By Michael Vogt on 2012-09-03

add comment

[Michael Vogt (mvo)]

And (1) is addressed now as well.

review: Needs Resubmitting

[Michael Vogt (mvo)]

<achuni> mvo: re: https://code.launchpad.net/~mvo/aptdaemon/support-change-credentials-on-add-repo/+merge/112098 when you have a minute
<mvo> achuni: sure
<achuni> mvo: looking at lines 57-69, there are two options, if there's an entry for the machine we want in the file already, or if not
<achuni> it there isn't an entry already, you add it at the end, that should be fine
* mvo nods
<achuni> if there is, you call re.sub()... I was thinking if the regex is relaxed enough to always match the machine entry
<achuni> mvo: if it isn't, it'll do nothing to the file and continue ahead happily
<mvo> achuni: hm, it should always match the machine, I can not think of case where it wouldn't, do you have a example in mind where it might fail?
 achuni: or are you suggesting to add somethin glike a assert to ensure its different after the regexp so that at least we catch the failures?
<achuni> mvo: a comment between machine and login, or a missing login or password?
 mvo: it might make sense to check if the sub() actually changed something (or call re.search() first) and proceed as if there weren't a machine in the file if not
<mvo> achuni: aha, excellent point about the comment - no password/login would be illegal I think and the code would not find anything
<achuni> ah ok
<mvo> achuni: yeah, makes sense, I will add it into he mp
<achuni> mvo: not sure what you can do if netrc_hosts reports there is an entry for the machine, but we can't find it with a regex
 mvo: what the *right* thing to do would be, I mean
<mvo> achuni: yeah
 achuni: I guess a better parser would be good but the format is a bit anoying

In a nutshell the "comment inside the machine" needs addressing.

[lp:~mvo/aptdaemon/support-change-credentials-on-add-repo]

853. By Michael Vogt on 2012-09-04

add code to ensure that if the regexp for replacing existing auth.conf entries does not work we just prepend as a fallback

854. By Michael Vogt on 2012-09-04

tests/test_worker.py: rename test function to ensure it reflect better what its actually testing

[Michael Vogt (mvo)]

I addressed the comment from achuni now. If the regexp does not work as a workaround it log a error and prepend the new entry so that the apt parser will find it first. I doubt this will happen in the real world as for most people this file will only be touched by applications but I think its good that this case is covered now as well.

[Martin Pitt (pitti)]

Some potential problems:

64 + netrc_hosts_as_text = open(auth_conf_path).read()

Can this file potentially contain non-ASCII characters? For passwords that's likely, or do they get encrypted/hashed?

it might be better to use something like

  with open(auth_conf_path, 'rb') as f:
      netrc_hosts_as_text = f.read().decode('UTF-8')

which works with py 2 and 3 (and also avoids the fd leak).

Same issue in

102 + with open(auth_conf_path_tmp.name, "w") as auth_conf_tmp:
103 + auth_conf_tmp.write(netrc_hosts_as_text)

I don't understand how this works at all, though. auth_conf_path_tmp is already an open and writable (binary) file object (NamedTemporaryFile), so how about you just use this instead of opening the file for writing again?

    auth_conf_path_tmp.write(netrc_hosts_as_text.encode('UTF-8'))

67 + new_netrc_entry = "\nmachine %s login %s password %s\n" % (
68 + machine, res.username, res.password)

I guess passwords in URLs get quoted, so that spaces appear as %20. Do they get unquoted along the way, so that there would suddenly be 7 or more columns?

I suggest to change the test case to use a password with a space and a non-ASCII char, do a full round-trip (into the file and back out), and ensure that the end result is still valid (then you do not need to test details, like how the quoting works, etc.)

It would be nice to additionally have an integration test, so that we do not only ensure that the auth.conf file is written correctly, but that whatever it actually uses sees the correct passwords, even if they are being quoted or encoded in the file.

The rest looks good to me, except potential UnicodeDecodeErrors in the tests for non-ASCII characters.

[lp:~mvo/aptdaemon/support-change-credentials-on-add-repo]

855. By Michael Vogt on 2012-09-05

address the first two review points from Martin (many thanks!) by using open(.., "rb") and read().decode("utf-8")

856. By Michael Vogt on 2012-09-05

use some special chars

[Michael Vogt (mvo)]

Thanks a lot for your detailed review!

On Wed, Sep 05, 2012 at 07:25:20AM -0000, Martin Pitt wrote:
> Some potential problems:
>
> 64 + netrc_hosts_as_text = open(auth_conf_path).read()
>
> Can this file potentially contain non-ASCII characters? For passwords that's likely, or do they get encrypted/hashed?

That can happen, its cleartext passwords.

> it might be better to use something like
>
> with open(auth_conf_path, 'rb') as f:
> netrc_hosts_as_text = f.read().decode('UTF-8')
>
> which works with py 2 and 3 (and also avoids the fd leak).

Thanks, I updated the code.

> Same issue in
>
> 102 + with open(auth_conf_path_tmp.name, "w") as auth_conf_tmp:
> 103 + auth_conf_tmp.write(netrc_hosts_as_text)
>
> I don't understand how this works at all, though. auth_conf_path_tmp is already an open and writable (binary) file object (NamedTemporaryFile), so how about you just use this instead of opening the file for writing again?
>
> auth_conf_path_tmp.write(netrc_hosts_as_text.encode('UTF-8'))

Your suggestion makes more sense I fixed the code now. I was using the
NamedTemporaryFile part just as a way to get the name, but of course I
can simply reuse it and just call flush() (or close()?).

> 67 + new_netrc_entry = "\nmachine %s login %s password %s\n" % (
> 68 + machine, res.username, res.password)
>
> I guess passwords in URLs get quoted, so that spaces appear as %20. Do they get unquoted along the way, so that there would suddenly be 7 or more columns?

They are quoted in the URL but never get unquoted by anything, the
netrc parser in libapt (that is based on the curl parser) does not
support quotes (neither %20 nor ").

> I suggest to change the test case to use a password with a space and a non-ASCII char, do a full round-trip (into the file and back out), and ensure that the end result is still valid (then you do not need to test details, like how the quoting works, etc.)

Good idea, I added that now, note that the space is a %20. The netrc
parser inside python will fail on non-ascii chars fwiw but apt should
read it just fine and pass it on as a bytestring.

> It would be nice to additionally have an integration test, so that we do not only ensure that the auth.conf file is written correctly, but that whatever it actually uses sees the correct passwords, even if they are being quoted or encoded in the file.

apt is the consumer of this file, I'm not sure how to write a
integration test that would cover this. I could call the netrc.netrc
code afterwards to ensure the file has the expected format but that is
not quite a integration test :/

> The rest looks good to me, except potential UnicodeDecodeErrors in the tests for non-ASCII characters.

Could you elaborate? Or is that covered by the suggestion to use a
non-ascii char inside the test?

Thanks,
 Michael

[Martin Pitt (pitti)]

> > The rest looks good to me, except potential UnicodeDecodeErrors in the tests
> for non-ASCII characters.
>
> Could you elaborate? Or is that covered by the suggestion to use a
> non-ascii char inside the test?

Yes, it is. I meant that with adding UTF chars the tests might fail.

In http://bazaar.launchpad.net/~mvo/aptdaemon/support-change-credentials-on-add-repo/revision/855, I think it's better to close the temp file instead of just flushing it. Otherwise the open fd will stay around even after renaming.

Thanks for the fixes!

review: Approve

[lp:~mvo/aptdaemon/support-change-credentials-on-add-repo]

857. By Michael Vogt on 2012-09-07

aptdaemon/worker.py: use close() instead of flush(), thanks to Martin Pitt

[Michael Vogt (mvo)]

Thanks again Martin, I fixed the flush() now and will upload soon.