Skip to content

Commit

Permalink
Fix external user login errors on single controller
Browse files Browse the repository at this point in the history
  • Loading branch information
@wallyworld
wallyworld committed Mar 10, 2022
1 parent 68afff8 commit 2d7cc4f
Show file tree
Hide file tree
Showing 11 changed files with 84 additions and 100 deletions.
9 changes: 5 additions & 4 deletions apiserver/authentication/agent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import (
"github.com/juju/juju/state"
)

// AgentAuthenticator performs authentication for machine and unit agents.
type AgentAuthenticator struct{}
// EntityAuthenticator performs authentication for juju entities.
type EntityAuthenticator struct{}

var _ EntityAuthenticator = (*AgentAuthenticator)(nil)
var _ Authenticator = (*EntityAuthenticator)(nil)

type taggedAuthenticator interface {
state.Entity
Expand All @@ -26,9 +26,10 @@ type taggedAuthenticator interface {

// Authenticate authenticates the provided entity.
// It takes an entityfinder and the tag used to find the entity that requires authentication.
func (*AgentAuthenticator) Authenticate(ctx context.Context, entityFinder EntityFinder, tag names.Tag, req params.LoginRequest) (state.Entity, error) {
func (*EntityAuthenticator) Authenticate(ctx context.Context, entityFinder EntityFinder, tag names.Tag, req params.LoginRequest) (state.Entity, error) {
entity, err := entityFinder.FindEntity(tag)
if errors.IsNotFound(err) {
logger.Debugf("cannot authenticate unknown agent: %v", tag)
return nil, errors.Trace(apiservererrors.ErrBadCreds)
}
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions apiserver/authentication/agent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func (s *agentAuthenticatorSuite) TestValidLogins(c *gc.C) {

for i, t := range testCases {
c.Logf("test %d: %s", i, t.about)
var authenticator authentication.AgentAuthenticator
var authenticator authentication.EntityAuthenticator
entity, err := authenticator.Authenticate(context.TODO(), s.State, t.entity.Tag(), params.LoginRequest{
Credentials: t.credentials,
Nonce: t.nonce,
Expand Down Expand Up @@ -139,7 +139,7 @@ func (s *agentAuthenticatorSuite) TestInvalidLogins(c *gc.C) {

for i, t := range testCases {
c.Logf("test %d: %s", i, t.about)
var authenticator authentication.AgentAuthenticator
var authenticator authentication.EntityAuthenticator
entity, err := authenticator.Authenticate(context.TODO(), s.State, t.entity.Tag(), params.LoginRequest{
Credentials: t.credentials,
Nonce: t.nonce,
Expand Down
9 changes: 5 additions & 4 deletions apiserver/authentication/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ package authentication
import (
"context"

"github.com/juju/names/v4"

"github.com/juju/juju/rpc/params"
"github.com/juju/juju/state"
"github.com/juju/names/v4"
)

// EntityAuthenticator is the interface all entity authenticators need to implement
// Authenticator is the interface all entity authenticators need to implement
// to authenticate juju entities.
type EntityAuthenticator interface {
// Authenticate authenticates the given entity
type Authenticator interface {
// Authenticate authenticates the given entity.
Authenticate(ctx context.Context, entityFinder EntityFinder, tag names.Tag, req params.LoginRequest) (state.Entity, error)
}

Expand Down
40 changes: 23 additions & 17 deletions apiserver/authentication/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ import (

var logger = loggo.GetLogger("juju.apiserver.authentication")

// UserAuthenticator performs authentication for local users. If a password
type UserAuthenticator struct {
AgentAuthenticator
// LocalUserAuthenticator performs authentication for local users. If a password
type LocalUserAuthenticator struct {
EntityAuthenticator

// Bakery holds the bakery that is used to mint and verify macaroons.
Bakery ExpirableStorageBakery
Expand Down Expand Up @@ -61,24 +61,27 @@ const (
externalLoginExpiryTime = 1 * time.Hour
)

var _ EntityAuthenticator = (*UserAuthenticator)(nil)
var _ Authenticator = (*LocalUserAuthenticator)(nil)

// Authenticate authenticates the entity with the specified tag, and returns an
// error on authentication failure.
//
// If and only if no password is supplied, then Authenticate will check for any
// valid macaroons. Otherwise, password authentication will be performed.
func (u *UserAuthenticator) Authenticate(
func (u *LocalUserAuthenticator) Authenticate(
ctx context.Context, entityFinder EntityFinder, tag names.Tag, req params.LoginRequest,
) (state.Entity, error) {
userTag, ok := tag.(names.UserTag)
if !ok {
return nil, errors.Errorf("invalid request")
}
if req.Credentials == "" && userTag.IsLocal() {
if !userTag.IsLocal() {
return nil, errors.Errorf("invalid request - expected local user")
}
if req.Credentials == "" {
return u.authenticateMacaroons(ctx, entityFinder, userTag, req)
}
return u.AgentAuthenticator.Authenticate(ctx, entityFinder, tag, req)
return u.EntityAuthenticator.Authenticate(ctx, entityFinder, tag, req)
}

// CreateLocalLoginMacaroon creates a macaroon that may be provided to a
Expand Down Expand Up @@ -157,7 +160,7 @@ func DischargeCaveats(tag names.UserTag, clock clock.Clock) []checkers.Caveat {
return firstPartyCaveats
}

func (u *UserAuthenticator) authenticateMacaroons(
func (u *LocalUserAuthenticator) authenticateMacaroons(
ctx context.Context, entityFinder EntityFinder, tag names.UserTag, req params.LoginRequest,
) (state.Entity, error) {
// Check for a valid request macaroon.
Expand Down Expand Up @@ -241,11 +244,11 @@ type ExternalMacaroonAuthenticator struct {
Clock clock.Clock
}

var _ EntityAuthenticator = (*ExternalMacaroonAuthenticator)(nil)
var _ Authenticator = (*ExternalMacaroonAuthenticator)(nil)

// Authenticate authenticates the provided entity. If there is no macaroon provided, it will
// return a *DischargeRequiredError containing a macaroon that can be used to grant access.
func (m *ExternalMacaroonAuthenticator) Authenticate(ctx context.Context, entityFinder EntityFinder, _ names.Tag, req params.LoginRequest) (state.Entity, error) {
func (m *ExternalMacaroonAuthenticator) Authenticate(ctx context.Context, _ EntityFinder, _ names.Tag, req params.LoginRequest) (state.Entity, error) {
authChecker := m.Bakery.Checker.Auth(req.Macaroons...)
ai, identErr := authChecker.Allow(ctx, identchecker.LoginOp)
if de, ok := errors.Cause(identErr).(*bakery.DischargeRequiredError); ok {
Expand All @@ -262,6 +265,7 @@ func (m *ExternalMacaroonAuthenticator) Authenticate(ctx context.Context, entity
return nil, errors.Trace(identErr)
}
username := ai.Identity.Id()
logger.Debugf("authenticated external user %q", username)
var tag names.UserTag
if names.IsValidUserName(username) {
// The name is a local name without an explicit @local suffix.
Expand All @@ -282,13 +286,15 @@ func (m *ExternalMacaroonAuthenticator) Authenticate(ctx context.Context, entity
return nil, errors.Errorf("external identity provider has provided ostensibly local name %q", username)
}
}
entity, err := entityFinder.FindEntity(tag)
if errors.IsNotFound(err) {
return nil, errors.Trace(apiservererrors.ErrBadCreds)
} else if err != nil {
return nil, errors.Trace(err)
}
return entity, nil
return externalUser{tag}, nil
}

type externalUser struct {
tag names.Tag
}

func (e externalUser) Tag() names.Tag {
return e.tag
}

// IdentityFromContext implements IdentityClient.IdentityFromContext.
Expand Down
28 changes: 8 additions & 20 deletions apiserver/authentication/user_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func (s *userAuthenticatorSuite) TestMachineLoginFails(c *gc.C) {
machinePassword := password

// attempt machine login
authenticator := &authentication.UserAuthenticator{}
authenticator := &authentication.LocalUserAuthenticator{}
_, err = authenticator.Authenticate(context.TODO(), nil, machine.Tag(), params.LoginRequest{
Credentials: machinePassword,
Nonce: nonce,
Expand All @@ -70,7 +70,7 @@ func (s *userAuthenticatorSuite) TestUnitLoginFails(c *gc.C) {
unitPassword := password

// Attempt unit login
authenticator := &authentication.UserAuthenticator{}
authenticator := &authentication.LocalUserAuthenticator{}
_, err = authenticator.Authenticate(context.TODO(), nil, unit.Tag(), params.LoginRequest{
Credentials: unitPassword,
Nonce: "",
Expand All @@ -86,7 +86,7 @@ func (s *userAuthenticatorSuite) TestValidUserLogin(c *gc.C) {
})

// User login
authenticator := &authentication.UserAuthenticator{}
authenticator := &authentication.LocalUserAuthenticator{}
_, err := authenticator.Authenticate(context.TODO(), s.State, user.Tag(), params.LoginRequest{
Credentials: "password",
Nonce: "",
Expand All @@ -102,7 +102,7 @@ func (s *userAuthenticatorSuite) TestUserLoginWrongPassword(c *gc.C) {
})

// User login
authenticator := &authentication.UserAuthenticator{}
authenticator := &authentication.LocalUserAuthenticator{}
_, err := authenticator.Authenticate(context.TODO(), s.State, user.Tag(), params.LoginRequest{
Credentials: "wrongpassword",
Nonce: "",
Expand All @@ -124,7 +124,7 @@ func (s *userAuthenticatorSuite) TestInvalidRelationLogin(c *gc.C) {
c.Assert(err, jc.ErrorIsNil)

// Attempt relation login
authenticator := &authentication.UserAuthenticator{}
authenticator := &authentication.LocalUserAuthenticator{}
_, err = authenticator.Authenticate(context.TODO(), nil, relation.Tag(), params.LoginRequest{
Credentials: "dummy-secret",
Nonce: "",
Expand All @@ -144,7 +144,7 @@ func (s *userAuthenticatorSuite) TestValidMacaroonUserLogin(c *gc.C) {
service := mockBakeryService{}

// User login
authenticator := &authentication.UserAuthenticator{Bakery: &service, Clock: testclock.NewClock(time.Time{})}
authenticator := &authentication.LocalUserAuthenticator{Bakery: &service, Clock: testclock.NewClock(time.Time{})}
_, err = authenticator.Authenticate(context.TODO(), s.State, user.Tag(), params.LoginRequest{
Credentials: "",
Nonce: "",
Expand Down Expand Up @@ -176,7 +176,7 @@ func (s *userAuthenticatorSuite) TestCreateLocalLoginMacaroon(c *gc.C) {
func (s *userAuthenticatorSuite) TestAuthenticateLocalLoginMacaroon(c *gc.C) {
service := mockBakeryService{}
clock := testclock.NewClock(time.Time{})
authenticator := &authentication.UserAuthenticator{
authenticator := &authentication.LocalUserAuthenticator{
Bakery: &service,
Clock: clock,
LocalUserIdentityLocation: "https://testing.invalid:1234/auth",
Expand Down Expand Up @@ -271,21 +271,14 @@ var authenticateSuccessTests = []struct {
about: "user that can be found",
dischargedUsername: "bobbrown@somewhere",
expectTag: "user-bobbrown@somewhere",
finder: simpleEntityFinder{
"user-bobbrown@somewhere": true,
},
finder: simpleEntityFinder{},
}, {
about: "user with no @ domain",
dischargedUsername: "bobbrown",
finder: simpleEntityFinder{
"user-bobbrown@external": true,
},
expectTag: "user-bobbrown@external",
}, {
about: "user not found in database",
dischargedUsername: "bobbrown@nowhere",
finder: simpleEntityFinder{},
expectError: "invalid entity name or password",
}, {
about: "invalid user name",
dischargedUsername: "--",
Expand All @@ -298,11 +291,6 @@ var authenticateSuccessTests = []struct {
"cheat@local": true,
},
expectError: `external identity provider has provided ostensibly local name "cheat@local"`,
}, {
about: "FindEntity error",
dischargedUsername: "bobbrown@nowhere",
finder: errorEntityFinder("lost in space"),
expectError: "lost in space",
}}

type alwaysIdent struct {
Expand Down
6 changes: 0 additions & 6 deletions apiserver/httpcontext/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"encoding/json"
"fmt"
"net/http"
"time"

"github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery"
"github.com/juju/errors"
Expand Down Expand Up @@ -87,11 +86,6 @@ type AuthInfo struct {
// Entity is the user/machine/unit/etc that has authenticated.
Entity Entity

// LastConnection returns the time of the last connection for
// the authenticated entity. If it's the zero value, then the
// entity has not previously logged in.
LastConnection time.Time

// Controller reports whether or not the authenticated
// entity is a controller agent.
Controller bool
Expand Down
19 changes: 5 additions & 14 deletions apiserver/stateauthenticator/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"net/http"
"strconv"
"strings"
"time"

"github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery"
"github.com/go-macaroon-bakery/macaroon-bakery/v3/httpbakery"
Expand Down Expand Up @@ -164,7 +163,7 @@ func (a *Authenticator) checkCreds(
req params.LoginRequest,
authTag names.Tag,
userLogin bool,
authenticator authentication.EntityAuthenticator,
authenticator authentication.Authenticator,
) (httpcontext.AuthInfo, error) {
var entityFinder authentication.EntityFinder = st
if userLogin {
Expand All @@ -186,21 +185,13 @@ func (a *Authenticator) checkCreds(
authInfo.Controller = entity.IsManager()
}

type withLastLogin interface {
LastLogin() (time.Time, error)
type withLogin interface {
UpdateLastLogin() error
}
if entity, ok := entity.(withLastLogin); ok {
lastLogin, err := entity.LastLogin()
if err == nil {
authInfo.LastConnection = lastLogin
} else if !state.IsNeverLoggedInError(err) {
return httpcontext.AuthInfo{}, errors.Trace(err)
if entity, ok := entity.(withLogin); ok {
if err := entity.UpdateLastLogin(); err != nil {
logger.Warningf("updating last login time for %v", authTag)
}
// TODO log or return error returned by
// UpdateLastLogin? Old code didn't do
// anything with it.
_ = entity.UpdateLastLogin()
}
return authInfo, nil
}
Expand Down
6 changes: 3 additions & 3 deletions apiserver/stateauthenticator/authenticator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,21 +54,21 @@ func (s *agentAuthenticatorSuite) TestAuthenticatorForTag(c *gc.C) {
func (s *agentAuthenticatorSuite) TestMachineGetsAgentAuthenticator(c *gc.C) {
authenticator, err := stateauthenticator.EntityAuthenticator(s.authenticator, names.NewMachineTag("0"))
c.Assert(err, jc.ErrorIsNil)
_, ok := authenticator.(*authentication.AgentAuthenticator)
_, ok := authenticator.(*authentication.EntityAuthenticator)
c.Assert(ok, jc.IsTrue)
}

func (s *agentAuthenticatorSuite) TestModelGetsAgentAuthenticator(c *gc.C) {
authenticator, err := stateauthenticator.EntityAuthenticator(s.authenticator, names.NewModelTag("deadbeef-0bad-400d-8000-4b1d0d06f00d"))
c.Assert(err, jc.ErrorIsNil)
_, ok := authenticator.(*authentication.AgentAuthenticator)
_, ok := authenticator.(*authentication.EntityAuthenticator)
c.Assert(ok, jc.IsTrue)
}

func (s *agentAuthenticatorSuite) TestUnitGetsAgentAuthenticator(c *gc.C) {
authenticator, err := stateauthenticator.EntityAuthenticator(s.authenticator, names.NewUnitTag("wordpress/0"))
c.Assert(err, jc.ErrorIsNil)
_, ok := authenticator.(*authentication.AgentAuthenticator)
_, ok := authenticator.(*authentication.EntityAuthenticator)
c.Assert(ok, jc.IsTrue)
}

Expand Down
Loading

0 comments on commit 2d7cc4f

Please sign in to comment.