review of auth ring spec

Vincent Ladeuil v.ladeuil+lp at free.fr
Fri Jul 13 10:55:59 BST 2007


>>>>> "mbp" == Martin Pool <mbp at sourcefrog.net> writes:

    mbp> I reviewed Vincent's spec
    mbp> http://bazaar-vcs.org/DraftSpecs/AuthenticationRing That
    mbp> is a very clear and readable spec and a useful feature.

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

I'll turn it into a document under doc/developer.

    mbp> On the spec itself:

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

I'll look into that, but the idea was to create an enhanced
PasswordManager that transports can subscribe to, so that seems
to feet.

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

I'll take any better name proposed but will stick with that one
in the mean time. keychain ? cached-credentials ?

    >> 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).

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

Indeed. Realms provide a way to use the same authentication for
disjoint hierarchies, but I'll wait for real use cases before
trying to address that.

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

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

Yup.

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

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

No. The only point being to handle default port values for protocols.

    >> 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).

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

Ok. I didn't want to introduce fields that were protocol
dependent but I don't have a strong opinion on that point.

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

    mbp> Hm I wonder if we have a factored-out method to match
    mbp> schemes disregarding decorators?

Unfortunately no, there is a FIXME in the redirection handling
about that.

    mbp> I suppose we would just trim from the front til we get a
    mbp> match.

I raised that point in another mail, we don't have a coherent
syntax for the decorators.

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

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

Indeed.

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

Well, given that the authentication method is an implementation
detail of the authentication itself and that server should accept
whatever method the client is capable of, I can't imagine a
server requesting different passwords for different
authentication methods. Again, until a real use case occurs, I'll
tend to avoid mentioning the authentication method in that file.

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

    mbp> I'm not so sure this is a good idea; sometimes
    mbp> subdomains have different security properties.  Is it
    mbp> really needed?

I know of several domains where sftp.domain.net and
http.domain.net use the same credentials, so yes, I think it's
needed.

    mbp> Maybe we should leave this out for now, and allow for
    mbp> *.foo.net later.

The 'first match wins' rule allows to declare different
credentials for different sub-domains without a need for regexps
or wildcard chars.

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

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

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

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

To me the title is mostly a comment, I don't think we need to
even make it unique.

It can't be the host only as different paths on the same host may
require different credentials. It can't be scheme+host either for
the same reason.

So, I chose to use it as a comment (and in that example I just
kept the host because it worked), but you can imagine :

[my private projects]
scheme=sftp
host=localhost
path=private
user=joe
password=pass

[my public projects]
scheme=sftp
host=localhost
path=public
user=jim
password=pass2

or even:

[read access]
scheme=http
host=localhost
user=joe
password=pass

[write access]
scheme=sftp
host=locahost
user=jim
password=pass2


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

Just did it very quickly at

http://svnbook.red-bean.com/nightly/en/svn.serverconfig.netmodel.html#svn.serverconfig.netmodel.credcache

Summary: 

- each credentials set is cached in a file whose name is a hash
  of the target. I find it hard to the users (where should I
  update ?), while being no problem for crackers. So a single
  file seems more user-friendly to me.

- svn has a --no-auth-cache option that some users may want to
  activate at a global level ?

- more interestingly svn is able to interface with windows and
  OSX keychains to provide encrypted credentials, so a future
  version can provide that too.

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

Hmmm, have you ever seen hosts with different proxies for
different targets ? Just curious, it may help people working with
the same laptop in various envs but I doubt it...

By 'ssh command' you mean additional parameters to the shh
command related to the credentials proposed to the server ?

    >> The default content of the file will be:

    mbp> So will this be created by default by Bazaar?

For compatibility, if such a file is created it should respect
the previous behavior of bzr. But I think the default version
will contains only comments.

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

Good point, I focused too much on providing a spec without any
constraint on implementations, but that point should be quite easy
to handle in any implementation.

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

Fully agree.

    >> 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).

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

Warning seems enough to me, the idea was to avoid unsuspecting
users to get tricked by a malicious file.

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

    mbp> We should leave that option open for the future.

Ok. Aaron also told me that the file can be rewritten without
losing the comments which was my main fear about updating the
file.

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

Ok.

    mbp> So all in all really good.

Thanks.

Sorry for the delay in my answer, you raised very fine points,
I'll try to rewrite my spec asap, but I wanted to keep the ball
rolling in the mean time,

     Vincent



More information about the bazaar mailing list