Skip to content

Commit

Permalink
Add API events for SA OAuth failures
Browse files Browse the repository at this point in the history 
Add user-viewable events when SA OAuth fails due to missing annotations or
tokens, returning an internal error with a hint to view `oc get events`

Signed-off-by: Matt Rogers <[email protected]>
  • Loading branch information
Matt Rogers committed Aug 1, 2017
1 parent f895b96 commit 659945d
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 23 deletions.
10 changes: 9 additions & 1 deletion pkg/cmd/server/origin/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import (
"github.com/openshift/origin/pkg/oauth/server/osinserver/registrystorage"
oauthutil "github.com/openshift/origin/pkg/oauth/util"
saoauth "github.com/openshift/origin/pkg/serviceaccounts/oauthclient"
corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
)

const (
Expand All @@ -92,7 +93,14 @@ func (c *AuthConfig) WithOAuth(handler http.Handler) (http.Handler, error) {
return nil, err
}
clientRegistry := clientregistry.NewRegistry(clientStorage)
combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(c.KubeClient.Core(), c.KubeClient.Core(), c.OpenShiftClient, clientRegistry, oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod))
combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
c.KubeClient.Core(),
c.KubeClient.Core(),
corev1.New(c.KubeExternalClient.Core().RESTClient()).Events(""),
c.OpenShiftClient,
clientRegistry,
oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod),
)

accessTokenStorage, err := accesstokenetcd.NewREST(c.RESTOptionsGetter, combinedOAuthClientGetter, c.EtcdBackends...)
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions pkg/cmd/server/origin/auth_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/pborman/uuid"

"k8s.io/apiserver/pkg/storage"
kclientsetexternal "k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
kclientset "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"

"github.com/openshift/origin/pkg/auth/server/session"
Expand All @@ -30,6 +31,8 @@ type AuthConfig struct {
// KubeClient is kubeclient with enough permission for the auth API
KubeClient kclientset.Interface

KubeExternalClient kclientsetexternal.Interface

// OpenShiftClient is osclient with enough permission for the auth API
OpenShiftClient osclient.Interface

Expand All @@ -52,6 +55,7 @@ type AuthConfig struct {
func BuildAuthConfig(masterConfig *MasterConfig) (*AuthConfig, error) {
options := masterConfig.Options
osClient, kubeClient := masterConfig.OAuthServerClients()
kubeExternalClient := masterConfig.KubeClientsetExternal()

var sessionAuth *session.Authenticator
var sessionHandlerWrapper handlerWrapper
Expand Down Expand Up @@ -87,6 +91,7 @@ func BuildAuthConfig(masterConfig *MasterConfig) (*AuthConfig, error) {
Options: *options.OAuthConfig,

KubeClient: kubeClient,
KubeExternalClient: kubeExternalClient,

OpenShiftClient: osClient,

Expand Down
11 changes: 10 additions & 1 deletion pkg/cmd/server/origin/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
apirequest "k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/apiserver/pkg/registry/rest"
corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
restclient "k8s.io/client-go/rest"
"k8s.io/client-go/util/flowcontrol"
kapi "k8s.io/kubernetes/pkg/api"
Expand Down Expand Up @@ -352,7 +353,15 @@ func (c OpenshiftAPIConfig) GetRestStorage() (map[schema.GroupVersion]map[string
saAccountGrantMethod = oauthapi.GrantHandlerType(c.ServiceAccountMethod)
}

combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(c.KubeClientInternal.Core(), c.KubeClientInternal.Core(), c.DeprecatedOpenshiftClient, clientRegistry, saAccountGrantMethod)
combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
c.KubeClientInternal.Core(),
c.KubeClientInternal.Core(),
corev1.New(c.KubeClientExternal.Core().RESTClient()).Events(""),
c.DeprecatedOpenshiftClient,
clientRegistry,
saAccountGrantMethod,
)

authorizeTokenStorage, err := authorizetokenetcd.NewREST(c.GenericConfig.RESTOptionsGetter, combinedOAuthClientGetter)
if err != nil {
return nil, fmt.Errorf("error building REST storage: %v", err)
Expand Down
51 changes: 38 additions & 13 deletions pkg/serviceaccounts/oauthclient/oauthclientregistry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"
apiserverserviceaccount "k8s.io/apiserver/pkg/authentication/serviceaccount"
apirequest "k8s.io/apiserver/pkg/endpoints/request"
corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
clientv1 "k8s.io/client-go/pkg/api/v1"
"k8s.io/client-go/tools/record"
kapi "k8s.io/kubernetes/pkg/api"
kcoreclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion"
"k8s.io/kubernetes/pkg/serviceaccount"
Expand All @@ -21,7 +25,6 @@ import (
oauthapi "github.com/openshift/origin/pkg/oauth/apis/oauth"
"github.com/openshift/origin/pkg/oauth/registry/oauthclient"
routeapi "github.com/openshift/origin/pkg/route/apis/route"
"k8s.io/apimachinery/pkg/util/sets"
)

const (
Expand Down Expand Up @@ -58,9 +61,10 @@ var legacyRouteGroupKind = routeapi.LegacySchemeGroupVersion.WithKind(routeKind)
// var ingressGroupKind = routeapi.SchemeGroupVersion.WithKind(IngressKind).GroupKind()

type saOAuthClientAdapter struct {
saClient kcoreclient.ServiceAccountsGetter
secretClient kcoreclient.SecretsGetter
routeClient osclient.RoutesNamespacer
saClient kcoreclient.ServiceAccountsGetter
secretClient kcoreclient.SecretsGetter
eventRecorder record.EventRecorder
routeClient osclient.RoutesNamespacer
// TODO add ingress support
//ingressClient ??

Expand Down Expand Up @@ -186,11 +190,14 @@ func (uri *redirectURI) merge(m *model) {

var _ oauthclient.Getter = &saOAuthClientAdapter{}

func NewServiceAccountOAuthClientGetter(saClient kcoreclient.ServiceAccountsGetter, secretClient kcoreclient.SecretsGetter, routeClient osclient.RoutesNamespacer, delegate oauthclient.Getter, grantMethod oauthapi.GrantHandlerType) oauthclient.Getter {
return &saOAuthClientAdapter{saClient: saClient, secretClient: secretClient, routeClient: routeClient, delegate: delegate, grantMethod: grantMethod, decoder: kapi.Codecs.UniversalDecoder()}
func NewServiceAccountOAuthClientGetter(saClient kcoreclient.ServiceAccountsGetter, secretClient kcoreclient.SecretsGetter, eventClient corev1.EventInterface, routeClient osclient.RoutesNamespacer, delegate oauthclient.Getter, grantMethod oauthapi.GrantHandlerType) oauthclient.Getter {
eventBroadcaster := record.NewBroadcaster()
eventBroadcaster.StartRecordingToSink(&corev1.EventSinkImpl{Interface: eventClient})
recorder := eventBroadcaster.NewRecorder(kapi.Scheme, clientv1.EventSource{Component: "service-account-oauth-client-getter"})
return &saOAuthClientAdapter{saClient: saClient, secretClient: secretClient, eventRecorder: recorder, routeClient: routeClient, delegate: delegate, grantMethod: grantMethod, decoder: kapi.Codecs.UniversalDecoder()}
}

func (a *saOAuthClientAdapter) GetClient(ctx apirequest.Context, name string, options *metav1.GetOptions) (*oauthapi.OAuthClient, error) {
func (a *saOAuthClientAdapter) GetClient(ctx apirequest.Context, name string, options *metav1.GetOptions) (saClient *oauthapi.OAuthClient, err error) {
saNamespace, saName, err := apiserverserviceaccount.SplitUsername(name)
if err != nil {
return a.delegate.GetClient(ctx, name, options)
Expand All @@ -201,30 +208,44 @@ func (a *saOAuthClientAdapter) GetClient(ctx apirequest.Context, name string, op
return nil, err
}

failEvents := []string{}
var failReason string
// Create a warning event upon failure
defer func() {
if err != nil && len(failEvents) > 0 && len(failReason) > 0 {
a.eventRecorder.Eventf(sa, kapi.EventTypeWarning, failReason, "%s", strings.Join(failEvents, ","))
}
}()

redirectURIs := []string{}
if modelsMap := parseModelsMap(sa.Annotations, a.decoder); len(modelsMap) > 0 {
if modelsMap := parseModelsMap(sa.Annotations, a.decoder, &failEvents); len(modelsMap) > 0 {
if uris := a.extractRedirectURIs(modelsMap, saNamespace); len(uris) > 0 {
redirectURIs = append(redirectURIs, uris.extractValidRedirectURIStrings()...)
}
}
if len(redirectURIs) == 0 {
return nil, fmt.Errorf(
"%v has no redirectURIs; set %v<some-value>=<redirect> or create a dynamic URI using %v<some-value>=<reference>",
err = fmt.Errorf("%v has no redirectURIs; set %v<some-value>=<redirect> or create a dynamic URI using %v<some-value>=<reference>",
name, OAuthRedirectModelAnnotationURIPrefix, OAuthRedirectModelAnnotationReferencePrefix,
)
failReason = "NoSAOAuthRedirectURIs"
failEvents = append(failEvents, err.Error())
return nil, err
}

tokens, err := a.getServiceAccountTokens(sa)
if err != nil {
return nil, err
}
if len(tokens) == 0 {
return nil, fmt.Errorf("%v has no tokens", name)
err = fmt.Errorf("%v has no tokens", name)
failReason = "NoSAOAuthTokens"
failEvents = append(failEvents, err.Error())
return nil, err
}

saWantsChallenges, _ := strconv.ParseBool(sa.Annotations[OAuthWantChallengesAnnotationPrefix])

saClient := &oauthapi.OAuthClient{
saClient = &oauthapi.OAuthClient{
ObjectMeta: metav1.ObjectMeta{Name: name},
ScopeRestrictions: getScopeRestrictionsFor(saNamespace, saName),
AdditionalSecrets: tokens,
Expand All @@ -243,7 +264,7 @@ func (a *saOAuthClientAdapter) GetClient(ctx apirequest.Context, name string, op
// parseModelsMap builds a map of model name to model using a service account's annotations.
// The model name is only used for building the map (it ties together the uri and reference annotations)
// and serves no functional purpose other than making testing easier.
func parseModelsMap(annotations map[string]string, decoder runtime.Decoder) map[string]model {
func parseModelsMap(annotations map[string]string, decoder runtime.Decoder, fails *[]string) map[string]model {
models := map[string]model{}
for key, value := range annotations {
prefix, name, ok := parseModelPrefixName(key)
Expand All @@ -255,11 +276,15 @@ func parseModelsMap(annotations map[string]string, decoder runtime.Decoder) map[
case OAuthRedirectModelAnnotationURIPrefix:
if u, err := url.Parse(value); err == nil {
m.updateFromURI(u)
} else {
*fails = append(*fails, fmt.Sprintf("failed to parse SA annotation %q: %s", prefix, err.Error()))
}
case OAuthRedirectModelAnnotationReferencePrefix:
r := &oauthapi.OAuthRedirectReference{}
if err := runtime.DecodeInto(decoder, []byte(value), r); err == nil {
m.updateFromReference(&r.Reference)
} else {
*fails = append(*fails, fmt.Sprintf("failed to decode SA annotation %q: %s", prefix, err.Error()))
}
}
models[name] = m
Expand Down
38 changes: 31 additions & 7 deletions pkg/serviceaccounts/oauthclient/oauthclientregistry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
apirequest "k8s.io/apiserver/pkg/endpoints/request"
clientgotesting "k8s.io/client-go/testing"
"k8s.io/client-go/tools/record"
kapi "k8s.io/kubernetes/pkg/api"
kapihelper "k8s.io/kubernetes/pkg/api/helper"
"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
Expand Down Expand Up @@ -41,6 +42,7 @@ func TestGetClient(t *testing.T) {

expectedDelegation bool
expectedErr string
expectedEventMsg string
expectedClient *oauthapi.OAuthClient
expectedKubeActions []clientgotesting.Action
expectedOSActions []clientgotesting.Action
Expand Down Expand Up @@ -76,6 +78,7 @@ func TestGetClient(t *testing.T) {
}),
osClient: ostestclient.NewSimpleFake(),
expectedErr: `system:serviceaccount:ns-01:default has no redirectURIs; set serviceaccounts.openshift.io/oauth-redirecturi.<some-value>`,
expectedEventMsg: `Warning NoSAOAuthRedirectURIs system:serviceaccount:ns-01:default has no redirectURIs; set serviceaccounts.openshift.io/oauth-redirecturi.<some-value>=<redirect> or create a dynamic URI using serviceaccounts.openshift.io/oauth-redirectreference.<some-value>=<reference>`,
expectedKubeActions: []clientgotesting.Action{clientgotesting.NewGetAction(serviceAccountsResource, "ns-01", "default")},
expectedOSActions: []clientgotesting.Action{},
},
Expand All @@ -90,8 +93,9 @@ func TestGetClient(t *testing.T) {
Annotations: map[string]string{OAuthRedirectModelAnnotationURIPrefix + "one": "http://anywhere"},
},
}),
osClient: ostestclient.NewSimpleFake(),
expectedErr: `system:serviceaccount:ns-01:default has no tokens`,
osClient: ostestclient.NewSimpleFake(),
expectedErr: `system:serviceaccount:ns-01:default has no tokens`,
expectedEventMsg: `Warning NoSAOAuthTokens system:serviceaccount:ns-01:default has no tokens`,
expectedKubeActions: []clientgotesting.Action{
clientgotesting.NewGetAction(serviceAccountsResource, "ns-01", "default"),
clientgotesting.NewListAction(secretsResource, secretKind, "ns-01", metav1.ListOptions{}),
Expand Down Expand Up @@ -547,7 +551,17 @@ func TestGetClient(t *testing.T) {

for _, tc := range testCases {
delegate := &fakeDelegate{}
getter := NewServiceAccountOAuthClientGetter(tc.kubeClient.Core(), tc.kubeClient.Core(), tc.osClient, delegate, oauthapi.GrantHandlerPrompt)
fakerecorder := record.NewFakeRecorder(100)
getter := saOAuthClientAdapter{
saClient: tc.kubeClient.Core(),
secretClient: tc.kubeClient.Core(),
eventRecorder: fakerecorder,
routeClient: tc.osClient,
delegate: delegate,
grantMethod: oauthapi.GrantHandlerPrompt,
decoder: kapi.Codecs.UniversalDecoder(),
}

client, err := getter.GetClient(apirequest.NewContext(), tc.clientName, &metav1.GetOptions{})
switch {
case len(tc.expectedErr) == 0 && err == nil:
Expand Down Expand Up @@ -577,8 +591,14 @@ func TestGetClient(t *testing.T) {
t.Errorf("%s: expected %#v, got %#v", tc.name, tc.expectedOSActions, tc.osClient.Actions())
continue
}
}

if len(tc.expectedEventMsg) > 0 {
ev := <-fakerecorder.Events
if tc.expectedEventMsg != ev {
t.Errorf("%s: expected event message %#v, got %#v", tc.name, tc.expectedEventMsg, ev)
}
}
}
}

type fakeDelegate struct {
Expand Down Expand Up @@ -816,8 +836,9 @@ func TestParseModelsMap(t *testing.T) {
},
},
} {
if !reflect.DeepEqual(test.expected, parseModelsMap(test.annotations, decoder)) {
t.Errorf("%s: expected %#v, got %#v", test.name, test.expected, parseModelsMap(test.annotations, decoder))
fails := []string{}
if !reflect.DeepEqual(test.expected, parseModelsMap(test.annotations, decoder, &fails)) {
t.Errorf("%s: expected %#v, got %#v", test.name, test.expected, parseModelsMap(test.annotations, decoder, &fails))
}
}
}
Expand Down Expand Up @@ -1183,7 +1204,10 @@ func buildRouteClient(routes []*routeapi.Route) saOAuthClientAdapter {
for _, route := range routes {
objects = append(objects, route)
}
return saOAuthClientAdapter{routeClient: ostestclient.NewSimpleFake(objects...)}
return saOAuthClientAdapter{
routeClient: ostestclient.NewSimpleFake(objects...),
eventRecorder: record.NewFakeRecorder(100),
}
}

func buildRedirectObjectReferenceString(kind, name, group string) string {
Expand Down
2 changes: 1 addition & 1 deletion vendor/github.com/RangelReale/osin/authorize.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 659945d

Please sign in to comment.