feat(kmip): add token and AWS enrollment for KMIP server#254
Conversation
Adds --enroll-method=token|aws to 'kmip start' and 'kmip systemd install', persisting and reusing the enrollment access token, alongside the existing machine-identity flow.
|
💬 Discussion in Slack: #pr-review-cli-254-feat-kmip-add-token-and-aws-enrollment-for-kmip-server Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
|
| Filename | Overview |
|---|---|
| packages/cmd/kmip.go | Core command handler for kmip start and kmip systemd install; adds token/AWS enrollment flows with a logic hole (fallthrough to re-enrollment with an already-spent token) and unvalidated domain used as the API endpoint. |
| packages/kmip/enroll.go | New file: key/value config store for enrollment state (access token, enrollment token, server ID, domain) at ~/.infisical/kmip/<name>.conf or /etc/infisical/kmip/<name>.conf; file permissions are 0600, logic is correct. |
| packages/kmip/aws_auth.go | New file: signs a presigned STS GetCallerIdentity request and forwards its headers to Infisical for AWS Auth enrollment; standard pattern, no actual outbound STS call is made. |
| packages/kmip/systemd.go | Refactored into helpers (kmipCommonEnv, writeKmipSystemdService, systemdPreconditionsMet) and two new installers for token/AWS methods; env-file values are not sanitized against newline injection. |
| packages/api/api.go | Adds CallKmipServerLogin following the same pattern as adjacent API calls; no issues found. |
| packages/api/model.go | Adds KmipServerLoginRequest and KmipServerLoginResponse structs; straightforward, no issues. |
Reviews (1): Last reviewed commit: "feat(kmip): add token and AWS enrollment..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| payloadHash := fmt.Sprintf("%x", hash.Sum(nil)) | ||
|
|
||
| signer := v4.NewSigner() | ||
| if err := signer.SignHTTP(ctx, awsCredentials, req, payloadHash, "sts", awsRegion, time.Now()); err != nil { |
There was a problem hiding this comment.
Medium: AWS enrollment proof is replayable
The SigV4 request only proves the AWS principal; kmipServerID is sent later as unsigned JSON. Anyone who obtains this enrollment payload can replay the same iamRequestBody and iamRequestHeaders with a different kmipServerId during the SigV4 validity window, authenticating this host to any KMIP server configuration that trusts the same AWS principal. Add a custom audience header containing the KMIP server ID before SignHTTP, forward it in IamRequestHeaders, and have the API verify that the signed header matches the JSON kmipServerId.
PR overviewThis pull request adds KMIP server enrollment support using either enrollment tokens or AWS IAM/SigV4 proof, including command-line handling for KMIP enrollment. It introduces AWS authentication request construction and token-based enrollment paths for KMIP server setup. There are currently two open security issues in the enrollment flow. The AWS enrollment proof can be replayed within its validity window because the KMIP server ID is not bound into the signed request, and token-based enrollment currently exposes the token through process arguments where local users may read it. No issues have been fixed or addressed yet, so the PR still needs changes before the enrollment mechanisms are safe to use. Open issues (2)
Fixed/addressed: 0 · PR risk: 6/10 |
| SetResult(&resBody). | ||
| SetHeader("User-Agent", USER_AGENT). | ||
| SetBody(request). | ||
| Post(fmt.Sprintf("%v/v1/kmip-servers/login", config.INFISICAL_URL)) |
There was a problem hiding this comment.
I know this is for the other PR but can we instead just keep this as kmip/servers instead of kmip-servers? (since we already do kmip/clients, just to be consistent
Picks up the published release with the AccessToken field, so the CLI builds without the local replace directive.
For enrollment-based KMIP servers the cert config (hostnames/IPs, TTL, common name, key algorithm) is configured in the UI and read by the daemon via /kmip/servers/connect, so --hostnames-or-ips is no longer required at launch.
infisical kmip start/systemd install now take <server-name> as a positional argument, e.g. 'infisical kmip start my-server'. The --server-name flag and INFISICAL_KMIP_SERVER_NAME env var still work as alternatives; the name is required (no longer defaults to kmip-server).
|
|
||
| serverName, err := util.GetCmdFlagOrEnvWithDefaultValue(cmd, "server-name", []string{INFISICAL_KMIP_SERVER_NAME_ENV_NAME}, "kmip-server") | ||
| // Enrollment token path | ||
| enrollToken, _ := cmd.Flags().GetString("token") |
There was a problem hiding this comment.
Medium: Enrollment token exposed in process arguments
--enroll-method=token only reads the enrollment token from the --token flag here and in the systemd install path. A local user can read another user's command line from process listings while enrollment is running and redeem that token for a KMIP server access token before the legitimate server does; accept the token from a less-exposed source such as INFISICAL_KMIP_ENROLLMENT_TOKEN, a token file, or stdin, and update the systemd install path the same way.
Picks up the /kmip/servers/connect daemon path so enrollment-based KMIP servers fetch their certificate from the platform instead of passing cert config at launch.
The e2e module (replace infisical-merge => ../) still pinned infisical-kmip v0.3.17 as an indirect dep, which left it un-tidy and failed the E2E 'go test' tidy check. Re-tidied to v0.3.19 to match the root module.
| util.HandleError(err, "Unable to parse hostnames or IPs") | ||
| } | ||
|
|
||
| enrollMethod, _ := cmd.Flags().GetString("enroll-method") |
There was a problem hiding this comment.
Wouldn't this mean that any manual restart silently falls back to legacy auth?
| if err := localkmip.InstallEnrolledKmipSystemdService(enrollResp.AccessToken, domain, listenAddress, serverName, certificateTTL, hostnamesOrIps); err != nil { | ||
| util.HandleError(err, "Failed to install systemd service") | ||
| } | ||
| case localkmip.EnrollMethodAws: |
There was a problem hiding this comment.
mmm is it intended that AWS does no auth validation at install time? 🤔
| err = localkmip.InstallKmipSystemdService(identityClientId, identityClientSecret, domain, listenAddress, serverName, certificateTTL, hostnamesOrIps) | ||
| if err != nil { | ||
| util.HandleError(err, "Failed to install systemd service") | ||
| enrollMethod, _ := cmd.Flags().GetString("enroll-method") |
There was a problem hiding this comment.
I think we are duplicating the code of lines 85-92 right? maybe we could unify it into a helper function? do you think it's worth it?
| util.HandleError(err, "Unable to parse identity client secret") | ||
| } | ||
|
|
||
| domain, err := util.GetCmdFlagOrEnvWithDefaultValue(cmd, "domain", []string{util.INFISICAL_API_URL_ENV_NAME}, "") |
There was a problem hiding this comment.
Here we use GetCmdFlagOrEnvWithDefaultValue for the domain, but on line 95 we use:
if flagDomain, _ := cmd.Flags().GetString("domain"); flagDomain != "" {
config.INFISICAL_URL = util.AppendAPIEndpoint(flagDomain)
} else if storedDomain, _ := localkmip.LoadStoredDomain(serverName); storedDomain != "" {
config.INFISICAL_URL = util.AppendAPIEndpoint(storedDomain)
} else if configFile, cfgErr := util.GetConfigFile(); cfgErr == nil && configFile.LoggedInUserDomain != "" {
config.INFISICAL_URL = util.AppendAPIEndpoint(configFile.LoggedInUserDomain)
}
| kmipStartCmd.Flags().String("enroll-method", "", "Enrollment method for the KMIP server: 'token' or 'aws'. When set, machine-identity flags are ignored.") | ||
| kmipStartCmd.Flags().String("token", "", "Enrollment token (when --enroll-method=token)") | ||
| kmipStartCmd.Flags().String("kmip-server-id", "", "KMIP server ID (when --enroll-method=aws)") | ||
| kmipStartCmd.Flags().String("domain", "", "Domain of your Infisical instance") |
There was a problem hiding this comment.
In one domain helper we say "Domain of your Infisical instance" in the other "Domain of your self-hosted Infisical instance" let's unify them please
3 participants
Description 📣
Adds
--enroll-method=token|awstoinfisical kmip startandkmip systemd install, so a KMIP server enrolls with a one-time token or AWS auth and reuses the derived access token across restarts; the legacy--identity-client-id/secretflow still works. Depends on a newinfisical-kmiprelease —go.modis still pinned tov0.3.17and must be bumped before merge (currently builds via a localreplace).Type ✨
Tests 🛠️
Manually enrolled a KMIP server via token and AWS (including
systemd install) against a local backend, verified registration and a real KMIP crypto op, and confirmed the legacy machine-identity flow still registers.