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
2 changes: 1 addition & 1 deletion api/v1beta1/infraenv_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ type InfraEnvSpec struct {

// OSImageVersion is the version of OS image to use when generating the InfraEnv.
// The version should refer to an OSImage specified in the AgentServiceConfig
// (i.e. OSImageVersion should equal to an OpenshiftVersion in OSImages list).
// It needs to be equal to either an OSImage's Version or OpenshiftVersion.
// Note: OSImageVersion can't be specified along with ClusterRef while creating an InfraEnv.
// +optional
OSImageVersion string `json:"osImageVersion,omitempty"`
Expand Down

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

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

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

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

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

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

19 changes: 14 additions & 5 deletions cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ func startKubeAPIControllers(
bm bminventory.InstallerInternals,
crdEventsHandler controllers.CRDEventsHandler,
osImages versions.OSImages,
osImageResolver versions.OsImageResolver,
versionHandler versions.Handler,
releaseHandler oc.Release,
clusterApi cluster.API,
Expand Down Expand Up @@ -311,6 +312,7 @@ func startKubeAPIControllers(
ImageServiceBaseURL: Options.BMConfig.ImageServiceBaseURL,
AuthType: Options.Auth.AuthType,
OsImages: osImages,
OsImageResolver: osImageResolver,
PullSecretHandler: controllers.NewPullSecretHandler(c, r, bm),
InsecureIPXEURLs: generateInsecureIPXEURLs,
ImageServiceEnabled: Options.EnableImageService,
Expand Down Expand Up @@ -536,9 +538,16 @@ func main() {
db,
)
failOnError(err, "failed to create Versions handlers")
osImageResolver := versions.NewOsImageResolver(
log.WithField("pkg", "versions"),
releaseHandler,
versionHandler,
osImages,
Options.ReleaseImageMirror,
)
domainHandler := domains.NewHandler(Options.BMConfig.BaseDNSDomains)
staticNetworkConfig := staticnetworkconfig.New(log.WithField("pkg", "static_network_config"), Options.StaticNetworkConfig)
ignitionBuilder, err := ignition.NewBuilder(log.WithField("pkg", "ignition"), staticNetworkConfig, mirrorRegistriesBuilder, releaseHandler, versionHandler)
ignitionBuilder, err := ignition.NewBuilder(log.WithField("pkg", "ignition"), staticNetworkConfig, mirrorRegistriesBuilder, releaseHandler, versionHandler, osImages)
failOnError(err, "failed to create ignition builder")
installConfigBuilder := installcfg.NewInstallConfigBuilder(log.WithField("pkg", "installcfg"), mirrorRegistriesBuilder, providerRegistry)

Expand All @@ -553,11 +562,11 @@ func main() {

manifestsApi := manifests.NewManifestsAPI(db, log.WithField("pkg", "manifests"), objectHandler, usageManager)
operatorsManager := operators.NewManager(log, manifestsApi, Options.OperatorsConfig, objectHandler)
hwValidator := hardware.NewValidator(log.WithField("pkg", "validators"), Options.HWValidatorConfig, operatorsManager, providerRegistry)
hwValidator := hardware.NewValidator(log.WithField("pkg", "validators"), Options.HWValidatorConfig, operatorsManager, providerRegistry, osImages)
connectivityValidator := connectivity.NewValidator(log.WithField("pkg", "validators"))
Options.InstructionConfig.HostFSMountDir = hostFSMountDir
instructionApi := hostcommands.NewInstructionManager(log.WithField("pkg", "instructions"), db, hwValidator,
releaseHandler, Options.InstructionConfig, connectivityValidator, eventsHandler, versionHandler, osImages, Options.EnableKubeAPI)
releaseHandler, Options.InstructionConfig, connectivityValidator, eventsHandler, versionHandler, osImageResolver, Options.EnableKubeAPI)

publicRegistries := map[string]bool{}
validations.ParsePublicRegistries(publicRegistries, Options.ValidationsConfig.PublicRegistries)
Expand Down Expand Up @@ -735,7 +744,7 @@ func main() {

bm := bminventory.NewBareMetalInventory(db, notificationStream, log.WithField("pkg", "Inventory"), hostApi, clusterApi, infraEnvApi, Options.BMConfig,
generator, eventsHandler, objectHandler, metricsManager, usageManager, operatorsManager, authHandler, authzHandler, ocpClient, ocmClient,
lead, pullSecretValidator, versionHandler, osImages, crdUtils, ignitionBuilder, hwValidator, dnsApi, installConfigBuilder, staticNetworkConfig,
lead, pullSecretValidator, versionHandler, osImages, osImageResolver, crdUtils, ignitionBuilder, hwValidator, dnsApi, installConfigBuilder, staticNetworkConfig,
Options.GCConfig, providerRegistry, generateInsecureIPXEURLs, Options.GeneratorConfig.InstallInvoker, disconnectedIgnitionGenerator)
events := events.NewApi(eventsHandler, logrus.WithField("pkg", "eventsApi"))

Expand Down Expand Up @@ -795,7 +804,7 @@ func main() {
go startPPROF(log)
}

go startKubeAPIControllers(ctrlMgr, log, bm, crdEventsHandler, osImages, versionHandler, releaseHandler, clusterApi, hostApi, manifestsApi, generateInsecureIPXEURLs, sys)
go startKubeAPIControllers(ctrlMgr, log, bm, crdEventsHandler, osImages, osImageResolver, versionHandler, releaseHandler, clusterApi, hostApi, manifestsApi, generateInsecureIPXEURLs, sys)

// Interrupt servers on SIGINT/SIGTERM
stop := make(chan os.Signal, 1)
Expand Down
2 changes: 1 addition & 1 deletion config/crd/bases/agent-install.openshift.io_infraenvs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ spec:
description: |-
OSImageVersion is the version of OS image to use when generating the InfraEnv.
The version should refer to an OSImage specified in the AgentServiceConfig
(i.e. OSImageVersion should equal to an OpenshiftVersion in OSImages list).
It needs to be equal to either an OSImage's Version or OpenshiftVersion.
Note: OSImageVersion can't be specified along with ClusterRef while creating an InfraEnv.
type: string
proxy:
Expand Down
2 changes: 1 addition & 1 deletion config/crd/resources.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3419,7 +3419,7 @@ spec:
description: |-
OSImageVersion is the version of OS image to use when generating the InfraEnv.
The version should refer to an OSImage specified in the AgentServiceConfig
(i.e. OSImageVersion should equal to an OpenshiftVersion in OSImages list).
It needs to be equal to either an OSImage's Version or OpenshiftVersion.
Note: OSImageVersion can't be specified along with ClusterRef while creating an InfraEnv.
type: string
proxy:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ spec:
description: |-
OSImageVersion is the version of OS image to use when generating the InfraEnv.
The version should refer to an OSImage specified in the AgentServiceConfig
(i.e. OSImageVersion should equal to an OpenshiftVersion in OSImages list).
It needs to be equal to either an OSImage's Version or OpenshiftVersion.
Note: OSImageVersion can't be specified along with ClusterRef while creating an InfraEnv.
type: string
proxy:
Expand Down
92 changes: 50 additions & 42 deletions internal/bminventory/inventory.go
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ type bareMetalInventory struct {
secretValidator validations.PullSecretValidator
versionsHandler versions.Handler
osImages versions.OSImages
osImageResolver versions.OsImageResolver
crdUtils CRDUtils
IgnitionBuilder ignition.IgnitionBuilder
hwValidator hardware.Validator
Expand Down Expand Up @@ -256,6 +257,7 @@ func NewBareMetalInventory(
pullSecretValidator validations.PullSecretValidator,
versionsHandler versions.Handler,
osImages versions.OSImages,
osImageResolver versions.OsImageResolver,
crdUtils CRDUtils,
IgnitionBuilder ignition.IgnitionBuilder,
hwValidator hardware.Validator,
Expand Down Expand Up @@ -291,6 +293,7 @@ func NewBareMetalInventory(
secretValidator: pullSecretValidator,
versionsHandler: versionsHandler,
osImages: osImages,
osImageResolver: osImageResolver,
crdUtils: crdUtils,
IgnitionBuilder: IgnitionBuilder,
hwValidator: hwValidator,
Expand Down Expand Up @@ -602,7 +605,7 @@ func (b *bareMetalInventory) getNewClusterReleaseImage(ctx context.Context, para
releaseVersion := *releaseImage.OpenshiftVersion
releaseArch := releaseImage.CPUArchitectures[0]
var osImage *models.OsImage
osImage, err = b.osImages.GetOsImage(releaseVersion, releaseArch)
osImage, err = b.osImageResolver.GetOsImageForRelease(releaseImage, releaseArch, swag.StringValue(params.PullSecret))
if err != nil || osImage.URL == nil {
return nil, errors.Errorf("No OS images are available for version %s and architecture %s", releaseVersion, releaseArch)
}
Expand Down Expand Up @@ -1287,16 +1290,14 @@ func (b *bareMetalInventory) updateExternalImageInfo(ctx context.Context, infraE
updates["type"] = imageType
infraEnv.Type = common.ImageTypePtr(imageType)

osImage, err := b.osImages.GetOsImageOrLatest(infraEnv.OpenshiftVersion, infraEnv.CPUArchitecture)
osImage, err := b.osImageResolver.GetOsImageForInfraEnv(ctx, infraEnv)
if err != nil {
return common.NewApiError(http.StatusBadRequest, err)
}

var version string
if osImage.OpenshiftVersion != nil {
version = *osImage.OpenshiftVersion
} else {
return errors.Errorf("OS image entry '%+v' missing OpenshiftVersion field", osImage)
version, err := versions.OsImageVersion(osImage)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the goal here is to make OpenshiftVersion optional, right?

if err != nil {
return err
}

var arch string
Expand Down Expand Up @@ -4316,7 +4317,7 @@ func (b *bareMetalInventory) DownloadMinimalInitrd(ctx context.Context, params i
var scriptContent, serviceContent string
if infraEnv.StaticNetworkConfig != "" {
var shouldUseNmstateService bool
shouldUseNmstateService, err = b.staticNetworkConfig.ShouldUseNmstateService(infraEnv.OpenshiftVersion)
shouldUseNmstateService, err = b.staticNetworkConfig.ShouldUseNmstateService(b.osImages.GetOpenshiftVersionForInfraEnv(infraEnv))
if err != nil {
return common.GenerateErrorResponder(err)
}
Expand Down Expand Up @@ -4930,27 +4931,14 @@ func validateProxySettings(httpProxy, httpsProxy, noProxy, ocpVersion *string) e
return nil
}

// validateClusterArchitectureAndVersion validates if architecture specified inside Infraenv matches one
// specified for the cluster. For single-arch clusters the validation needs to only compare values
// of the params. For multiarch cluster we want to see if the multiarch release image contains the
// the architecture specifically requested by the InfraEnv. We don't need to explicitly validate if
// the OS image exists because if not, this will be detected by the function generating the ISO.
func validateClusterArchitectureAndVersion(v versions.Handler, c *common.Cluster, cpuArch, ocpVersion string) error {
// For late-binding we don't know the cluster yet
// validateInfraEnvArchitectureMatchesCluster validates if architecture specified inside Infraenv matches one
// specified for the cluster.
func validateInfraEnvArchitectureMatchesCluster(c *common.Cluster, cpuArch string) error {
if c == nil {
return nil
}
if ocpVersion == "" {
ocpVersion = c.OpenshiftVersion
}
if c.CPUArchitecture != common.MultiCPUArchitecture {
if c.CPUArchitecture != "" && c.CPUArchitecture != cpuArch {
return errors.Errorf("Specified CPU architecture (%s) doesn't match the cluster (%s)", cpuArch, c.CPUArchitecture)
}
} else {
if err := v.ValidateReleaseImageForRHCOS(ocpVersion, cpuArch); err != nil {
return err
}
if c.CPUArchitecture != common.MultiCPUArchitecture && c.CPUArchitecture != "" && c.CPUArchitecture != cpuArch {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we still need to normalize these somewhere? IIRC there was some headache around aarch vs arm and x86 vs amd64?

return errors.Errorf("Specified CPU architecture (%s) doesn't match the cluster (%s)", cpuArch, c.CPUArchitecture)
}

return nil
Expand Down Expand Up @@ -5228,15 +5216,11 @@ func (b *bareMetalInventory) RegisterInfraEnvInternal(ctx context.Context, kubeK
}

openshiftVersion := params.InfraenvCreateParams.OpenshiftVersion

if b.EnableImageService {
var osImage *models.OsImage
osImage, err = b.osImages.GetOsImageOrLatest(params.InfraenvCreateParams.OpenshiftVersion, params.InfraenvCreateParams.CPUArchitecture)
if err != nil {
return common.NewApiError(http.StatusBadRequest, err)
}
openshiftVersion = *osImage.OpenshiftVersion
openshiftVersion, err = b.getOsImageVersion(ctx, openshiftVersion, params.InfraenvCreateParams.CPUArchitecture, swag.StringValue(params.InfraenvCreateParams.PullSecret))
if err != nil {
return err
}
Comment on lines 5218 to 5222

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Pull secret is used for external OS-image resolution before it's validated.

At line 5219 pullSecret is derived from user input and passed to b.osImageResolver.GetOsImageForVersion(...) at line 5224, but b.ValidatePullSecret(...) doesn't run until line 5317 — after the unvalidated secret has already been used to authenticate an external registry/release-image lookup. Previously OSImages lookups never took a pull secret at all, so this is a new trust-boundary gap introduced by this PR: an unvalidated/malformed pull secret is used before the validation gate that's supposed to guard it.

Consider moving ValidatePullSecret ahead of the OS image resolution call.

🔒️ Suggested reordering
 openshiftVersion := params.InfraenvCreateParams.OpenshiftVersion
 pullSecret := swag.StringValue(params.InfraenvCreateParams.PullSecret)
+mirroredRegistries := extractMirroredRegistriesFromConfig(log, mirrorRegistryConfiguration)
+if err = b.ValidatePullSecret(mirroredRegistries, pullSecret, ocm.UserNameFromContext(ctx), ""); err != nil {
+	err = errors.Wrap(secretValidationToUserError(err), "pull secret for new infraEnv is invalid")
+	return common.NewApiError(http.StatusBadRequest, err)
+}

 if b.EnableImageService {
 	var osImage *models.OsImage
 	if openshiftVersion != "" {
 		osImage, err = b.osImageResolver.GetOsImageForVersion(ctx, openshiftVersion, params.InfraenvCreateParams.CPUArchitecture, pullSecret)
 		...

(remove the now-duplicate ValidatePullSecret call further down)

As per path instructions, **/*.{py,js,ts,go,rs,java,rb,php,kt,swift,cs} requires "Validate at trust boundaries with allow-lists, not deny-lists."

Also applies to: 5316-5321

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/bminventory/inventory.go` around lines 5218 - 5238, Move the
existing b.ValidatePullSecret validation ahead of the
b.osImageResolver.GetOsImageForVersion call in the infraenv creation flow,
validating pullSecret before any external OS-image resolution uses it. Remove
the later duplicate validation near the subsequent processing, while preserving
the existing validation behavior and error handling.

Source: Path instructions


if kubeKey == nil {
kubeKey = &types.NamespacedName{}
}
Expand Down Expand Up @@ -5380,10 +5364,31 @@ func (b *bareMetalInventory) RegisterInfraEnvInternal(ctx context.Context, kubeK
return b.GetInfraEnvInternal(ctx, installer.GetInfraEnvParams{InfraEnvID: *infraEnv.ID})
}

func (b *bareMetalInventory) getOsImageVersion(ctx context.Context, openshiftVersion, cpuArchitecture, pullSecret string) (string, error) {
if !b.EnableImageService {
return openshiftVersion, nil

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this still do something other than pass the user's value back to them? Previously we were checking which versions were actually configured in the service. What effect will this have it it's wrong?

}

var osImage *models.OsImage
var err error
if openshiftVersion != "" {
osImage, err = b.osImageResolver.GetOsImageForVersion(ctx, openshiftVersion, cpuArchitecture, pullSecret)
if err != nil {
return "", common.NewApiError(http.StatusBadRequest, err)
}
} else {
osImage, err = b.osImages.GetLatestOsImage(cpuArchitecture)
if err != nil {
return "", common.NewApiError(http.StatusBadRequest, err)
}
}
return versions.OsImageVersion(osImage)
}

func (b *bareMetalInventory) validateInfraEnvCreateParams(ctx context.Context, params installer.RegisterInfraEnvParams, cluster *common.Cluster) error {
var err error

if err = validateClusterArchitectureAndVersion(b.versionsHandler, cluster, params.InfraenvCreateParams.CPUArchitecture, params.InfraenvCreateParams.OpenshiftVersion); err != nil {
if err = validateInfraEnvArchitectureMatchesCluster(cluster, params.InfraenvCreateParams.CPUArchitecture); err != nil {
return err
}

Expand Down Expand Up @@ -5657,14 +5662,17 @@ func (b *bareMetalInventory) UpdateInfraEnvInternal(ctx context.Context, params
openshiftVersion = *params.InfraEnvUpdateParams.OpenshiftVersion
}

if b.EnableImageService {
_, err = b.osImages.GetOsImageOrLatest(openshiftVersion, infraEnv.CPUArchitecture)
if err != nil {
return common.NewApiError(http.StatusBadRequest, err)
}
pullSecret := params.InfraEnvUpdateParams.PullSecret
if pullSecret == "" {
pullSecret = infraEnv.PullSecret
}

_, err = b.getOsImageVersion(ctx, openshiftVersion, infraEnv.CPUArchitecture, pullSecret)
if err != nil {
return err
}

if err = validateClusterArchitectureAndVersion(b.versionsHandler, cluster, infraEnv.CPUArchitecture, openshiftVersion); err != nil {
if err = validateInfraEnvArchitectureMatchesCluster(cluster, infraEnv.CPUArchitecture); err != nil {
return err
}
if err = featuresupport.ValidateIncompatibleFeatures(log, infraEnv.CPUArchitecture, cluster, &infraEnv.InfraEnv, params.InfraEnvUpdateParams); err != nil {
Expand Down Expand Up @@ -6727,7 +6735,7 @@ func (b *bareMetalInventory) V2DownloadInfraEnvFiles(ctx context.Context, params
var netFiles []staticnetworkconfig.StaticNetworkConfigData
if infraEnv.StaticNetworkConfig != "" {
var shouldUseNmstateService bool
shouldUseNmstateService, err = b.staticNetworkConfig.ShouldUseNmstateService(infraEnv.OpenshiftVersion)
shouldUseNmstateService, err = b.staticNetworkConfig.ShouldUseNmstateService(b.osImages.GetOpenshiftVersionForInfraEnv(infraEnv))
if err != nil {
return common.GenerateErrorResponder(err)
}
Expand Down
Loading