Skip to content

Commit

Permalink
Add API events for SA OAuth failures
Browse files Browse the repository at this point in the history 
Collect errors during validation of a service account's OAuth
configuration, logging them to a warning event upon a fatal error.

Signed-off-by: Matt Rogers <[email protected]>
  • Loading branch information
Matt Rogers committed Sep 19, 2017
1 parent 01d9694 commit 543ec16
Show file tree
Hide file tree
Showing 6 changed files with 163 additions and 40 deletions.
1 change: 1 addition & 0 deletions pkg/cmd/server/origin/oauth_apiserver_adapter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func NewOAuthServerConfigFromMasterConfig(masterConfig *MasterConfig) (*oauthapi
// TODO pass a privileged client config through during construction. It is NOT a loopback client.
oauthServerConfig.OpenShiftClient = masterConfig.PrivilegedLoopbackOpenShiftClient
oauthServerConfig.KubeClient = masterConfig.PrivilegedLoopbackKubernetesClientsetInternal
oauthServerConfig.KubeExternalClient = masterConfig.PrivilegedLoopbackKubernetesClientsetExternal

// Build the list of valid redirect_uri prefixes for a login using the openshift-web-console client to redirect to
if !options.DisabledFeatures.Has(configapi.FeatureWebConsole) {
Expand Down
16 changes: 15 additions & 1 deletion pkg/oauth/apiserver/apiserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import (
"k8s.io/apimachinery/pkg/runtime/serializer"
"k8s.io/apiserver/pkg/registry/rest"
genericapiserver "k8s.io/apiserver/pkg/server"
corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
restclient "k8s.io/client-go/rest"
kclientsetexternal "k8s.io/kubernetes/pkg/client/clientset_generated/clientset"
coreclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion"

osclient "github.com/openshift/origin/pkg/client"
Expand Down Expand Up @@ -121,8 +123,20 @@ func (c *OAuthAPIServerConfig) newV1RESTStorage() (map[string]rest.Storage, erro
if err != nil {
return nil, err
}
kubeClient, err := kclientsetexternal.NewForConfig(c.CoreAPIServerClientConfig)
if err != nil {
return nil, err
}

combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(coreClient, coreClient, deprecatedClient, oauthClient.OAuthClients(), saAccountGrantMethod)
combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
coreClient,
coreClient,
// TODO: fix this construction
corev1.New(kubeClient.Core().RESTClient()).Events(""),
deprecatedClient,
oauthClient.OAuthClients(),
saAccountGrantMethod,
)
authorizeTokenStorage, err := authorizetokenetcd.NewREST(c.GenericConfig.RESTOptionsGetter, combinedOAuthClientGetter)
if err != nil {
return nil, fmt.Errorf("error building REST storage: %v", err)
Expand Down
11 changes: 10 additions & 1 deletion pkg/oauth/apiserver/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"k8s.io/apiserver/pkg/authentication/request/union"
x509request "k8s.io/apiserver/pkg/authentication/request/x509"
kuser "k8s.io/apiserver/pkg/authentication/user"
corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/kubernetes/pkg/client/retry"

"github.com/openshift/origin/pkg/auth/authenticator/challenger/passwordchallenger"
Expand Down Expand Up @@ -79,7 +80,15 @@ func (c *OAuthServerConfig) WithOAuth(handler http.Handler) (http.Handler, error
// pass through all other requests
mux.Handle("/", handler)

combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(c.KubeClient.Core(), c.KubeClient.Core(), c.OpenShiftClient, c.OAuthClientClient, oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod))
combinedOAuthClientGetter := saoauth.NewServiceAccountOAuthClientGetter(
c.KubeClient.Core(),
c.KubeClient.Core(),
// TODO: simplify this construction
corev1.New(c.KubeExternalClient.Core().RESTClient()).Events(""),
c.OpenShiftClient,
c.OAuthClientClient,
oauthapi.GrantHandlerType(c.Options.GrantConfig.ServiceAccountMethod),
)

errorPageHandler, err := c.getErrorHandler()
if err != nil {
Expand Down
4 changes: 4 additions & 0 deletions pkg/oauth/apiserver/oauth_apiserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
genericfilters "k8s.io/apiserver/pkg/server/filters"
"k8s.io/client-go/rest"
kapi "k8s.io/kubernetes/pkg/api"
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 @@ -40,6 +41,9 @@ type OAuthServerConfig struct {
// KubeClient is kubeclient with enough permission for the auth API
KubeClient kclientset.Interface

// KubeExternalClient is for creating user events
KubeExternalClient kclientsetexternal.Interface

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

Expand Down
106 changes: 79 additions & 27 deletions pkg/serviceaccounts/oauthclient/oauthclientregistry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,11 @@ 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"
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 @@ -20,7 +24,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 All @@ -46,8 +49,8 @@ var modelPrefixes = []string{
// namesToObjMapperFunc is linked to a given GroupKind.
// Based on the namespace and names provided, it builds a map of resource name to redirect URIs.
// The redirect URIs represent the default values as specified by the resource.
// These values can be overridden by user specified data.
type namesToObjMapperFunc func(namespace string, names sets.String) map[string]redirectURIList
// These values can be overridden by user specified data. Errors returned are informative and non-fatal.
type namesToObjMapperFunc func(namespace string, names sets.String) (map[string]redirectURIList, []error)

var emptyGroupKind = schema.GroupKind{} // Used with static redirect URIs
var routeGroupKind = routeapi.SchemeGroupVersion.WithKind(routeKind).GroupKind()
Expand All @@ -57,9 +60,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 @@ -188,22 +192,35 @@ var _ oauthclient.Getter = &saOAuthClientAdapter{}
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,
routeClient: routeClient,
delegate: delegate,
grantMethod: grantMethod,
decoder: kapi.Codecs.UniversalDecoder(),
saClient: saClient,
secretClient: secretClient,
eventRecorder: recorder,
routeClient: routeClient,
delegate: delegate,
grantMethod: grantMethod,
decoder: kapi.Codecs.UniversalDecoder(),
}
}

func joinErrors(errors []error) string {
msg := []string{}
for _, e := range errors {
msg = append(msg, e.Error())
}
return strings.Join(msg, ",")
}

func (a *saOAuthClientAdapter) Get(name string, options metav1.GetOptions) (*oauthapi.OAuthClient, error) {
var err error
saNamespace, saName, err := apiserverserviceaccount.SplitUsername(name)
if err != nil {
return a.delegate.Get(name, options)
Expand All @@ -214,25 +231,44 @@ func (a *saOAuthClientAdapter) Get(name string, options metav1.GetOptions) (*oau
return nil, err
}

var saErrors []error
var failReason string
// Create a warning event combining the collected annotation errors upon failure.
defer func() {
if err != nil && len(saErrors) > 0 && len(failReason) > 0 {
a.eventRecorder.Eventf(sa, kapi.EventTypeWarning, failReason, "%s", joinErrors(saErrors))
}
}()

redirectURIs := []string{}
if modelsMap := parseModelsMap(sa.Annotations, a.decoder); len(modelsMap) > 0 {
if uris := a.extractRedirectURIs(modelsMap, saNamespace); len(uris) > 0 {
modelsMap, saErrors := parseModelsMap(sa.Annotations, a.decoder)
if len(modelsMap) > 0 {
uris, extractErrors := a.extractRedirectURIs(modelsMap, saNamespace)
if len(uris) > 0 {
redirectURIs = append(redirectURIs, uris.extractValidRedirectURIStrings()...)
}
if len(extractErrors) > 0 {
saErrors = append(saErrors, extractErrors...)
}
}
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"
saErrors = append(saErrors, err)
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"
saErrors = append(saErrors, err)
return nil, err
}

saWantsChallenges, _ := strconv.ParseBool(sa.Annotations[OAuthWantChallengesAnnotationPrefix])
Expand All @@ -255,9 +291,10 @@ func (a *saOAuthClientAdapter) Get(name string, options metav1.GetOptions) (*oau

// 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 {
// and serves no functional purpose other than making testing easier. Errors returned are informative and non-fatal.
func parseModelsMap(annotations map[string]string, decoder runtime.Decoder) (map[string]model, []error) {
models := map[string]model{}
parseErrors := []error{}
for key, value := range annotations {
prefix, name, ok := parseModelPrefixName(key)
if !ok {
Expand All @@ -268,16 +305,20 @@ func parseModelsMap(annotations map[string]string, decoder runtime.Decoder) map[
case OAuthRedirectModelAnnotationURIPrefix:
if u, err := url.Parse(value); err == nil {
m.updateFromURI(u)
} else {
parseErrors = append(parseErrors, err)
}
case OAuthRedirectModelAnnotationReferencePrefix:
r := &oauthapi.OAuthRedirectReference{}
if err := runtime.DecodeInto(decoder, []byte(value), r); err == nil {
m.updateFromReference(&r.Reference)
} else {
parseErrors = append(parseErrors, err)
}
}
models[name] = m
}
return models
return models, parseErrors
}

// parseModelPrefixName determines if the given key is a model prefix.
Expand All @@ -292,9 +333,10 @@ func parseModelPrefixName(key string) (string, string, bool) {
}

// extractRedirectURIs builds redirect URIs using the given models and namespace.
// The returned redirect URIs may contain duplicates and invalid entries.
func (a *saOAuthClientAdapter) extractRedirectURIs(modelsMap map[string]model, namespace string) redirectURIList {
// The returned redirect URIs may contain duplicates and invalid entries. Errors returned are informative and non-fatal.
func (a *saOAuthClientAdapter) extractRedirectURIs(modelsMap map[string]model, namespace string) (redirectURIList, []error) {
var data redirectURIList
routeErrors := []error{}
groupKindModelListMapper := map[schema.GroupKind]modelList{} // map of GroupKind to all models belonging to it
groupKindModelToURI := map[schema.GroupKind]namesToObjMapperFunc{
routeGroupKind: a.redirectURIsFromRoutes,
Expand All @@ -318,27 +360,37 @@ func (a *saOAuthClientAdapter) extractRedirectURIs(modelsMap map[string]model, n

for gk, models := range groupKindModelListMapper {
if names := models.getNames(); names.Len() > 0 {
if objMapper := groupKindModelToURI[gk](namespace, names); len(objMapper) > 0 {
objMapper, errs := groupKindModelToURI[gk](namespace, names)
if len(objMapper) > 0 {
data = append(data, models.getRedirectURIs(objMapper)...)
}
if len(errs) > 0 {
routeErrors = append(routeErrors, errs...)
}
}
}

return data
return data, routeErrors
}

// redirectURIsFromRoutes is the namesToObjMapperFunc specific to Routes.
// Returns a map of route name to redirect URIs that contain the default data as specified by the route's ingresses.
func (a *saOAuthClientAdapter) redirectURIsFromRoutes(namespace string, osRouteNames sets.String) map[string]redirectURIList {
// Errors returned are informative and non-fatal.
func (a *saOAuthClientAdapter) redirectURIsFromRoutes(namespace string, osRouteNames sets.String) (map[string]redirectURIList, []error) {
var routes []routeapi.Route
routeErrors := []error{}
routeInterface := a.routeClient.Routes(namespace)
if osRouteNames.Len() > 1 {
if r, err := routeInterface.List(metav1.ListOptions{}); err == nil {
routes = r.Items
} else {
routeErrors = append(routeErrors, err)
}
} else {
if r, err := routeInterface.Get(osRouteNames.List()[0], metav1.GetOptions{}); err == nil {
routes = append(routes, *r)
} else {
routeErrors = append(routeErrors, err)
}
}
routeMap := map[string]redirectURIList{}
Expand All @@ -347,7 +399,7 @@ func (a *saOAuthClientAdapter) redirectURIsFromRoutes(namespace string, osRouteN
routeMap[route.Name] = redirectURIsFromRoute(&route)
}
}
return routeMap
return routeMap, routeErrors
}

// redirectURIsFromRoute returns a list of redirect URIs that contain the default data as specified by the given route's ingresses.
Expand Down
Loading

0 comments on commit 543ec16

Please sign in to comment.