Skip to content
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
10 changes: 10 additions & 0 deletions pkg/apihelpers/apihelpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,16 @@ var (
},
},
},
{
// The Distribution registry re-reads htpasswd on mtime change,
// so credential rotation does not require a service restart.
Path: "/etc/iri-registry/auth/htpasswd",
Actions: []opv1.NodeDisruptionPolicyStatusAction{
{
Type: opv1.NoneStatusAction,
},
},
},
{
Path: constants.GPGNoRebootPath,
Actions: []opv1.NodeDisruptionPolicyStatusAction{
Expand Down
23 changes: 22 additions & 1 deletion pkg/controller/bootstrap/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ func (b *Bootstrap) Run(destDir string) error {
iri *mcfgv1alpha1.InternalReleaseImage
iriTLSCert *corev1.Secret
osImageStream *mcfgv1alpha1.OSImageStream
iriAuthSecret *corev1.Secret
)
for _, info := range infos {
if info.IsDir() {
Expand Down Expand Up @@ -202,6 +203,9 @@ func (b *Bootstrap) Run(destDir string) error {
if obj.GetName() == ctrlcommon.InternalReleaseImageTLSSecretName {
iriTLSCert = obj
}
if obj.GetName() == ctrlcommon.InternalReleaseImageAuthSecretName {
iriAuthSecret = obj
}
case *mcfgv1alpha1.OSImageStream:
// If given, it's treated as user input with config such as the default stream
osImageStream = obj
Expand Down Expand Up @@ -259,6 +263,23 @@ func (b *Bootstrap) Run(destDir string) error {
}

pullSecretBytes := pullSecret.Data[corev1.DockerConfigJsonKey]

// If IRI auth is enabled, merge the IRI registry credentials into the pull
// secret before rendering template MCs. This ensures the bootstrap-rendered
// MCs use the same pull secret content as the in-cluster IRI controller
// will produce, avoiding a rendered MachineConfig hash mismatch.
if fgHandler != nil && fgHandler.Enabled(features.FeatureGateNoRegistryClusterInstall) {
if iri != nil && iriAuthSecret != nil {
password := string(iriAuthSecret.Data["password"])
merged, mergeErr := internalreleaseimage.MergeIRIAuthIntoPullSecret(pullSecretBytes, password, cconfig.Spec.DNS.Spec.BaseDomain)
if mergeErr != nil {
return fmt.Errorf("failed to merge IRI auth into pull secret during bootstrap: %w", mergeErr)
}
pullSecretBytes = merged
klog.Infof("Merged IRI registry auth into pull secret for bootstrap rendering")
}
}

iconfigs, err := template.RunBootstrap(b.templatesDir, cconfig, pullSecretBytes, apiServer)
if err != nil {
return err
Expand Down Expand Up @@ -321,7 +342,7 @@ func (b *Bootstrap) Run(destDir string) error {

if fgHandler != nil && fgHandler.Enabled(features.FeatureGateNoRegistryClusterInstall) {
if iri != nil {
iriConfigs, err := internalreleaseimage.RunInternalReleaseImageBootstrap(iri, iriTLSCert, cconfig)
iriConfigs, err := internalreleaseimage.RunInternalReleaseImageBootstrap(iri, iriTLSCert, iriAuthSecret, cconfig)
if err != nil {
return err
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/controller/common/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ const (
// InternalReleaseImageTLSSecretName is the name of the secret manifest containing the InternalReleaseImage TLS certificate.
InternalReleaseImageTLSSecretName = "internal-release-image-tls"

// InternalReleaseImageAuthSecretName is the name of the secret containing IRI registry htpasswd auth credentials.
InternalReleaseImageAuthSecretName = "internal-release-image-registry-auth"

// APIServerInstanceName is a singleton name for APIServer configuration
APIServerBootstrapFileLocation = "/etc/mcs/bootstrap/api-server/api-server.yaml"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import (
)

// RunInternalReleaseImageBootstrap generates the MachineConfig objects for InternalReleaseImage that would have been generated by syncInternalReleaseImage.
func RunInternalReleaseImageBootstrap(iri *mcfgv1alpha1.InternalReleaseImage, iriSecret *corev1.Secret, cconfig *mcfgv1.ControllerConfig) ([]*mcfgv1.MachineConfig, error) {
func RunInternalReleaseImageBootstrap(iri *mcfgv1alpha1.InternalReleaseImage, iriSecret, iriAuthSecret *corev1.Secret, cconfig *mcfgv1.ControllerConfig) ([]*mcfgv1.MachineConfig, error) {
configs := []*mcfgv1.MachineConfig{}

for _, role := range SupportedRoles {
r := NewRendererByRole(role, iri, iriSecret, cconfig)
r := NewRendererByRole(role, iri, iriSecret, iriAuthSecret, cconfig)
mc, err := r.CreateEmptyMachineConfig()
if err != nil {
return nil, err
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ import (
)

func TestRunInternalReleaseImageBootstrap(t *testing.T) {
configs, err := RunInternalReleaseImageBootstrap(&mcfgv1alpha1.InternalReleaseImage{}, iriCertSecret().obj, cconfig().obj)
configs, err := RunInternalReleaseImageBootstrap(&mcfgv1alpha1.InternalReleaseImage{}, iriCertSecret().obj, iriAuthSecret().obj, cconfig().obj)
assert.NoError(t, err)
verifyAllInternalReleaseImageMachineConfigs(t, configs)
assert.Len(t, configs, 2)
verifyInternalReleaseMasterMachineConfig(t, configs[0])
verifyInternalReleaseWorkerMachineConfig(t, configs[1])
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package internalreleaseimage

import (
"bytes"
"context"
"fmt"
"reflect"
Expand Down Expand Up @@ -54,6 +55,7 @@ var (
// Controller defines the InternalReleaseImage controller.
type Controller struct {
client mcfgclientset.Interface
kubeClient clientset.Interface
eventRecorder record.EventRecorder

syncHandler func(mcp string) error
Expand Down Expand Up @@ -93,6 +95,7 @@ func New(

ctrl := &Controller{
client: mcfgClient,
kubeClient: kubeClient,
eventRecorder: ctrlcommon.NamespacedEventRecorder(eventBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: "machineconfigcontroller-internalreleaseimagecontroller"})),
queue: workqueue.NewTypedRateLimitingQueueWithConfig(
workqueue.DefaultTypedControllerRateLimiter[string](),
Expand Down Expand Up @@ -278,7 +281,8 @@ func (ctrl *Controller) updateSecret(obj, _ interface{}) {
secret := obj.(*corev1.Secret)

// Skip any event not related to the InternalReleaseImage secrets
if secret.Name != ctrlcommon.InternalReleaseImageTLSSecretName {
if secret.Name != ctrlcommon.InternalReleaseImageTLSSecretName &&
secret.Name != ctrlcommon.InternalReleaseImageAuthSecretName {
return
}

Expand Down Expand Up @@ -341,8 +345,21 @@ func (ctrl *Controller) syncInternalReleaseImage(key string) error {
return fmt.Errorf("could not get Secret %s: %w", ctrlcommon.InternalReleaseImageTLSSecretName, err)
}

iriAuthSecret, err := ctrl.secretLister.Secrets(ctrlcommon.MCONamespace).Get(ctrlcommon.InternalReleaseImageAuthSecretName)
if err != nil {
return fmt.Errorf("could not get Secret %s: %w", ctrlcommon.InternalReleaseImageAuthSecretName, err)
}

// Ensure the htpasswd field is in sync with the password field. If the
// password was rotated, this generates a new bcrypt hash and updates the
// secret before re-rendering the MachineConfig.
iriAuthSecret, err = ctrl.reconcileAuthSecret(iriAuthSecret)
if err != nil {
return fmt.Errorf("failed to reconcile IRI auth secret: %w", err)
}

for _, role := range SupportedRoles {
r := NewRendererByRole(role, iri, iriSecret, cconfig)
r := NewRendererByRole(role, iri, iriSecret, iriAuthSecret, cconfig)

mc, err := ctrl.mcLister.Get(r.GetMachineConfigName())
isNotFound := errors.IsNotFound(err)
Expand All @@ -369,6 +386,11 @@ func (ctrl *Controller) syncInternalReleaseImage(key string) error {
}
}

// Merge IRI auth credentials into the global pull secret
if err := ctrl.mergeIRIAuthIntoPullSecret(cconfig, iriAuthSecret); err != nil {
return fmt.Errorf("failed to merge IRI auth into pull secret: %w", err)
}

// Initialize status if empty
if err := ctrl.initializeInternalReleaseImageStatus(iri); err != nil {
return err
Expand Down Expand Up @@ -453,6 +475,74 @@ func (ctrl *Controller) addFinalizerToInternalReleaseImage(iri *mcfgv1alpha1.Int
return err
}

func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error {
password := string(authSecret.Data["password"])
if password == "" {
return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name)
}

baseDomain := cconfig.Spec.DNS.Spec.BaseDomain

// Fetch current pull secret from openshift-config
pullSecret, err := ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Get(
context.TODO(), ctrlcommon.GlobalPullSecretName, metav1.GetOptions{})
if err != nil {
return fmt.Errorf("could not get pull-secret: %w", err)
}

mergedBytes, err := MergeIRIAuthIntoPullSecret(pullSecret.Data[corev1.DockerConfigJsonKey], password, baseDomain)
if err != nil {
return err
}

// No change needed
if bytes.Equal(mergedBytes, pullSecret.Data[corev1.DockerConfigJsonKey]) {
return nil
}

pullSecret.Data[corev1.DockerConfigJsonKey] = mergedBytes
_, err = ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Update(
context.TODO(), pullSecret, metav1.UpdateOptions{})
if err == nil {
klog.Infof("Updated pull secret with IRI registry auth credentials from secret %s/%s (uid=%s, resourceVersion=%s)", authSecret.Namespace, authSecret.Name, authSecret.UID, authSecret.ResourceVersion)
}
return err
}
Comment on lines +478 to +510
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Potential nil pointer dereference on cconfig.Spec.DNS.

Line 484 accesses cconfig.Spec.DNS.Spec.BaseDomain without checking if DNS is nil. If the ControllerConfig doesn't have DNS configured, this will panic.

🛡️ Proposed fix: add nil guard
 func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error {
 	password := string(authSecret.Data["password"])
 	if password == "" {
 		return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name)
 	}

+	if cconfig.Spec.DNS == nil {
+		return fmt.Errorf("ControllerConfig DNS not configured, cannot determine IRI registry host")
+	}
 	baseDomain := cconfig.Spec.DNS.Spec.BaseDomain
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error {
password := string(authSecret.Data["password"])
if password == "" {
return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name)
}
baseDomain := cconfig.Spec.DNS.Spec.BaseDomain
// Fetch current pull secret from openshift-config
pullSecret, err := ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Get(
context.TODO(), ctrlcommon.GlobalPullSecretName, metav1.GetOptions{})
if err != nil {
return fmt.Errorf("could not get pull-secret: %w", err)
}
mergedBytes, err := MergeIRIAuthIntoPullSecret(pullSecret.Data[corev1.DockerConfigJsonKey], password, baseDomain)
if err != nil {
return err
}
// No change needed
if bytes.Equal(mergedBytes, pullSecret.Data[corev1.DockerConfigJsonKey]) {
return nil
}
pullSecret.Data[corev1.DockerConfigJsonKey] = mergedBytes
_, err = ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Update(
context.TODO(), pullSecret, metav1.UpdateOptions{})
if err == nil {
klog.Infof("Updated pull secret with IRI registry auth credentials from secret %s/%s (uid=%s, resourceVersion=%s)", authSecret.Namespace, authSecret.Name, authSecret.UID, authSecret.ResourceVersion)
}
return err
}
func (ctrl *Controller) mergeIRIAuthIntoPullSecret(cconfig *mcfgv1.ControllerConfig, authSecret *corev1.Secret) error {
password := string(authSecret.Data["password"])
if password == "" {
return fmt.Errorf("IRI auth secret %s/%s has empty password", authSecret.Namespace, authSecret.Name)
}
if cconfig.Spec.DNS == nil {
return fmt.Errorf("ControllerConfig DNS not configured, cannot determine IRI registry host")
}
baseDomain := cconfig.Spec.DNS.Spec.BaseDomain
// Fetch current pull secret from openshift-config
pullSecret, err := ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Get(
context.TODO(), ctrlcommon.GlobalPullSecretName, metav1.GetOptions{})
if err != nil {
return fmt.Errorf("could not get pull-secret: %w", err)
}
mergedBytes, err := MergeIRIAuthIntoPullSecret(pullSecret.Data[corev1.DockerConfigJsonKey], password, baseDomain)
if err != nil {
return err
}
// No change needed
if bytes.Equal(mergedBytes, pullSecret.Data[corev1.DockerConfigJsonKey]) {
return nil
}
pullSecret.Data[corev1.DockerConfigJsonKey] = mergedBytes
_, err = ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.OpenshiftConfigNamespace).Update(
context.TODO(), pullSecret, metav1.UpdateOptions{})
if err == nil {
klog.Infof("Updated pull secret with IRI registry auth credentials from secret %s/%s (uid=%s, resourceVersion=%s)", authSecret.Namespace, authSecret.Name, authSecret.UID, authSecret.ResourceVersion)
}
return err
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/controller/internalreleaseimage/internalreleaseimage_controller.go`
around lines 478 - 510, The code in mergeIRIAuthIntoPullSecret reads
cconfig.Spec.DNS.Spec.BaseDomain without guarding for a nil DNS pointer; add a
nil check at the start of mergeIRIAuthIntoPullSecret to handle missing DNS
(e.g., if cconfig.Spec.DNS == nil) and return a clear error (or fallback
behavior) instead of dereferencing; update references to baseDomain to use the
validated value so the function never panics when ControllerConfig has no DNS
configured.


// reconcileAuthSecret ensures the htpasswd field in the IRI auth secret is in
// sync with the password field. If the password has changed (or htpasswd is
// missing), it generates a new bcrypt hash and updates the secret. This is the
// trigger for single-phase credential rotation: the updated htpasswd causes the
// MachineConfig to be re-rendered, which MCDs roll out to nodes. Brief registry
// downtime during the rollout is accepted.
func (ctrl *Controller) reconcileAuthSecret(authSecret *corev1.Secret) (*corev1.Secret, error) {
password := string(authSecret.Data["password"])
htpasswd := string(authSecret.Data["htpasswd"])

if HtpasswdMatchesPassword(htpasswd, IRIRegistryUsername, password) {
return authSecret, nil
}

klog.V(4).Infof("IRI auth secret htpasswd does not match password, regenerating for credential rotation")

newHtpasswd, err := GenerateHtpasswdEntry(IRIRegistryUsername, password)
if err != nil {
return nil, fmt.Errorf("failed to generate htpasswd: %w", err)
}

updated := authSecret.DeepCopy()
updated.Data["htpasswd"] = []byte(newHtpasswd)

result, err := ctrl.kubeClient.CoreV1().Secrets(ctrlcommon.MCONamespace).Update(
context.TODO(), updated, metav1.UpdateOptions{})
if err != nil {
return nil, fmt.Errorf("failed to update IRI auth secret: %w", err)
}

klog.Infof("Regenerated IRI auth secret htpasswd for credential rotation (secret %s/%s)", authSecret.Namespace, authSecret.Name)
return result, nil
}

func (ctrl *Controller) cascadeDelete(iri *mcfgv1alpha1.InternalReleaseImage) error {
mcName := iri.GetFinalizers()[0]
err := ctrl.client.MachineconfigurationV1().MachineConfigs().Delete(context.TODO(), mcName, metav1.DeleteOptions{})
Expand Down
Loading