[RFC]: Azure: Proposal to Integrate User Creation in Ignition via Ignition Config #2185
Conversation
Via the commands: go get github.com/GehirnInc/crypt go mod vendor
Add the ability to synthesize Ignition configurations dynamically from Azure Instance Metadata Service (IMDS) and OVF provisioning data.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable feature for Azure provisioning by allowing Ignition to generate its configuration directly from cloud metadata. The overall approach is well-structured, and the inclusion of comprehensive tests is excellent. I have a few suggestions to enhance robustness, improve maintainability, and address a critical issue in the password hashing implementation. My feedback also touches on a key design decision presented in the RFC regarding flag handling.
| func HashPassword(password string) (string, error) { | ||
| salt, err := generateSalt(16) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| crypt := sha512_crypt.New() | ||
| return crypt.Generate([]byte(password), []byte("$6$"+salt)) | ||
| } |
There was a problem hiding this comment.
The HashPassword function unnecessarily calls a custom generateSalt helper. The sha512_crypt library can correctly generate a random salt if you pass nil as the salt argument to its Generate method. This avoids potential bugs from a custom implementation and simplifies the code.
func HashPassword(password string) (string, error) {
crypt := sha512_crypt.New()
return crypt.Generate([]byte(password), nil)
}| // generateSalt generates a random salt of the specified length. | ||
| func generateSalt(length int) (string, error) { | ||
| bytes := make([]byte, length) | ||
| if _, err := rand.Read(bytes); err != nil { | ||
| return "", err | ||
| } | ||
| // Use base64 encoding and trim to desired length | ||
| salt := base64.StdEncoding.EncodeToString(bytes) | ||
| // Remove any characters that might cause issues in crypt salt | ||
| salt = strings.ReplaceAll(salt, "+", ".") | ||
| salt = strings.ReplaceAll(salt, "=", "") | ||
| if len(salt) > length { | ||
| salt = salt[:length] | ||
| } | ||
| return salt, nil | ||
| } |
There was a problem hiding this comment.
This generateSalt function should be removed. It re-implements functionality already present in the sha512_crypt library and is subtly buggy as it doesn't produce a salt fully conforming to the crypt(3) alphabet. The sha512_crypt library's Generate method handles random salt generation correctly when passed a nil salt, making this helper unnecessary.
| // Never allow cloud config generation during fetch-offline stage (no networking) | ||
| if flags.stage == "fetch" && flags.platform == "azure" { | ||
| flags.generateCloudConfig = true | ||
| } |
There was a problem hiding this comment.
The comment on line 76 is misleading. It states "Never allow cloud config generation during fetch-offline stage", but the code block enables generateCloudConfig for the fetch stage on Azure. This can cause confusion for future developers. The comment should be updated to accurately reflect that this block auto-enables the feature for the Azure platform.
| // Never allow cloud config generation during fetch-offline stage (no networking) | |
| if flags.stage == "fetch" && flags.platform == "azure" { | |
| flags.generateCloudConfig = true | |
| } | |
| // Auto-enable cloud config generation for Azure during the fetch stage. | |
| // This is part of the "Option B" approach described in the RFC. | |
| if flags.stage == "fetch" && flags.platform == "azure" { | |
| flags.generateCloudConfig = true | |
| } |
| RemainAfterExit=yes | ||
| EnvironmentFile=/run/ignition.env | ||
| ExecStart=/usr/bin/ignition --root=/sysroot --platform=${PLATFORM_ID} --stage=fetch ${IGNITION_ARGS} | ||
| ExecStart=/usr/bin/ignition --root=/sysroot --platform=${PLATFORM_ID} --stage=fetch --generate-cloud-config=true ${IGNITION_ARGS} |
There was a problem hiding this comment.
This change hardcodes --generate-cloud-config=true for the fetch stage. As noted in the RFC description, this is "Option B" which auto-enables the feature. While this simplifies the default case, it removes the flexibility for an Azure user to provide a standard Ignition config via user data if they wished. Have you considered making this configurable via a kernel argument, allowing users to opt-out of this new behavior if needed? This would align more with "Option A" and provide greater control.
| func (e *Engine) fetchGeneratedConfig() (types.Config, error) { | ||
| e.Logger.Info("using generated cloud config for platform %q", e.PlatformConfig.Name()) | ||
| cfg, err := e.PlatformConfig.GenerateConfig(e.Fetcher) | ||
| if err != nil { | ||
| return types.Config{}, err | ||
| } | ||
|
|
||
| e.State.FetchedConfigs = append(e.State.FetchedConfigs, state.FetchedConfig{ | ||
| Kind: "user", | ||
| Source: fmt.Sprintf("%s-generator", e.PlatformConfig.Name()), | ||
| Referenced: false, | ||
| }) | ||
|
|
||
| configFetcher := ConfigFetcher{ | ||
| Logger: e.Logger, | ||
| Fetcher: e.Fetcher, | ||
| State: e.State, | ||
| } | ||
|
|
||
| return configFetcher.RenderConfig(cfg) | ||
| } |
There was a problem hiding this comment.
This new fetchGeneratedConfig function is missing a call to e.Fetcher.UpdateHttpTimeoutsAndCAs before configFetcher.RenderConfig is called. The original fetchProviderConfig flow includes this step to ensure that any timeouts or CAs specified in the just-fetched config are used for subsequent resource fetches (e.g., remote files referenced in the config). Without this, Ignition will use the default timeouts, which might not be what the user intended.
Additionally, there is significant code duplication between fetchGeneratedConfig and the end of fetchProviderConfig. Consider refactoring to have a single ConfigFetcher creation and RenderConfig call to improve maintainability.
func (e *Engine) fetchGeneratedConfig() (types.Config, error) {
e.Logger.Info("using generated cloud config for platform %q", e.PlatformConfig.Name())
cfg, err := e.PlatformConfig.GenerateConfig(e.Fetcher)
if err != nil {
return types.Config{}, err
}
e.State.FetchedConfigs = append(e.State.FetchedConfigs, state.FetchedConfig{
Kind: "user",
Source: fmt.Sprintf("%s-generator", e.PlatformConfig.Name()),
Referenced: false,
})
// Update the http client to use the timeouts and CAs from the newly fetched
// config, before rendering and fetching remote resources.
if err := e.Fetcher.UpdateHttpTimeoutsAndCAs(cfg.Ignition.Timeouts, cfg.Ignition.Security.TLS.CertificateAuthorities, cfg.Ignition.Proxy); err != nil {
return types.Config{}, err
}
configFetcher := ConfigFetcher{
Logger: e.Logger,
Fetcher: e.Fetcher,
State: e.State,
}
return configFetcher.RenderConfig(cfg)
}| switch { | ||
| case metaErr != nil: | ||
| return types.Config{}, metaErr | ||
| case provisioningErr != nil: | ||
| return types.Config{}, provisioningErr | ||
| default: | ||
| return types.Config{}, fmt.Errorf("azure: no instance metadata or provisioning data available") | ||
| } |
There was a problem hiding this comment.
In the case where both IMDS and OVF metadata fetching fail, this logic correctly returns an error. However, it only returns the metaErr, even if provisioningErr also occurred. To provide better diagnostics to the user, it would be more helpful to return both errors. You can use errors.Join (available in Go 1.20+) to combine them.
var allErrors []error
if metaErr != nil {
allErrors = append(allErrors, metaErr)
}
if provisioningErr != nil {
allErrors = append(allErrors, provisioningErr)
}
if len(allErrors) > 0 {
return types.Config{}, errors.Join(allErrors...)
}
return types.Config{}, fmt.Errorf("azure: no instance metadata or provisioning data available")| user.PasswordHash = cfgutil.StrToPtr(passwordHash) | ||
| } | ||
|
|
||
| sudoersFile := newDataFile("/etc/sudoers.d/50-azure-cloud-config", 0440, "%wheel ALL=(ALL) NOPASSWD: ALL\n") |
There was a problem hiding this comment.
There's an inconsistency in the naming of the generated sudoers file. The code creates /etc/sudoers.d/50-azure-cloud-config, but the PR description and demo output refer to it as /etc/sudoers.d/azure-cloud-config. To avoid confusion for users and maintainers, it would be best to make the code and documentation consistent. The 50- prefix is good practice for ordering, so I'd recommend updating the documentation to match the code.
|
Let's start with a discussion about the design and the goals first before we look at code. Can you open an issue with your goals and what this would achieve (i.e. most of this PR text, but in an issue)? Thanks |
2 participants
Summary
The PR is part of a larger effort to modernize Azure provisioning for Flatcar by moving away from the WALinuxAgent provisioning agent (PA) and leveraging Ignition as the execution engine for early-boot configuration.
The idea is to emit an Ignition config from Azure metadata and let Ignition apply it. This leverages Ignition’s existing capabilities for:
This RFC seeks feedback on the proposed approach.
Overview
The
--generate-cloud-configflag enables Ignition to dynamically synthesize an Ignition configuration from cloud provider metadata at boot time, rather than requiring a pre-baked Ignition config in user data.Architecture
Flag Integration
There are two potential approaches for enabling cloud config generation:
Option A: Explicit Flag
The flag would be passed via the systemd service or kernel command line, allowing operators to explicitly opt-in or opt-out of cloud config generation.
Option B: Auto-Enable by Platform (Current Implementation)
This approach automatically enables generation for Azure during the fetch stage without requiring explicit flag passing.
The current implementation uses Option B due to challenges passing the flag through the boot process. Feedback is welcome on which approach is preferred - explicit flag control offers more flexibility, while auto-enable simplifies configuration for supported platforms.
The flag flows through the execution engine:
internal/main.go): Determines whetherGenerateCloudConfigis enabled (via flag or auto-detection)GenerateCloudConfigbool is set on theexec.Enginestructinternal/exec/engine.go): DuringfetchProviderConfig(), ifGenerateCloudConfigis true, it callsfetchGeneratedConfig()instead of the normal fetch pathPlatformConfig.GenerateConfig()which invokes the platform-specific generatorAzure Provider Implementation
The Azure provider registers a
GenerateCloudConfigfunction:Data Sources
The generator collects metadata from two sources:
Azure Instance Metadata Service (IMDS) - HTTP endpoint at
169.254.169.254compute.osProfile.adminUsername)compute.publicKeys[].keyData)OVF Provisioning Data - XML from attached CD-ROM (
ovf-env.xml)LinuxProvisioningConfigurationSet.UserName)LinuxProvisioningConfigurationSet.UserPassword)LinuxProvisioningConfigurationSet.SSH.PublicKeys)DisableSshPasswordAuthentication)Execution Flow
Systemd Integration
The dracut module's
ignition-fetch.serviceis modified to pass the flag:Benefits
GenerateCloudConfigto support similar functionalityGenerated Config Example
{ "ignition": { "version": "3.6.0" }, "passwd": { "users": [{ "name": "azureuser", "groups": ["wheel"], "homeDir": "/home/azureuser", "shell": "/bin/bash", "passwordHash": "$6$...", "sshAuthorizedKeys": ["ssh-rsa AAAA...", "ssh-ed25519 AAAA..."] }] }, "storage": { "files": [ { "path": "/etc/sudoers.d/azure-cloud-config", "mode": 288, "contents": { "source": "data:,%25wheel%20ALL%3D(ALL)%20NOPASSWD%3A%20ALL%0A" } }, { "path": "/etc/ssh/sshd_config.d/50-azure-cloud-config.conf", "mode": 420, "contents": { "source": "data:,%23%20Custom%20SSHD%20settings%0APasswordAuthentication%20no%0APermitRootLogin%20no%0AAllowUsers%20azureuser%0A" } } ] } }Demo Output
The following logs demonstrate the cloud config generation in action on an Azure VM.
Boot Logs (journalctl)
Resulting Files on Disk
Sudoers config (
/etc/sudoers.d/azure-cloud-config):SSHD config (
/etc/ssh/sshd_config.d/50-azure-cloud-config.conf):