UKHomeOffice / vault-sidekick

Vault sidekick
Apache License 2.0
195 stars 62 forks source link

Fix lease expiration checks to prevent permenently revoked resources after temporary `vault` issue #96

Closed cpick closed 4 years ago

cpick commented 4 years ago

Currently, time.Duration(secret.LeaseDuration) is added to the current time to determine leaseExpireTime. This is erroneous because secret.LeaseDuration is in seconds and time.Duration stores nanoseconds. This causes leaseExpireTime to be effectively set to ~time.Now().

Due to the above bug, time.Now() is always after leaseExpireTime, but the code also has a bug reversing the meaning of the lease expiration check so the lease-expired codepath is never taken. This means that if a lease ever does expire (which we've seen when there are vault connectivity issues that last longer than the 5-20% cushion in the lease-renewal time) then vault-sidekick erroneously attempts to renew it forever even though it should really re-retrieve the resource.

Multiply the duration by time.Second to get the right units to fix leaseExpireTime and fix the lease expiration check to work properly. Together these mean that temporary vault issues don't result in permanently-expired/revoked vault-sidekick resources.

These fixes remove the need for / closes #89

james-bjss commented 4 years ago

Thanks @cpick