Skip to content

sia: include x-principals in the rsa ssh host certificate csr - #3448

Draft
havetisyan wants to merge 2 commits into
masterfrom
ssh-host-csr-xprincipals
Draft

sia: include x-principals in the rsa ssh host certificate csr#3448
havetisyan wants to merge 2 commits into
masterfrom
ssh-host-csr-xprincipals

Conversation

@havetisyan

@havetisyan havetisyan commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

The certsign-based CSR path (SshHostKeyType=rsa) only ever requested the ZTS cloud domain hostnames, so hosts using an RSA SSH host key could not get the hostname, the operator-configured SshPrincipals or the provider's additional principals into their host certificate the way the ECDSA SSHCertRequest path can.

Add GenerateSSHHostCSRWithXPrincipals, which keeps the cloud domain hostnames in the principals field (used to derive the key id) and puts the full requested set - hostname, SshPrincipals, provider principals and the private ip - in the new xprincipals field that ZTS validates against in InstanceCertManager.validPrincipals. Hoist the additional-provider- principals lookup in generateSshRequest out of the ECDSA branch so both key types share it, and switch the RSA branch to the new function.

GenerateSSHHostCSR is left in place for existing callers.

Description

Contribution Checklist:

  • The pull request does not introduce any breaking changes
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

Attach Screenshots (Optional)

The certsign-based CSR path (SshHostKeyType=rsa) only ever requested the
ZTS cloud domain hostnames, so hosts using an RSA SSH host key could not
get the hostname, the operator-configured SshPrincipals or the provider's
additional principals into their host certificate the way the ECDSA
SSHCertRequest path can.

Add GenerateSSHHostCSRWithXPrincipals, which keeps the cloud domain
hostnames in the principals field (used to derive the key id) and puts
the full requested set - hostname, SshPrincipals, provider principals and
the private ip - in the new xprincipals field that ZTS validates against
in InstanceCertManager.validPrincipals. Hoist the additional-provider-
principals lookup in generateSshRequest out of the ECDSA branch so both
key types share it, and switch the RSA branch to the new function.

GenerateSSHHostCSR is left in place for existing callers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Henry Avetisyan <hga@yahooinc.com>
Copilot AI lite review requested due to automatic review settings August 10, 2026 18:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Go SIA (Service Identity Agent) SSH host certificate CSR generation so the RSA CSR path can request the full intended set of SSH principals (hostname, configured SshPrincipals, provider additional principals, and private IP) using a new xprincipals field, aligning it with the richer ECDSA SSHCertRequest behavior and ZTS validation expectations.

Changes:

  • Add XPrincipals support to the CSR JSON model and introduce GenerateSSHHostCSRWithXPrincipals for RSA SSH host key CSRs.
  • Refactor generateSshRequest so provider additional principals are applied for both RSA and ECDSA branches.
  • Add/extend unit tests covering both the legacy CSR behavior and the new x-principals CSR behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
libs/go/sia/util/util.go Adds xprincipals to SSH CSR model and implements new RSA CSR generator with XPrincipals payload.
libs/go/sia/util/util_test.go Adds unit tests validating legacy CSR output and new x-principals CSR output.
libs/go/sia/agent/agent.go Refactors SSH request generation to include provider principals for RSA and switches RSA to new CSR generator.
libs/go/sia/agent/agent_test.go Extends tests to validate RSA CSR includes principals vs x-principals as expected.
Suppressed comments (1)

libs/go/sia/util/util.go:443

  • sshPrincipals is split on commas without trimming/empty filtering. Values like "host1, host2" or a trailing comma will generate principals with leading spaces / empty strings, which can cause ZTS principal validation failures. Trim whitespace and skip empty entries when populating xprincipals.
	if sshPrincipals != "" {
		xprincipals = append(xprincipals, strings.Split(sshPrincipals, ",")...)
	}

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread libs/go/sia/util/util.go Outdated
Comment thread libs/go/sia/util/util.go Outdated
Requesting x-principals changes what ZTS validates - the field is what
InstanceCertManager.validPrincipals checks the hostname against, and an
absent field means pass-through - so make the new behavior opt-in via
Options.SshIncludePrincipals rather than changing every rsa host key
deployment at once. When the flag is not set the rsa path keeps calling
GenerateSSHHostCSR and produces exactly the payload it did before.

Mark xprincipals as omitempty/optional so the legacy CSR stays
byte-identical and so an empty list is never sent, since the server
validates against an empty list instead of skipping the check. Drop the
unused instanceId argument from GenerateSSHHostCSRWithXPrincipals -
SSHKeyReq has no instance id field to carry it.

Tests cover both sides of the flag in generateSshRequest, and pin the
presence and absence of the xprincipals field in the marshalled CSR,
including the no-cloud-domains case where it must be omitted rather
than emitted as an empty list.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Henry Avetisyan <hga@yahooinc.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (4)

libs/go/sia/util/util_test.go:1505

  • This comment says an empty xprincipals list would trigger hostname validation. In ZTS validateSSHHostnames, an empty list leads to cnames.isEmpty() and returns true (no validation performed). Consider rewording to explain the omission here is simply because there are no values to send and the field is omitempty.
	// finally with no optional arguments and no zts cloud domains there are no
	// x-principals to report, and the field must be omitted rather than sent as
	// an empty list which the server would validate the hostname against

libs/go/sia/agent/agent.go:568

  • GetAdditionalSshHostPrincipals is now called unconditionally whenever SSH is enabled, even for the RSA legacy CSR path where the value is unused (SshIncludePrincipals is false). Since provider implementations may hit instance metadata, this introduces unnecessary network/latency and work on the common RSA legacy path.
		sshPrincipals := opts.SshPrincipals
		// additional ssh host principals are added on best effort basis, hence error below is ignored.
		additionalSshHostPrincipals, _ := opts.Provider.GetAdditionalSshHostPrincipals(opts.MetaEndPoint)
		if additionalSshHostPrincipals != "" {
			if sshPrincipals != "" {

libs/go/sia/config/config.go:218

  • SshIncludePrincipals was added to the runtime Options struct, but it is not present in the JSON Config struct and is not populated from env/config in the options loaders. As a result the feature cannot be enabled by users in production (it will always remain the default false unless set programmatically/tests).
	HostnameSuffix         string              //hostname suffix in case we need to auto-generate hostname
	SshPrincipals          string              //ssh additional principals
	SshIncludePrincipals   bool                //optional flag to include additional principals in host rsa key certs

libs/go/sia/util/util_test.go:1421

  • These test comments claim that an empty xprincipals list is treated differently from an absent field and that ZTS would validate the hostname against an empty list. In ZTS InstanceCertManager.validPrincipals / validateSSHHostnames, an empty list results in no hostname/cname validation and returns true; the only special case is when xPrincipals is null (missing field), which short-circuits validation and logs an error. Updating the comments will avoid misleading future readers.

This issue also appears on line 1503 of the same file.

	// the legacy csr only includes the zts cloud domain based principals and
	// must not carry an x-principals field at all - an empty x-principals list
	// is not the same as an absent one since the server only skips hostname
	// validation when the field is not present

@havetisyan
havetisyan marked this pull request as draft August 13, 2026 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants