Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AUTH-541: OIDC structured auth config #713

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
292 changes: 292 additions & 0 deletions pkg/controllers/externaloidc/externaloidc_controller.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,292 @@
package externaloidc

import (
"context"
"crypto/tls"
"crypto/x509"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"strings"
"time"

configv1 "github.com/openshift/api/config/v1"
configinformers "github.com/openshift/client-go/config/informers/externalversions"
configv1listers "github.com/openshift/client-go/config/listers/config/v1"
"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/events"
"github.com/openshift/library-go/pkg/operator/resource/retry"
"github.com/openshift/library-go/pkg/operator/v1helpers"
"golang.org/x/net/http/httpproxy"

"k8s.io/apimachinery/pkg/api/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
apiserverv1beta1 "k8s.io/apiserver/pkg/apis/apiserver/v1beta1"
corev1ac "k8s.io/client-go/applyconfigurations/core/v1"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
corev1listers "k8s.io/client-go/listers/core/v1"
"k8s.io/client-go/util/cert"
"k8s.io/utils/ptr"
)

const (
configNamespace = "openshift-config"
managedNamespace = "openshift-config-managed"
targetAuthConfigCMName = "auth-config"
authConfigDataKey = "auth-config.json"
oidcDiscoveryEndpointPath = "/.well-known/openid-configuration"
)

type externalOIDCController struct {
name string
eventName string
authLister configv1listers.AuthenticationLister
configMapLister corev1listers.ConfigMapLister
configMaps corev1client.ConfigMapsGetter
}

func NewExternalOIDCController(
kubeInformersForNamespaces v1helpers.KubeInformersForNamespaces,
configInformer configinformers.SharedInformerFactory,
operatorClient v1helpers.OperatorClient,
configMaps corev1client.ConfigMapsGetter,
recorder events.Recorder,
) factory.Controller {

c := &externalOIDCController{
name: "ExternalOIDCController",
eventName: "external-oidc-controller",

authLister: configInformer.Config().V1().Authentications().Lister(),
configMapLister: kubeInformersForNamespaces.ConfigMapLister(),
configMaps: configMaps,
}

return factory.New().WithInformers(
// track openshift-config for changes to the provider's CA bundle
kubeInformersForNamespaces.InformersFor(configNamespace).Core().V1().ConfigMaps().Informer(),
// track auth resource
configInformer.Config().V1().Authentications().Informer(),
).WithFilteredEventsInformers(
// track openshift-config-managed/auth-config cm in case it gets changed externally
factory.NamesFilter(targetAuthConfigCMName),
kubeInformersForNamespaces.InformersFor(managedNamespace).Core().V1().ConfigMaps().Informer(),
).WithSync(c.sync).
WithSyncDegradedOnError(operatorClient).
ToController(c.name, recorder.WithComponentSuffix(c.eventName))
}

func (c *externalOIDCController) sync(ctx context.Context, syncCtx factory.SyncContext) error {
auth, err := c.authLister.Get("cluster")
if err != nil {
return fmt.Errorf("could not get authentication/cluster: %v", err)
}

if auth.Spec.Type != configv1.AuthenticationTypeOIDC {
// auth type is "IntegratedOAuth", "" or "None"; delete structured auth configmap if it exists
if _, err := c.configMapLister.ConfigMaps(managedNamespace).Get(targetAuthConfigCMName); errors.IsNotFound(err) {
return nil
} else if err != nil {
return err
}

if err := c.configMaps.ConfigMaps(managedNamespace).Delete(ctx, targetAuthConfigCMName, metav1.DeleteOptions{}); err == nil {
Copy link
Contributor

@ibihim ibihim Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So at this line, we are in !featureGates.Enabled(features.FeatureGateExternalOIDC) == false, which means that the features is enabled. Now we check if Auth isn't set to external OIDC and if it isn't, we attempt to delete the resource.

Wouldn't it be more beneficial to look up in the Informer Cache if it exists, before attempting to delete it? That way we could save a call to the apiserver? It is good to know as this controller might get triggered a couple of times.

syncCtx.Recorder().Eventf(c.eventName, "Removed auth configmap %s/%s", managedNamespace, targetAuthConfigCMName)

} else if !apierrors.IsNotFound(err) {
return fmt.Errorf("could not delete existing configmap %s/%s: %v", managedNamespace, targetAuthConfigCMName, err)
}

return nil
}

authConfig, err := c.generateAuthConfig(*auth)
if err != nil {
return err
}

b, err := json.Marshal(authConfig)
if err != nil {
return fmt.Errorf("could not marshal auth config into JSON: %v", err)
}
authConfigJSON := string(b)

existingCM, err := c.configMapLister.ConfigMaps(managedNamespace).Get(targetAuthConfigCMName)
if err != nil && !apierrors.IsNotFound(err) {
return fmt.Errorf("could not retrieve auth configmap %s/%s to check data before sync: %v", managedNamespace, targetAuthConfigCMName, err)
}

if existingCM != nil && existingCM.Data[authConfigDataKey] == authConfigJSON {
return nil
}

if err := validateAuthenticationConfiguration(*authConfig); err != nil {
return fmt.Errorf("auth config validation failed: %v", err)
}

cm := corev1ac.ConfigMap(targetAuthConfigCMName, managedNamespace).WithData(map[string]string{authConfigDataKey: authConfigJSON})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been trying to reduce no-op Apply requests by extracting the current apply configuration from the local informer's cache with https://pkg.go.dev/k8s.io/client-go/applyconfigurations/core/v1#ExtractConfigMap and doing a apiequality.Semantic.DeepEqual between them first. I haven't heard of any issues arising from that approach yet, does it make sense to do it here to avoid making a write on every resync?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the intention of this check: https://github.com/openshift/cluster-authentication-operator/pull/713/files#diff-3c99f304cc2949488aa2fa2b8aea2d7e8ddb0c8baa42e66f128eacd7cdbda11aR135-R137

	if existingCM != nil && existingCM.Data[authConfigDataKey] == authConfigJSON {
		return nil
	}

Do you think the DeepEqual() is preferable?

if _, err := c.configMaps.ConfigMaps(managedNamespace).Apply(ctx, cm, metav1.ApplyOptions{FieldManager: c.name, Force: true}); err != nil {
return fmt.Errorf("could not apply changes to auth configmap %s/%s: %v", managedNamespace, targetAuthConfigCMName, err)
}

syncCtx.Recorder().Eventf(c.eventName, "Synced auth configmap %s/%s", managedNamespace, targetAuthConfigCMName)

return nil
}

// generateAuthConfig creates a structured JWT AuthenticationConfiguration for OIDC
// from the configuration found in the authentication/cluster resource
func (c *externalOIDCController) generateAuthConfig(auth configv1.Authentication) (*apiserverv1beta1.AuthenticationConfiguration, error) {
authConfig := apiserverv1beta1.AuthenticationConfiguration{
TypeMeta: metav1.TypeMeta{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, by side-stepping api-machinery and using json.Marshal we need to set it by hand. Do we do that in other places as well or do we leverage the serializer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apiserverv1beta1.AuthenticationConfiguration won't go via an API call, but instead it will be serialized into a json file and read back by the kube-apiserver, so for the moment there's no apimachinery to handle this AFAIK.

Copy link
Contributor

@ibihim ibihim Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has a register.go. We would need to create a runtime.Scheme and add the object to the scheme, to then be able to use the codec for serialization and deserialization?

I am not sure, I am not an expert in api machinery topics and can't tell if it is "the idiomatic way" or if it is a "waste of time!".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, wasn't very familiar with schemes, TIL 👍 Pushing a fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, that we use the codec, we can drop the TypeMeta, right?

Kind: "AuthenticationConfiguration",
APIVersion: "apiserver.config.k8s.io/v1beta1",
},
}

for _, provider := range auth.Spec.OIDCProviders {
jwt := apiserverv1beta1.JWTAuthenticator{
Issuer: apiserverv1beta1.Issuer{
URL: provider.Issuer.URL,
AudienceMatchPolicy: apiserverv1beta1.AudienceMatchPolicyMatchAny,
},
ClaimMappings: apiserverv1beta1.ClaimMappings{
Username: apiserverv1beta1.PrefixedClaimOrExpression{
Claim: provider.ClaimMappings.Username.Claim,
},
Groups: apiserverv1beta1.PrefixedClaimOrExpression{
Claim: provider.ClaimMappings.Groups.Claim,
Prefix: &provider.ClaimMappings.Groups.Prefix,
},
},
}

if len(provider.Issuer.Audiences) > 0 {
jwt.Issuer.Audiences = make([]string, 0, len(provider.Issuer.Audiences))
for _, aud := range provider.Issuer.Audiences {
jwt.Issuer.Audiences = append(jwt.Issuer.Audiences, string(aud))
}
}

if len(provider.Issuer.CertificateAuthority.Name) > 0 {
caConfigMap, err := c.configMapLister.ConfigMaps(configNamespace).Get(provider.Issuer.CertificateAuthority.Name)
if err != nil {
return nil, fmt.Errorf("could not retrieve auth configmap %s/%s to check CA bundle: %v", configNamespace, provider.Issuer.CertificateAuthority.Name, err)
}

caData, ok := caConfigMap.Data["ca-bundle.crt"]
if !ok || len(caData) == 0 {
return nil, fmt.Errorf("configmap %s/%s key \"ca-bundle.crt\" missing or empty", configNamespace, provider.Issuer.CertificateAuthority.Name)
}

jwt.Issuer.CertificateAuthority = caData
}

switch provider.ClaimMappings.Username.PrefixPolicy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no default case, right? Or do we want to err if unset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UsernamePrefixPolicy type won't allow any other value: https://github.com/liouk/cluster-authentication-operator/blob/de4ac96b4cd7c9f8de3a966d69ab31035403decb/vendor/github.com/openshift/api/config/v1/types_authentication.go#L410

If you prefer, for completeness, I can add a default with an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so there will be a validation error, when someone tries to create one with another value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, this will be rejected by the API when someone will try to edit the authentication/cluster resource.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if a new valid option is added in the future? Can there be a period of time during an upgrade from N to N+1 where there is a cluster-authentication-operator at N and a CRD at N+1? I would include a default case to avoid all doubt.

case configv1.NoOpinion:
jwt.ClaimMappings.Username.Prefix = ptr.To("")
case configv1.NoPrefix:
jwt.ClaimMappings.Username.Prefix = ptr.To("-")
case configv1.Prefix:
if provider.ClaimMappings.Username.Prefix == nil {
return nil, fmt.Errorf("nil username prefix while policy expects one")
} else {
jwt.ClaimMappings.Username.Prefix = &provider.ClaimMappings.Username.Prefix.PrefixString
}
default:
return nil, fmt.Errorf("invalid username prefix policy: %s", provider.ClaimMappings.Username.PrefixPolicy)
}

for i, rule := range provider.ClaimValidationRules {
if rule.RequiredClaim == nil {
return nil, fmt.Errorf("empty validation rule at index %d", i)
}

jwt.ClaimValidationRules = append(jwt.ClaimValidationRules, apiserverv1beta1.ClaimValidationRule{
Claim: rule.RequiredClaim.Claim,
RequiredValue: rule.RequiredClaim.RequiredValue,
})
}

authConfig.JWT = append(authConfig.JWT, jwt)
}

return &authConfig, nil
}

// validateAuthenticationConfiguration performs validations that are not done at the server-side,
// including validation that the provided CA cert (or system CAs if not specified) can be used for
// TLS cert verification
func validateAuthenticationConfiguration(auth apiserverv1beta1.AuthenticationConfiguration) error {
for _, jwt := range auth.JWT {
var caCertPool *x509.CertPool
var err error
if len(jwt.Issuer.CertificateAuthority) > 0 {
caCertPool, err = cert.NewPoolFromBytes([]byte(jwt.Issuer.CertificateAuthority))
if err != nil {
return fmt.Errorf("issuer CA is invalid: %v", err)
}
}

// make sure we can access the issuer with the given cert pool (system CAs used if pool is empty)
if err := validateCACert(jwt.Issuer.URL, caCertPool); err != nil {
certMessage := "using the specified CA cert"
if caCertPool == nil {
certMessage = "using the system CAs"
}
return fmt.Errorf("could not validate IDP URL %s: %v", certMessage, err)
}
}

return nil
}

// validateCACert makes a request to the provider's well-known endpoint using the
// specified CA cert pool to validate that the certs in the pool match the host
func validateCACert(hostURL string, caCertPool *x509.CertPool) error {
client := &http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{RootCAs: caCertPool},
Proxy: func(*http.Request) (*url.URL, error) {
if proxyConfig := httpproxy.FromEnvironment(); len(proxyConfig.HTTPSProxy) > 0 {
return url.Parse(proxyConfig.HTTPSProxy)
}
return nil, nil
},
},
Timeout: 5 * time.Second,
}

wellKnown := strings.TrimSuffix(hostURL, "/") + oidcDiscoveryEndpointPath
req, err := http.NewRequest(http.MethodGet, wellKnown, nil)
if err != nil {
return fmt.Errorf("could not create well-known HTTP request: %v", err)
}

var resp *http.Response
var connErr error
retryCtx, cancel := context.WithTimeout(req.Context(), 10*time.Second)
defer cancel()
retry.RetryOnConnectionErrors(retryCtx, func(ctx context.Context) (done bool, err error) {
resp, connErr = client.Do(req)
return connErr == nil, connErr
})
if connErr != nil {
return fmt.Errorf("GET well-known error: %v", connErr)
}
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
body, err := io.ReadAll(resp.Body)
if err != nil {
return fmt.Errorf("unable to read response body; HTTP status: %s; error: %v", resp.Status, err)
}

return fmt.Errorf("unexpected well-known status code %s: %s", resp.Status, body)
}

return nil
}
Loading