Skip to content

refactor(azure): Extract SSH key cert handling to new certs module#6802

Open
peytonr18 wants to merge 13 commits intocanonical:mainfrom
peytonr18:probertson-extract-ssh-key-cert
Open

refactor(azure): Extract SSH key cert handling to new certs module#6802
peytonr18 wants to merge 13 commits intocanonical:mainfrom
peytonr18:probertson-extract-ssh-key-cert

Conversation

@peytonr18
Copy link
Copy Markdown
Contributor

@peytonr18 peytonr18 commented Mar 23, 2026

Proposed Commit Message

refactor(azure): Extract SSH key cert handling to new certs module

Refactor x509 certificate and SSH key handling logic from 
OpenSSLManager and DataSourceAzure into a new dedicated 
module cloudinit/sources/azure/certs.py with three utility functions:

  - is_openssh_formatted(): Validates OpenSSH format keys
  - is_x509_certificate(): Validates x509 certificates via opensslprobert
  - convert_x509_to_openssh(): Converts x509 certs to OpenSSH keys

This refactoring maintains existing functionality without changes while 
preparing the codebase for upcoming work to support x509 certificates 
from Azure IMDS.

Additional Context

Purpose:
Refactors x509 certificate and SSH key handling logic from OpenSSLManager and DataSourceAzure into a dedicated module (cloudinit/sources/azure/certs.py).

Changes:

  • New module: cloudinit/sources/azure/certs.py with three functions:
  • is_openssh_formatted() - Validates OpenSSH format keys
  • is_x509_certificate() - Validates x509 certificates using openssl
  • convert_x509_to_openssh() - Converts x509 certs to OpenSSH keys
  • Updated: cloudinit/sources/helpers/azure.py --> _get_ssh_key_from_cert() now uses new module
  • Updated: cloudinit/sources/DataSourceAzure.py --> _key_is_openssh_formatted() now uses new module

Test Coverage:

  • Unit tests for OpenSSH key validation and basic checks (various types, edge cases, markers, empty strings)
  • Integration tests with openssl for x509 validation & with openssl + ssh-keygen for cert conversion
  • Mocked tests for error handling (openssl/ssh-keygen failures)

Merge type

  • Squash merge using "Proposed Commit Message"

Replaces #6561

@peytonr18
refactor(azure): Extract SSH key cert handling to new certs module
37f876c
@peytonr18
Resolving subp errors: add integration tests instead of mocking compl…
1741d49 
…ete openssl and ssh-keygen functionality
@peytonr18
refactor(azure): Add certificate validation and improve logging verbo…
90eb57b 
…sity

Added extract_x509_certificate() to validate certificates in bundles, integrated validation into parse_certificates(), and toned down debug logs to avoid sounding like failures.
@peytonr18
azure: use shared x509 extractor for wireserver certs
e401a2e 
Fold the regex-based certificate parsing into cloudinit.sources.azure.certs so
helpers no longer reimplement it, and wire OpenSSLManager.parse_certificates to
loop over the helper. Added a regression test that confirms CRLF-mixed bundles
still yield every fingerprint + key pair.
@peytonr18
Merge remote-tracking branch 'origin/main' into probertson-extract-ss…
c1bcb63 
…h-key-cert
@peytonr18
Merge remote-tracking branch 'origin/main' into probertson-extract-ss…
4333b98 
…h-key-cert
@peytonr18
Merge remote-tracking branch 'origin/main' into probertson-extract-ss…
e8f70ae 
…h-key-cert
@peytonr18
test: mock generate_certificate in test_parse_certificates_processes_…
2ae19f0 
…all_certificates to avoid openssl dependency
@blackboxsw blackboxsw self-assigned this Mar 25, 2026
cjp256
cjp256 reviewed Mar 26, 2026
View reviewed changes
Comment thread cloudinit/sources/azure/certs.py Outdated
LOG.debug("Empty key content provided.")
return False
# See https://bugs.launchpad.net/cloud-init/+bug/1910835
if "\r\n" in key.strip():
Copy link
Copy Markdown
Contributor

@cjp256 cjp256 Mar 26, 2026

Choose a reason for hiding this comment

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

what happens with AuthKeyLineParser if \r\n is found in the middle of the key? does it still validate?

Copy link
Copy Markdown
Contributor Author

@peytonr18 peytonr18 Mar 31, 2026

Choose a reason for hiding this comment

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

Edit: I've updated in the latest commit: instead of rejecting keys with embedded \r\n, we now sanitize them by stripping the CRLF sequences before validation. This handles the Azure-generated key case (LP: #1910835) where \r\n gets injected into the base64 data.

The flow in _get_public_ssh_keys_from_imds() is now:

  1. Collect raw keys from IMDS
  2. Sanitize any keys containing \r\n via sanitize_openssh_key()
  3. Validate with is_openssh_formatted()
  4. Return clean, validated keys

For example, a key like ssh-rsa AAAA\r\nBBBB comment gets sanitized to ssh-rsa AAAABBBB comment, validated, and returned cleanly for authorized_keys.

cjp256
cjp256 reviewed Mar 26, 2026
View reviewed changes
Comment thread cloudinit/sources/azure/certs.py Outdated
cjp256
cjp256 reviewed Mar 26, 2026
View reviewed changes
Comment thread cloudinit/sources/helpers/azure.py Outdated
@peytonr18
Refactor Azure certificate parsing to extract all x509 certs at once …
bfa3e7c 
…and iterate

them directly in parse_certificates, replacing the previous loop/find slicing flow.

Also clarify is_openssh_formatted behavior by explicitly rejecting embedded CRLF
and improving debug messages; update tests and
type hints to match the new extraction API.
bpryan99
bpryan99 approved these changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown

@bpryan99 bpryan99 left a comment

Choose a reason for hiding this comment

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

LGTM! Good test coverage.

@peytonr18
fix(azure): Sanitize SSH keys with embedded CRLF instead of rejecting
b481306 
Azure-generated SSH keys may contain \r\n sequences embedded in the
base64 key data (LP: #1910835). Previously, keys with embedded CRLF
were rejected outright by is_openssh_formatted(). Instead, sanitize
the keys by stripping \r\n before validation so they can be properly
written to authorized_keys.
@peytonr18 peytonr18 force-pushed the probertson-extract-ssh-key-cert branch from b09ed45 to b481306 Compare March 31, 2026 19:21
cjp256
cjp256 reviewed Apr 2, 2026
View reviewed changes
Comment thread cloudinit/sources/azure/certs.py
cjp256
cjp256 reviewed Apr 2, 2026
View reviewed changes
Comment thread cloudinit/sources/azure/certs.py Outdated
cjp256
cjp256 reviewed Apr 2, 2026
View reviewed changes
Comment thread cloudinit/sources/azure/certs.py
cjp256
cjp256 reviewed Apr 2, 2026
View reviewed changes
Comment thread cloudinit/sources/DataSourceAzure.py Outdated
cjp256
cjp256 reviewed Apr 2, 2026
View reviewed changes
Comment thread cloudinit/sources/DataSourceAzure.py Outdated
cjp256
cjp256 reviewed Apr 2, 2026
View reviewed changes
Comment thread tests/unittests/sources/azure/test_certs.py Outdated
cjp256
cjp256 reviewed Apr 2, 2026
View reviewed changes
Comment thread tests/unittests/sources/test_azure_helper.py Outdated
cjp256
cjp256 reviewed Apr 2, 2026
View reviewed changes
Comment thread cloudinit/sources/azure/certs.py Outdated
@peytonr18 peytonr18 force-pushed the probertson-extract-ssh-key-cert branch 2 times, most recently from f42d866 to 324e304 Compare April 2, 2026 21:22
@peytonr18
refactor(azure): Address PR review feedback for certs module
ff09ae0 
- Use pytest.mark.parametrize, fixtures, and monkeypatch in tests
- Remove raw docstring prefix; clarify CRLF deprecation in docstring
- Remove redundant _key_is_openssh_formatted wrapper
- Simplify sanitize_openssh_key call site and log messages
- Reuse variables and constants instead of inline string literals
@peytonr18 peytonr18 force-pushed the probertson-extract-ssh-key-cert branch from 324e304 to ff09ae0 Compare April 2, 2026 21:27
cjp256
cjp256 reviewed Apr 3, 2026
View reviewed changes
Comment thread cloudinit/sources/azure/certs.py
cjp256
cjp256 reviewed Apr 3, 2026
View reviewed changes
Comment thread tests/unittests/sources/azure/test_certs.py
cjp256
cjp256 reviewed Apr 3, 2026
View reviewed changes
Comment thread tests/unittests/sources/azure/test_certs.py Outdated
cjp256
cjp256 reviewed Apr 3, 2026
View reviewed changes
Comment thread tests/unittests/sources/azure/test_certs.py Outdated
cjp256
cjp256 reviewed Apr 3, 2026
View reviewed changes
Comment thread tests/unittests/sources/azure/test_certs.py Outdated
cjp256
cjp256 reviewed Apr 3, 2026
View reviewed changes
Comment thread tests/unittests/sources/azure/test_certs.py Outdated
@peytonr18 peytonr18 force-pushed the probertson-extract-ssh-key-cert branch from f827718 to 3808c99 Compare April 6, 2026 20:37
cjp256
cjp256 approved these changes Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@cjp256 cjp256 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Peyton!

@cjp256
Copy link
Copy Markdown
Contributor

cjp256 commented Apr 15, 2026

@blackboxsw this is ready for review when you get a chance, thanks!

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging blackboxsw, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag blackboxsw to reopen it.)

@github-actions github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Apr 30, 2026
@peytonr18
Copy link
Copy Markdown
Contributor Author

peytonr18 commented Apr 30, 2026

Hi @blackboxsw, this one is on track to be marked stale whenever you get a chance - thank you!

@github-actions github-actions Bot removed the stale-pr Pull request is stale; will be auto-closed soon label May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@cjp256 cjp256 cjp256 approved these changes

@anhvoms anhvoms anhvoms approved these changes

@bpryan99 bpryan99 bpryan99 approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@blackboxsw blackboxsw

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@peytonr18 @cjp256 @anhvoms @bpryan99 @blackboxsw