Merge lp:~tasdomas/juju-core/charm-store-auth into lp:~go-bot/juju-core/trunk

[Domas Monkus]

Charm store auth token support.

Juju client (deploy and upgrade subcommands) should send an authentication token
to the charm store with all requests, if the token was specified for the environment
under the charm-store-auth key.

https://codereview.appspot.com/22390043/

Charm store auth token support.

Juju client (deploy and upgrade subcommands) should send an authentication token
to the charm store with all requests, if the token was specified for the environment
under the charm-store-auth key.

https://codereview.appspot.com/22390043/

[William Reade (fwereade)]

state/apiserver/client/client.go also uses the charm store, and should
use the auth token. That's not hard in itself, but it's maybe a sign
that we should factor it a little differently -- should we maybe be
passing a CharmStoreAuther into InferRepository? That'd get around the
what-if-it's-global problem I allude to below, at the cost of uglifying
an already ugly func... but I think it's the lesser sin. Thoughts?

https://codereview.appspot.com/22390043/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/22390043/diff/1/cmd/juju/deploy.go#newcode135
cmd/juju/deploy.go:135: CS.SetAuthToken(auth)
I'm a bit twitchy about setting the auth token on what is likely to be
the global charm store object.

https://codereview.appspot.com/22390043/

[Roger Peppe (rogpeppe)]

Thanks for this. A few thoughts below.

https://codereview.appspot.com/22390043/diff/20001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/22390043/diff/20001/charm/repo.go#newcode69
charm/repo.go:69: func (s *CharmStore) SetAuthToken(token string) {
This isn't quite right.

Store is a global variable, so this method can alter global state
without any mutual exclusion.

Better might be just to export the authToken field.
Then it's clear to callers that they need to copy
Store (or create a new one) before adding an auth token.

Alternatively we could add a method to CharmStore, say:

func (cs *CharmStore) WithAuth(authToken string) Repository

which would return a new repository value with authentication
attached.

https://codereview.appspot.com/22390043/diff/20001/charm/repo.go#newcode80
charm/repo.go:80: req.Header.Add("juju-auth", s.authToken)
Is it ok to be adding our own http headers like this?
Might it not be better to include the auth token in the URL?

https://codereview.appspot.com/22390043/diff/40001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/22390043/diff/40001/charm/repo.go#newcode69
charm/repo.go:69: func (s *CharmStore) setAuthToken(token string) {
I'm not sure about this either - this means that the only way of getting
an authenticated connection is by using InferRepository, but we might
not want to do that.

And in fact, it's potentially a little dangerous, as InferRepository
might potentially change in the future so it can infer more than one
charm site, and then we run the danger of giving our secret credentials
to that other site.

I'd be tempted to keep the auth credentials away from InferRepository
and let the client set them as appropriate.

https://codereview.appspot.com/22390043/

[Domas Monkus (tasdomas)]

Reviewers: mp+194130_code.launchpad.net, fwereade, rog,

Message:
Please take a look.

Description:
Charm store auth token support.

Juju client (deploy and upgrade subcommands) should send an
authentication token
to the charm store with all requests, if the token was specified for the
environment
under the charm-store-auth key.

https://code.launchpad.net/~tasdomas/juju-core/charm-store-auth/+merge/194130

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/22390043/

Affected files (+107, -8 lines):
   A [revision details]
   M charm/export_test.go
   M charm/repo.go
   M cmd/juju/deploy.go
   M cmd/juju/upgradecharm.go
   M environs/config/config.go
   M environs/config/config_test.go
   M store/server.go

[Roger Peppe (rogpeppe)]

Good direction, thanks. Time for another round :-)

https://codereview.appspot.com/22390043/diff/100001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/22390043/diff/100001/charm/repo.go#newcode70
charm/repo.go:70: func (s CharmStore) WithAuthToken(token string)
Repository {
This should be a method on *CharmStore like the other methods.

https://codereview.appspot.com/22390043/diff/100001/charm/repo.go#newcode71
charm/repo.go:71: authCS := *Store
s/Store/*s/

https://codereview.appspot.com/22390043/diff/100001/charm/repo.go#newcode83
charm/repo.go:83: /* To comply with RFC 2617, we send the auth token in
Please use the conventional // comments.

https://codereview.appspot.com/22390043/diff/100001/charm/repo.go#newcode87
charm/repo.go:87:
base64.StdEncoding.EncodeToString([]byte(s.authToken)))
A few things here:
"CSAuth" doesn't seem like a great name ("CS" can stand for many
things). How about just "charmstore" (this is always used in an
Authorization header context, so the "Auth" seems redundant)?

Also, can't we define the authorization token to be hex (for example)
thus avoiding the need to base64 encode it? In actual fact standard
base64 really isn't good here, as / is a separator character which is
forbidden in a token.

Also I don't think this actually conforms to RFC 2617 (AFAICS auth-param
is of the form [token "=" token] and the auth-scheme shouldn't be a
quoted string)

To summarise, how about this instead?

req.Header.Add("Authorization", "charmstore token=" + s.authToken)

?

https://codereview.appspot.com/22390043/diff/100001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/22390043/diff/100001/cmd/juju/upgradecharm.go#newcode142
cmd/juju/upgradecharm.go:142: // If a charm store auth token is set,
pass it on to the charm store
Perhaps factor out this piece of code into a separate function
to be called from anywhere necessary.

e.g.

// authorizeCharmRepo returns a version of the given
// charm repository with authorization information
// added from the given environment as appropriate.
func authorizeCharmRepo(repo charm.Repository, cfg *config.Config)
charm.Repository

https://codereview.appspot.com/22390043/diff/100001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/22390043/diff/100001/store/server.go#newcode86
store/server.go:86: tokenParts := strings.SplitN(token, " ", 2)
This should check that the authorization scheme is as expected.

Also, I'm not sure if this is quite in the right place. I'd be more
inclined to add an authorization checker in NewServer.
There's an idiom that comes in useful here.
Something like this, perhaps:

// server is the internal version of Server, which
// is created when serving a request for a particular
// user.
type server struct {
     *Server
     authorized bool
}

func (s *Server) handle(path string, serverHandler func(server,
http.ResponseWriter, *http.Request)) {
     handler := func(w http.ResponseWriter, r *http.Request) {
         s := server{Server: s}
         if token := r.Header.Get("Authorization"); token != "" {
              if authorization matches ... {
                   s....

[Domas Monkus (tasdomas)]

Please take a look.

https://codereview.appspot.com/22390043/

[Roger Peppe (rogpeppe)]

Almost there, thanks! Just a few more minor fixes required.

https://codereview.appspot.com/22390043/diff/100001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/22390043/diff/100001/charm/repo.go#newcode83
charm/repo.go:83: /* To comply with RFC 2617, we send the auth token in
On 2013/11/08 13:08:29, rog wrote:
> Please use the conventional // comments.

not done yet...

https://codereview.appspot.com/22390043/diff/120001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/22390043/diff/120001/charm/repo.go#newcode85
charm/repo.go:85: req.Header.Add("Authorization", "charmstore
"+s.authToken)
I still think that we should follow the letter of rfc 2617 and use the
attribute=value form (probably with "token" for the attribute. Apart
from anything else, it means we're free to add a "realm=" specifier at
some point (also keeping within the RFC).

https://codereview.appspot.com/22390043/diff/120001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/22390043/diff/120001/cmd/juju/main.go#newcode163
cmd/juju/main.go:163: Authorize charm store repository with
authorization token (if any)
Please use // comments to fit in with the usual style.

Also, doc comments should be whole sentences.
Something like this, perhaps:

// AuthorizeCharmRepo returns a repository that adds
// authorization from the given configuration
// to the given repository.

One other thing: I don't think this is universal enough
to warrant inclusion in main.go. I'd put it just next to
one of the places that it's used.

https://codereview.appspot.com/22390043/diff/120001/store/server.go
File store/server.go (right):

https://codereview.appspot.com/22390043/diff/120001/store/server.go#newcode67
store/server.go:67: log.Errorf("Invalid authentication scheme: %s",
scheme)
If you wrote some of this information to the response writer, it would
be easy to write a test for this functionality (something that's
probably worth doing, even if the specific test will change in time).

https://codereview.appspot.com/22390043/

[Kapil Thangavelu (hazmat)]

Looks like there's a conflict marker in the merge.

[Domas Monkus (tasdomas)]

Please take a look.

https://codereview.appspot.com/22390043/

[Roger Peppe (rogpeppe)]

Looking good. Just a few more minor things and we'll soon be there.
Thanks for bearing with me.

https://codereview.appspot.com/22390043/diff/140001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/22390043/diff/140001/charm/repo.go#newcode68
charm/repo.go:68: // Return a copy of the store with auth token set
// WithAuthToken returns a copy of the store with the
// authorization token set.

(doc comments should always be full sentences)

https://codereview.appspot.com/22390043/diff/140001/charm/repo.go#newcode69
charm/repo.go:69: func (s *CharmStore) WithAuthToken(token string)
Repository {
Maybe we should call this WithAuthAttrs (and rename "token" to
"authAttrs" now that that's what it is, and document in this method that
it should be a list of attr=value pairs.

https://codereview.appspot.com/22390043/diff/140001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/22390043/diff/140001/cmd/juju/upgradecharm.go#newcode115
cmd/juju/upgradecharm.go:115: // A new charm URL was explicitly
specified.
How does this comment relate to the statement it's attached to?

https://codereview.appspot.com/22390043/diff/140001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/22390043/diff/140001/environs/config/config.go#newcode81
environs/config/config.go:81: // recognised are "agent-version" and
"development", of types string
We should mention charm-store-auth here.

https://codereview.appspot.com/22390043/diff/140001/environs/config/config.go#newcode234
environs/config/config.go:234: // Ensure that the auth token is a set of
key=value pairs.
Or perhaps (please verify the regexp though, and perhaps make it
stricter):

var validAuthToken = regexp.MustCompile(`^([^\s=]+=[^\s=](,\s*)?)*$`)

then check with validAuthToken.MatchString.

https://codereview.appspot.com/22390043/diff/140001/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/22390043/diff/140001/environs/config/config_test.go#newcode508
environs/config/config_test.go:508: "charm-store-auth": "tokenvalue",
I'd like to see a few more test cases here - there are quite a few ways
the code could get it wrong.

https://codereview.appspot.com/22390043/

[Domas Monkus (tasdomas)]

Please take a look.

https://codereview.appspot.com/22390043/

[William Reade (fwereade)]

LGTM so long as the API-side work is the next branch (ie ServiceDeploy
and ServiceSetCharm, which may themselves hit the charm store).

I'd like it most if the two sides landed together, tbh, because this
work will break unexpectedly as soon as the CLI deploy work moves behind
the API -- but if the followup is in progress I can live with it.

https://codereview.appspot.com/22390043/diff/160001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/22390043/diff/160001/charm/repo.go#newcode70
charm/repo.go:70: func (s *CharmStore) WithAuthAttrs(authAttrs string)
Repository {
I'm not totally wild about just passing in the header content -- what's
the ultimate source of this? Do people have to specify the exact header
contents in their config? Can we maybe provide a slightly cleaner
experience there?

That said, I keep forgetting... this is just a first cut for useful
demoability.

https://codereview.appspot.com/22390043/

[Roger Peppe (rogpeppe)]

LGTM modulo the below.
Thanks!

https://codereview.appspot.com/22390043/diff/160001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/22390043/diff/160001/environs/config/config.go#newcode240
environs/config/config.go:240: " of key-value pairs, not '%s'",
authToken)
s/'%s'/%q/

https://codereview.appspot.com/22390043/diff/160001/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/22390043/diff/160001/environs/config/config_test.go#newcode541
environs/config/config_test.go:541: about: "Auth token invalid
2.",
s/2./3./

how about some more test cases?

ok cases:
    "x=y,"
    "x=y, \tz=w"
    ""
    "x=y,z=w, \t"

not ok cases:
    "x==y"
    " x=y"
    "x=y "
    "=y"
    "x=y =z"

see the missingAttributeNoDefault function for an example
of how to do the above without duplicating all the
struct initialisation code for each case.

e.g.
// authTokenConfigTest returns a config test that checks
// that a configuration with the given auth token
// will pass or fail, depending on the value of ok.
func authTokenConfigTest(token string, ok bool) configTest

https://codereview.appspot.com/22390043/

[Domas Monkus (tasdomas)]

Please take a look.

https://codereview.appspot.com/22390043/

[Roger Peppe (rogpeppe)]

Still LGTM with one trivial remark.

Please feel free to go ahead and merge with that addressed (that's what
LGTM means in general)

https://codereview.appspot.com/22390043/diff/180001/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/22390043/diff/180001/environs/config/config_test.go#newcode550
environs/config/config_test.go:550: testName = fmt.Sprintf("Valid auth
token test: %q", token)
s/Valid/Invalid/
I think. otherwise there's no need to construct the message twice.

https://codereview.appspot.com/22390043/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-11-27 17:03, Roger Peppe wrote:
> Still LGTM with one trivial remark.
>
> Please feel free to go ahead and merge with that addressed (that's
> what LGTM means in general)
>
>
> https://codereview.appspot.com/22390043/diff/180001/environs/config/config_test.go
>
>
File environs/config/config_test.go (right):
>
> https://codereview.appspot.com/22390043/diff/180001/environs/config/config_test.go#newcode550
>
>
environs/config/config_test.go:550: testName = fmt.Sprintf("Valid auth
> token test: %q", token) s/Valid/Invalid/ I think. otherwise there's
> no need to construct the message twice.
>
> https://codereview.appspot.com/22390043/
>

I don't think Domas is in the right group to land it, so we probably
need to merge it for him once it is finished.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlKV8akACgkQJdeBCYSNAAMn+ACeLgn0HsxwaQm4YHKg83OXWPq+
cjwAoIPzAC0vYjzytVfxY/oTL/q5RF2k
=YiqQ
-----END PGP SIGNATURE-----

[Domas Monkus (tasdomas)]

Please take a look.

https://codereview.appspot.com/22390043/

[Domas Monkus (tasdomas)]

Please take a look.

https://codereview.appspot.com/22390043/

=== modified file 'charm/export_test.go'
--- charm/export_test.go	2013-09-04 13:24:57 +0000
+++ charm/export_test.go	2013-12-02 15:48:16 +0000
@@ -8,5 +8,5 @@
 var IfaceExpander = ifaceExpander
 
 func NewStore(url string) *CharmStore {
-	return &CharmStore{url}
+	return &CharmStore{BaseURL: url}
 }

=== modified file 'charm/repo.go'
--- charm/repo.go	2013-10-10 20:58:54 +0000
+++ charm/repo.go	2013-12-02 15:48:16 +0000
@@ -59,15 +59,39 @@
 
 // CharmStore is a Repository that provides access to the public juju charm store.
 type CharmStore struct {
-	BaseURL string
-}
-
-var Store = &CharmStore{"https://store.juju.ubuntu.com"}
+	BaseURL   string
+	authAttrs string // a list of attr=value pairs, comma separated
+}
+
+var Store = &CharmStore{BaseURL: "https://store.juju.ubuntu.com"}
+
+// WithAuthAttrs return a Repository with the authentication token list set.
+// authAttrs is a list of attr=value pairs.
+func (s *CharmStore) WithAuthAttrs(authAttrs string) Repository {
+	authCS := *s
+	authCS.authAttrs = authAttrs
+	return &authCS
+}
+
+// Perform an http get, adding custom auth header if necessary.
+func (s *CharmStore) get(url string) (resp *http.Response, err error) {
+	req, err := http.NewRequest("GET", url, nil)
+	if err != nil {
+		return nil, err
+	}
+	if s.authAttrs != "" {
+		// To comply with RFC 2617, we send the authentication data in
+		// the Authorization header with a custom auth scheme
+		// and the authentication attributes.
+		req.Header.Add("Authorization", "charmstore "+s.authAttrs)
+	}
+	return http.DefaultClient.Do(req)
+}
 
 // Info returns details for a charm in the charm store.
 func (s *CharmStore) Info(curl *URL) (*InfoResponse, error) {
 	key := curl.String()
-	resp, err := http.Get(s.BaseURL + "/charm-info?charms=" + url.QueryEscape(key))
+	resp, err := s.get(s.BaseURL + "/charm-info?charms=" + url.QueryEscape(key))
 	if err != nil {
 		return nil, err
 	}
@@ -99,7 +123,7 @@
 	if digest != "" {
 		query += "@" + digest
 	}
-	resp, err := http.Get(s.BaseURL + "/charm-event?charms=" + url.QueryEscape(query))
+	resp, err := s.get(s.BaseURL + "/charm-event?charms=" + url.QueryEscape(query))
 	if err != nil {
 		return nil, err
 	}
@@ -240,7 +264,7 @@
 	}
 	path := filepath.Join(CacheDir, Quote(curl.String())+".charm")
 	if verify(path, digest) != nil {
-		resp, err := http.Get(s.BaseURL + "/charm/" + url.QueryEscape(curl.Path()))
+		resp, err := s.get(s.BaseURL + "/charm/" + url.QueryEscape(curl.Path()))
 		if err != nil {
 			return nil, err
 		}

=== modified file 'cmd/juju/deploy.go'
--- cmd/juju/deploy.go	2013-10-08 14:53:20 +0000
+++ cmd/juju/deploy.go	2013-12-02 15:48:16 +0000
@@ -9,10 +9,10 @@
 	"os"
 
 	"launchpad.net/gnuflag"
-
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/juju"
 	"launchpad.net/juju-core/juju/osenv"
 	"launchpad.net/juju-core/names"
@@ -128,6 +128,9 @@
 	if err != nil {
 		return err
 	}
+
+	repo = AuthorizeCharmRepo(repo, conf)
+
 	// TODO(fwereade) it's annoying to roundtrip the bytes through the client
 	// here, but it's the original behaviour and not convenient to change.
 	// PutCharm will always be required in some form for local charms; and we
@@ -173,3 +176,15 @@
 	})
 	return err
 }
+
+// AuthorizeCharmRepo returns a repository with authentication added
+// from the specified configuration.
+func AuthorizeCharmRepo(repo charm.Repository, cfg *config.Config) charm.Repository {
+	// If a charm store auth token is set, pass it on to the charm store
+	if auth := cfg.CharmStoreAuth(); auth != "" {
+		if CS, isCS := repo.(*charm.CharmStore); isCS {
+			repo = CS.WithAuthAttrs(auth)
+		}
+	}
+	return repo
+}

=== modified file 'cmd/juju/upgradecharm.go'
--- cmd/juju/upgradecharm.go	2013-10-02 21:36:45 +0000
+++ cmd/juju/upgradecharm.go	2013-12-02 15:48:16 +0000
@@ -111,6 +111,12 @@
 	if err != nil {
 		return err
 	}
+
+	conf, err := conn.State.EnvironConfig()
+	if err != nil {
+		return err
+	}
+
 	oldURL, _ := service.CharmURL()
 	var newURL *charm.URL
 	if c.SwitchURL != "" {
@@ -131,6 +137,9 @@
 	if err != nil {
 		return err
 	}
+
+	repo = AuthorizeCharmRepo(repo, conf)
+
 	// If no explicit revision was set with either SwitchURL
 	// or Revision flags, discover the latest.
 	explicitRevision := true

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2013-11-28 09:09:30 +0000
+++ environs/config/config.go	2013-12-02 15:48:16 +0000
@@ -8,6 +8,7 @@
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"time"
 
@@ -82,8 +83,8 @@
 //
 // The required keys (after any files have been read) are "name",
 // "type" and "authorized-keys", all of type string.  Additional keys
-// recognised are "agent-version" and "development", of types string
-// and bool respectively.
+// recognised are "agent-version" (string) and "development" (bool) as
+// well as charm-store-auth (string containing comma-separated key=value pairs).
 func New(withDefaults Defaulting, attrs map[string]interface{}) (*Config, error) {
 	checker := noDefaultsChecker
 	if withDefaults {
@@ -235,6 +236,14 @@
 		}
 	}
 
+	// Ensure that the auth token is a set of key=value pairs.
+	authToken := cfg.CharmStoreAuth()
+	validAuthToken := regexp.MustCompile(`^([^\s=]+=[^\s=]+(,\s*)?)*$`)
+	if !validAuthToken.MatchString(authToken) {
+		return fmt.Errorf("charm store auth token needs to be a set"+
+			" of key-value pairs, not %q", authToken)
+	}
+
 	// Check the immutable config values.  These can't change
 	if old != nil {
 		for _, attr := range immutableAttributes {
@@ -478,6 +487,11 @@
 	return c.asString("logging-config")
 }
 
+// Auth token sent to charm store
+func (c *Config) CharmStoreAuth() string {
+	return c.asString("charm-store-auth")
+}
+
 // ProvisionerSafeMode reports whether the provisioner should not
 // destroy machines it does not know about.
 func (c *Config) ProvisionerSafeMode() bool {
@@ -536,6 +550,7 @@
 	"api-port":                  schema.ForceInt(),
 	"syslog-port":               schema.ForceInt(),
 	"logging-config":            schema.String(),
+	"charm-store-auth":          schema.String(),
 	"provisioner-safe-mode":     schema.Bool(),
 
 	// Deprecated fields, retain for backwards compatibility.
@@ -578,6 +593,8 @@
 	"state-port":  DefaultStatePort,
 	"api-port":    DefaultAPIPort,
 	"syslog-port": DefaultSyslogPort,
+	// Authentication string sent with requests to the charm store
+	"charm-store-auth": "",
 }
 
 func allowEmpty(attr string) bool {

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2013-11-28 09:11:12 +0000
+++ environs/config/config_test.go	2013-12-02 15:48:16 +0000
@@ -5,6 +5,7 @@
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 	stdtesting "testing"
 	"time"
@@ -555,6 +556,20 @@
 			"type":                      "ec2",
 		},
 	},
+	authTokenConfigTest("token=value, tokensecret=value", true),
+	authTokenConfigTest("token=value, ", true),
+	authTokenConfigTest("token=value, \ttokensecret=value", true),
+	authTokenConfigTest("", true),
+	authTokenConfigTest("token=value, tokensecret=value, \t", true),
+	authTokenConfigTest("=", false),
+	authTokenConfigTest("tokenvalue", false),
+	authTokenConfigTest("token=value, sometoken=", false),
+	authTokenConfigTest("token==value", false),
+	authTokenConfigTest(" token=value", false),
+	authTokenConfigTest("=value", false),
+	authTokenConfigTest("token=value, =z", false),
+	authTokenConfigTest("token=value =z", false),
+	authTokenConfigTest("\t", false),
 	missingAttributeNoDefault("default-series"),
 	missingAttributeNoDefault("firewall-mode"),
 	missingAttributeNoDefault("development"),
@@ -565,6 +580,28 @@
 	// missingAttributeNoDefault("api-port"),
 }
 
+// authTokenConfigTest returns a config test that checks
+// that a configuration with the given auth token
+// will pass or fail, depending on the value of ok.
+func authTokenConfigTest(token string, ok bool) configTest {
+	var testName string
+	var err string
+
+	if ok {
+		testName = fmt.Sprintf("Valid auth token test: %q", token)
+	} else {
+		testName = fmt.Sprintf("Invalid auth token test: %q", token)
+		err = fmt.Sprintf("charm store auth token needs to be a set of key-value pairs, not %q", token)
+	}
+
+	return configTest{
+		about:       testName,
+		useDefaults: config.UseDefaults,
+		attrs:       sampleConfig.Merge(testing.Attrs{"charm-store-auth": token}),
+		err:         regexp.QuoteMeta(err),
+	}
+}
+
 func missingAttributeNoDefault(attrName string) configTest {
 	return configTest{
 		about:       fmt.Sprintf("No default: missing %s", attrName),
@@ -853,6 +890,7 @@
 		"api-port":                  4321,
 		"syslog-port":               2345,
 		"default-series":            "precise",
+		"charm-store-auth":          "token=auth",
 	}
 	cfg, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)