Skip to content

feat(agent): revoke credentials on shutdown#54

Merged
varonix0 merged 5 commits intomainfrom
daniel/agent-manage-leases
Nov 7, 2025
Merged

feat(agent): revoke credentials on shutdown#54
varonix0 merged 5 commits intomainfrom
daniel/agent-manage-leases

Conversation

@varonix0
Copy link
Member

@varonix0 varonix0 commented Nov 7, 2025

Description 📣

This PR allows for the agent to self-revoke managed credentials (currently dynamic secret leases and access tokens are supported). There's a new flag called revoke-credentials-on-shutdown (default to false), which dictates this behavior.

Additionally I also fixed multiple dynamic secret leases being treated as one single lease. We had a check like this before:

	for _, lease := range d.leases {
		if lease.SecretPath == secretPath && lease.Environment == environment && lease.ProjectSlug == projectSlug && lease.Slug == slug {
			return &lease
		}
	}

Which wasn't factoring in template ID's. This means if you have multiple dynamic secret templates using the same dynamic secret, it would treat all those templates as the same. This has also been resolved in this PR.

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

@varonix0 varonix0 self-assigned this Nov 7, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This PR adds self-revocation of managed credentials on agent shutdown via a new revoke-credentials-on-shutdown flag (defaults to false). It also fixes a bug where multiple dynamic secret leases using the same dynamic secret were incorrectly treated as a single lease by adding LeaseID comparison to the deduplication logic.

Key Changes:

  • Added RevokeCredentialsOnShutdown configuration flag to control credential cleanup behavior
  • Implemented RevokeCredentials() method that revokes dynamic secret leases and access tokens during shutdown
  • Fixed lease deduplication by comparing LeaseID in addition to other fields (SecretPath, Environment, ProjectSlug, Slug)
  • Added TemplateWithID wrapper to track template IDs for better lease management
  • Added GetAllLeases() method to retrieve all active leases
  • Added isShuttingDown flag to gracefully stop background goroutines

Issues Found:

  • Critical syntax error at line 1293: undefined err variable will cause compilation failure
  • Critical race condition: GetAllLeases() returns internal slice without copying while lock is released, allowing concurrent modification
  • Logic bug: Template files are cleared for any lease matching a templateID, even when other active leases still need that file
  • Performance concern: 20-second blocking wait for token during shutdown creates unnecessary delay

Confidence Score: 1/5

  • This PR contains critical bugs that will cause compilation failure and runtime race conditions
  • Score reflects compilation-blocking syntax error at line 1293, race condition in GetAllLeases() that can corrupt lease data, and logic bug that prematurely clears template files. The undefined err variable will prevent the code from compiling.
  • packages/cmd/agent.go requires immediate attention - fix syntax error at line 1293, race condition in GetAllLeases(), and template file clearing logic

Important Files Changed

File Analysis

Filename Score Overview
packages/cmd/agent.go 2/5 Adds credential revocation on shutdown with new flag. Fixed lease deduplication bug by including LeaseID. Critical issues: race condition in GetAllLeases(), undefined err variable at line 1293, template file clearing logic bug.

Sequence Diagram

sequenceDiagram
    participant User
    participant Agent
    participant TokenManager
    participant LeaseManager
    participant InfisicalAPI
    
    User->>Agent: SIGINT/SIGTERM
    Agent->>Agent: Set isShuttingDown = true
    
    alt revokeCredentialsOnShutdown enabled
        Agent->>TokenManager: RevokeCredentials()
        TokenManager->>TokenManager: Wait for token (max 20s)
        
        TokenManager->>LeaseManager: GetAllLeases()
        LeaseManager-->>TokenManager: Return leases
        
        TokenManager->>LeaseManager: Lock mutex
        
        loop For each dynamic secret lease
            TokenManager->>InfisicalAPI: DeleteById(leaseId)
            alt Lease found
                InfisicalAPI-->>TokenManager: Success
                TokenManager->>TokenManager: Clear template file
            else Lease not found (404)
                InfisicalAPI-->>TokenManager: 404 error
                TokenManager->>TokenManager: Skip lease
            end
        end
        
        loop For each sink file
            TokenManager->>TokenManager: Read token from file
            TokenManager->>InfisicalAPI: RevokeAccessToken()
            InfisicalAPI-->>TokenManager: Success
            TokenManager->>TokenManager: Clear token file
        end
        
        alt Active token not in deleted list
            TokenManager->>InfisicalAPI: RevokeAccessToken()
            InfisicalAPI-->>TokenManager: Success
        end
        
        TokenManager->>LeaseManager: Unlock mutex
        TokenManager-->>Agent: Return result
    end
    
    Agent->>Agent: os.Exit(exitCode)
Loading

1 file reviewed, 5 comments

Edit Code Review Agent Settings | Greptile

@varonix0 varonix0 merged commit 5c0f96c into main Nov 7, 2025
3 checks passed
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