-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add certificates to Infisical agent #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Greptile OverviewGreptile SummaryThis PR extends the Infisical Agent from a secrets management tool to also handle PKI certificate lifecycle management. The changes introduce comprehensive certificate management capabilities including automated issuance, renewal, and file management. The implementation adds new API models for certificate operations (IssueCertificateRequest, CertificateAttributes, etc.) and corresponding API client functions for issuing, retrieving, and renewing certificates. The core functionality is integrated into the agent's main monitoring loop, providing concurrent certificate lifecycle management alongside existing secrets operations. The certificate management engine supports both manual certificate specification and CSR-based workflows, implements configurable renewal policies to prevent expiration, and includes post-event hooks for downstream system integration (like reloading web servers). A new configuration file demonstrates practical usage with nginx integration, and the implementation follows the agent's existing architectural patterns of concurrent goroutines, unified configuration management, and graceful shutdown handling. Important Files Changed
Confidence score: 3/5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 2 comments
| CertificateRequestID string `json:"certificateRequestId,omitempty"` | ||
| } | ||
|
|
||
| type GetCertificateRequestResponse struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it seems there is a lint issue here
| } `yaml:"config"` | ||
| } | ||
|
|
||
| type Certificate struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to name this as AgentCertificateConfig likewise appending everything with agent
| } | ||
|
|
||
| return tm.infisicalClient.Auth().UniversalAuthLogin(clientID, clientSecret) | ||
| log.Debug().Msgf("calling UniversalAuthLogin with clientID: %s", clientID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
| log.Error().Msgf("UniversalAuthLogin failed: %v", err) | ||
| return infisicalSdk.MachineIdentityCredential{}, err | ||
| } | ||
| log.Debug().Msg("UniversalAuthLogin succeeded") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here
| return | ||
|
|
||
| case "failed": | ||
| log.Error().Msgf("certificate %s renewal failed", displayName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to add additional context on which certificate this error corresponds to like certificateId, commonName Like a standardize format for all the logs shown here. This is because as user can setup multiple certs, they may not understand the source of this error
| } | ||
|
|
||
| func (tm *AgentManager) PollCertificateRequest(certificateId int, certificate *Certificate) { | ||
| displayName := tm.getCertificateDisplayName(certificateId, certificate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing mutex hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I saw it below. Same comment. It's better move it here
| return true | ||
| } | ||
|
|
||
| if renewBefore, err := parseDurationWithDays(cert.RenewBeforeExpiry); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for invalid days
| } | ||
|
|
||
| func (tm *AgentManager) ExecutePostHook(command string, timeoutSecs int64, hookType string, certificateId int, certificate *Certificate) { | ||
| if command == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing mutex
| func (tm *AgentManager) ShouldRenewCertificate(certificateId int) bool { | ||
| state := tm.certificateStates[certificateId] | ||
|
|
||
| if state.Status != "active" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we show an error message incase the certificate is revoked or expired?
| } | ||
|
|
||
| func (tm *AgentManager) WriteCertificateFiles(certificate *Certificate, response *api.CertificateResponse) error { | ||
| destPath := certificate.DestinationPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing mutex
Description 📣
This PR introduces certificate management capabilities to the Infisical Agent. It adds support for:
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets