Skip to content

feat: support Vault as TLS automation cert issuer - #440

Open
clement0010 wants to merge 24 commits into
feat/ct/tls-static-dynamicfrom
feat/ct/tls-vault-ca
Open

feat: support Vault as TLS automation cert issuer#440
clement0010 wants to merge 24 commits into
feat/ct/tls-static-dynamicfrom
feat/ct/tls-vault-ca

Conversation

@clement0010

@clement0010 clement0010 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Related Tickets

Issue: #423

Changes

  • Support tls.automation.issuer.vault config. When configured, Gateway can issue a TLS cert via Vault through the /sign endpoint.
    • The issued TLS cert is verified to contain the exact requested alternative names and IP sans, TTL and the public key.
  • Rename the shared Vault auth config structs (SSHCAVault*Vault*) for reuse by both SSH and TLS configs; the YAML config schema is unchanged
  • Support the same Vault connection and auth options as the SSH CA integration: token, AppRole, GCP, and AWS auth, with VAULT_TOKEN fallback and automatic token renewal
  • Add an integration test to test the Vault PKI signing endpoint

@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.29%. Comparing base (d96a70f) to head (2c4e8b8).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
internal/vault/vault.go 90.90% 2 Missing ⚠️
internal/connect/cert/issuer.go 98.50% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                       @@
##           feat/ct/tls-static-dynamic     #440      +/-   ##
==============================================================
+ Coverage                       93.93%   94.29%   +0.36%     
==============================================================
  Files                              46       46              
  Lines                            2918     2999      +81     
==============================================================
+ Hits                             2741     2828      +87     
+ Misses                            176      170       -6     
  Partials                            1        1              
Flag Coverage Δ
integration 62.75% <61.66%> (+2.92%) ⬆️
unit 89.82% <91.66%> (+0.59%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
internal/config/config.go 95.65% <100.00%> (+0.57%) ⬆️
internal/sshhandler/ca.go 97.50% <100.00%> (ø)
internal/connect/cert/issuer.go 96.21% <98.50%> (+2.36%) ⬆️
internal/vault/vault.go 90.72% <90.90%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@clement0010
clement0010 force-pushed the feat/ct/tls-vault-ca branch from e1a31e6 to 152ba43 Compare August 10, 2026 14:28
@clement0010
clement0010 requested a balanced review from Copilot August 10, 2026 14:32
@clement0010
clement0010 marked this pull request as ready for review August 10, 2026 14:32

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

Adds Vault PKI support for dynamically issued downstream TLS certificates and reuses shared Vault authentication across TLS and SSH.

Changes:

  • Adds Vault-backed dynamic TLS issuance and configuration.
  • Refactors Vault authentication into a shared package.
  • Adds downstream TLS support for Web App resources and local tooling.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
.golangci.yml Allows the issuer interface name.
internal/config/config.go Adds Vault TLS CA configuration.
internal/config/config_test.go Tests shared Vault and TLS configuration.
internal/connect/cert.go Implements Vault PKI certificate issuance.
internal/connect/cert_provider.go Propagates handshake contexts.
internal/connect/cert_provider_test.go Tests Vault provider creation.
internal/connect/cert_reloader.go Adopts the context-aware interface.
internal/connect/cert_reloader_test.go Updates reloader tests.
internal/connect/cert_test.go Tests dynamic and Vault issuers.
internal/connect/conn.go Passes handshake context during issuance.
internal/sshhandler/ca.go Uses the shared Vault client.
internal/sshhandler/proxy_test.go Updates shared Vault test setup.
internal/token/gat_claims.go Adds Web App downstream TLS metadata.
internal/token/gat_claims_test.go Tests downstream TLS claims.
internal/vault/vault.go Provides shared Vault authentication and renewal.
internal/vault/vault_test.go Tests shared Vault behavior.
test/fake/client.go Supports TLS-enabled Web App tokens.
test/integration/ssh_test.go Migrates SSH Vault integration configuration.
tools/local/main.go Adds a local HTTPS Web App client.
Suppressed comments (1)

internal/connect/cert.go:282

  • This omits the requested host from alt_names and sends IP literals as DNS SANs. If the Vault role sets exclude_cn_from_sans, the primary hostname is not covered; IP resources also require ip_sans, so their handshakes fail hostname verification. Split all names by IP/hostname, include the primary hostname in alt_names, and send IPs through ip_sans as described in the PR.
	if len(names) > 1 {
		data["alt_names"] = strings.Join(names[1:], ",")
	}

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

Comment thread internal/connect/cert.go Outdated
Comment thread internal/connect/cert.go Outdated
Comment thread internal/connect/cert.go Outdated
@clement0010
clement0010 force-pushed the feat/ct/tls-static-dynamic branch 2 times, most recently from 98469da to b7273ef Compare August 11, 2026 12:25
@clement0010
clement0010 force-pushed the feat/ct/tls-vault-ca branch 3 times, most recently from 63c44a0 to b56499c Compare August 11, 2026 12:52
@clement0010
clement0010 requested a balanced review from Copilot August 12, 2026 07:39
@github-actions

Copy link
Copy Markdown

This pull request is stale because it has been open 5 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions Bot added the wontfix This will not be worked on label Aug 22, 2026
@clement0010 clement0010 removed the wontfix This will not be worked on label Aug 22, 2026
Comment thread internal/connect/cert.go Outdated
Comment thread internal/connect/cert.go Outdated
@clement0010
clement0010 force-pushed the feat/ct/tls-static-dynamic branch from 6e9d18b to 7486aad Compare September 3, 2026 02:51
clement0010 and others added 5 commits September 4, 2026 15:06
- add tls.automation.issuer.vault, issuing leaf certificates through Vault's
  PKI secrets engine alongside the existing local CA issuer
- share the Vault client, auth and token-renewal code between the TLS and
  SSH CAs in a new internal/vault package
- issue DNS SANs only; the no-SNI local-address fallback still serves probes,
  which skip verification

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Health probes dial the gateway by IP without SNI, so the local-address
fallback requests a certificate for a bare IP. Split IP aliases into
ip_sans when issuing through Vault so those certificates verify, matching
the self-sign issuer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@clement0010 clement0010 changed the title feat: support Vault as a TLS CA for dynamic downstream certificates feat: support Vault as TLS automation cert issuer Sep 4, 2026
Comment thread internal/config/config.go
Comment on lines -185 to -189
Address string `yaml:"address"` // Vault server address, e.g. https://vault.example.com:8200
CABundleFile string `yaml:"caBundleFile,omitempty"` // Path to a PEM CA bundle for verifying Vault's TLS certificate; omit to use the system trust store
Auth SSHCAVaultAuthConfig `yaml:"auth"`

Namespace string `yaml:"namespace,omitempty"` // Optional Vault namespace

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are moved to VaultConfig

ttl: "5m"
ca:
vault:
server: "https://vault:8200"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should be address typo from previous commit

Comment thread internal/connect/listener.go
Comment thread internal/connect/cert_issuer.go Outdated
@clement0010
clement0010 force-pushed the feat/ct/tls-static-dynamic branch 2 times, most recently from 2f39806 to af8c627 Compare September 7, 2026 12:17
clement0010 and others added 5 commits September 8, 2026 00:25
Port the Vault issuer onto the `sign` seam the base branch introduced:
`vaultIssuer` now implements `sign` and reuses the shared `issueCertificate`,
so CSR building, chain assembly and single-name issuance live in one place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The base branch dropped the no-SNI local-address fallback, so a handshake
without SNI now asks Vault to sign a request with no names at all, which a
PKI role only accepts with require_cn=false.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reseat the Vault issuer on the DER-based `sign` seam the base branch settled on:
`vaultIssuer` no longer holds a key config or issues certificates itself, it just
parses the request and forwards it to Vault. Leaf key generation, CSR building and
chain assembly now live in `automation` for both backends.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-point the Vault backend at the `certificateRequest` struct the base branch
introduced: `sign` takes the request instead of DER, so `vaultIssuer` asks it for a
CSR only when it needs one to forward, reads the names off its fields, and no longer
carries its own `ttl`. `commonName` and `verifyIssuedCertificate` read the struct too,
which leaves no `x509.ParseCertificateRequest` in the package.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@clement0010
clement0010 requested a review from minhtule September 8, 2026 13:29
Comment thread internal/sshhandler/ca.go
Comment thread internal/vault/vault.go Outdated
Comment on lines +37 to +66
tests := []struct {
name string
authSetup func(t *testing.T) gatewayconfig.VaultAuthConfig
}{
{
name: "token",
authSetup: func(t *testing.T) gatewayconfig.VaultAuthConfig {
t.Helper()

return gatewayconfig.VaultAuthConfig{
Token: testutil.SetupVaultToken(t, vaultContainerID),
}
},
},
{
name: "approle",
authSetup: func(t *testing.T) gatewayconfig.VaultAuthConfig {
t.Helper()

roleID, secretID := testutil.SetupVaultAppRole(t, vaultContainerID)

return gatewayconfig.VaultAuthConfig{
AppRole: &gatewayconfig.VaultAppRoleConfig{
RoleID: roleID,
SecretID: secretID,
},
}
},
},
}

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.

We can do it later but I think it's better to have a dedicate test for Vault auth. When we support a new auth method, we shouldn't have to add them to both this and SSH test.

Comment thread test/integration/testutil/vault.go
Comment thread internal/connect/cert/issuer.go Outdated
Comment thread internal/connect/cert/issuer.go Outdated
Comment thread internal/connect/cert/issuer_test.go
Comment thread internal/connect/cert/issuer_test.go Outdated
Comment thread internal/connect/cert/issuer_test.go Outdated
Comment thread internal/connect/cert/issuer_test.go Outdated
Comment on lines +231 to +232
// pem_bundle returns the CA chain concatenated onto the leaf
"format": "pem_bundle",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it'd be simpler to use pem_bundle to let Vault return the certificate chains. So we avoid constructing the chain ourselves with vaultCAChainPEMs()

Comment thread test/integration/testutil/vault.go Outdated
Comment thread internal/connect/cert/issuer.go Outdated
Comment thread internal/connect/cert/issuer.go Outdated
Comment thread internal/connect/cert/issuer_test.go Outdated

@minhtule minhtule 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.

LGTM 👍

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.

3 participants