review of auth ring spec

[Martin Pool]

I reviewed Vincent's spec http://bazaar-vcs.org/DraftSpecs/AuthenticationRing

That is a very clear and readable spec and a useful feature.

Preliminaries: for many design documents we seem to have shifted from
putting them on the wiki to putting them into the doc/developer
directory.  This spec was drafted before that change.  In general this
seems to be working pretty well: it means that they're tracked by
Bundle Buggy so we can make sure they do not go too long without
review (as this one has); they are not any harder to write; and the
conversation more naturally happens on the list rather than both on
the list and in the wiki.  I think we can continue doing that, and if
Vincent wants to turn this into a rest file in the developer document
that'd be good with me.

On the spec itself:

I can imagine wanting to store other schemes here - for example
passwords to access svn servers through that plugin, or Launchpad
xmlrpc calls.  So this layer should ideally not make too many
assumptions that it's being used by the Transport layer.

Robert dislikes the name 'auth ring'; I think it's ok.

> The realm` can be ignored as long as it is still presented to the user when prompting for the password (unless someone found a way to declare two different realms for the same path).

OK.  I do wonder if it would be better to use the realm as a clue as
to what existing password to use, rather than the path.  But I guess
it's probably rare to have several independent authentication schemes
on the same http host, and hard to generalize about how they should be
treated if they do exist.

> HTTP proxy can be handled as HTTP (or HTTPS).

Of course we need to match up the proxy credentials by the proxy's
host/port, rather than that of the destination server.

> Note that we ignore the port (which can be provided in the URL) to simplify definitions.

Is there a reason why including the port would be hard?

> Also note that an optional unsecure field will be allowed to force the connection to HTTPS hosts that provides a self certified certificate (the default should be to refuse the connection and inform the user).

Please be more explicit and call it something like
'accept_untrusted_certificate', 'unsecure' could mean several
different things.

> scheme matches even if decorators are used in the requested URL

Hm I wonder if we have a factored-out method to match schemes
disregarding decorators?  I suppose we would just trim from the front
til we get a match.

> Possible values are None and base64. When the field is absent, None is assumed. Additional encodings may be added in future versions.

I guess 'None' means plaintext, so why not just call it 'plaintext'
and make that the default if it's not given.

http allows the server to ask for various authentication methods.  I
guess only Basic and Digest can be served by passwords.  I wonder if
they need to be stored in this file.

> host matches if included in the request URL. foo.net will match a requested bzr.foo.net.

I'm not so sure this is a good idea; sometimes subdomains have
different security properties.  Is it really needed?  Maybe we should
leave this out for now, and allow for *.foo.net later.

> The section name is an arbitrary string, only the DEFAULT value is reserved and should appear as the *last* section.

Hm, I would have expected the section name to be the url to which this
applies.  In your example

[foo.net]
scheme=ftp
host=foo.net
user=joe
password=secret-pass

It seems redundant to give it twice, and I can't think of anything
else that would be a more reasonable title.

By the way did you look at how subversion does this?

Maybe in a per-host section I'd want to specify a per-host proxy
server, ssh command, etc.

> The default content of the file will be:

So will this be created by default by Bazaar?

I think I would rather not write out the user's local username, but
rather use that as a fallback every time we are run, in case they run
from different machines or change their username or something.

I can see value in creating a file with comments about what it can
contain, if it does not exist.

> 600 read/write for owner only by default, if another mode (more permissive) is used, no passwords are extracted from it and the user will always be prompted (should be mentioned as a comment in the file itself).

If the permissions are too weak, we should give a warning every time
we try to read it.  I'm not so sure we should *refuse* to read it.

> Why can't bzr update the authentication file when it queried the user for a password ?

We should leave that option open for the future.  I don't see anything
in the current design that would prohibit us asking the user if they
want to save the password, or giving them an option that updates it.
If we did that I think we should just default to base-64 encoding; the
user doesn't need to care about the details.

So all in all really good.

-- 
Martin