Skip to content
This repository has been archived by the owner. It is now read-only.

Add Vault Cache and Exponential Backoff for Login #236

Merged
merged 3 commits into from May 23, 2018

Conversation

Projects
None yet
7 participants
@rfliam
Copy link
Contributor

commented Dec 14, 2017

Add a cache to the Vault credentials manager. The cache is flagable
and defaults to off. The cache uses the vault set lease duration and
re-fetches secrets halfway to the expiration time.

Add an exponential backoff for login and renewal of vault credentials.
This slows down spam to vault in the event of a bad configuration or
during heavy load. This backoff starts at 1 second and max's out at 64
seconds. This is currently not configurable.

As part of the change refactor so to test both the cache and the retry
strategies. Add unit tests to this effect. It also adds more comments
in the code and cleanups up some go naming stuff.

This change has not been tested with a full deployment yet (I think the PR will do that for me? ;p)

This is needed because if a user configures secrets in to be passed as params to a check resource it
is trivial to inadvertently attack your own secrets infrastructure.

I considered trying to add a "meta" credentials cache, but I'm not sure that a single caching strategy/manager will be able to correctly abstract all possible credential stores expected caching behaviors. For example k8s already has a sophisticated caching mechanism available in its standard library.

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Dec 14, 2017

@rfliam Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster

This comment has been minimized.

Copy link

commented Dec 14, 2017

@rfliam Thank you for signing the Contributor License Agreement!

@jtarchie

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2017

Adding caching at the application level may be masking the issue. It seems your vault deployment might need to be scaled out to handle the traffic coming into it. Though concourse is causing the load, delegating a caching layer just for vault doesn't seem right to me.

What have you tried actually scaling out vault?

@rfliam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2017

Our vault instance could handle the load, it is just completely unnecessary. The vault API is designed to use caching. That is why lease_duration exists. Most vault clients feature it.

Concourse is doing the "wrong thing" by not caching a secret that is intended to be cached.

One pipeline with vault secret configured sent our load from 500 TPS to 1300 TPS. (The lack of exponential backoff and a hard-coded 1s retry is also a distributed systems no no).

@vito

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

/cc @EugenMayer - this PR would probably make you happy

also ref. hashicorp/vault#3651

Kind of a bummer that there's no blessed Go client that implements this kind of functionality; I was hesitant to implement it ourselves because caching credentials sounds like a bad idea if there's no precedent or strong need for it. But it sounds like there's a means for it via Vault's API, and precedent in other popular libraries, though it's still kind of a bummer that we have to implement the caching ourselves.

Given that, I want to be super careful about this and make sure we're taking into consideration any "gotchas" with the implementation (e.g. accidentally using an expired credential after it's been rotated), and any security risks involved (e.g. holding all the secrets in the ATC's head). I say this having not yet reviewed the code in detail.

Are there other Go consumers of Vault we can look to as an example? Have other projects run into this pain point?

@rfliam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2017

Agreed on the go client not having caching being a bit of a bummer. But on the other hand the nature of the cache is probably use case specific anyways. Concourse for example is a read mostly cache. Requests for previously un-requested secrets are going to be very low.

Hence having a cache focused on fast reads and low memory overhead for concourse makes sense. We could use golang-lru from hashicorp but it doesn't actively purge records and does cache eviction on the main thread.

The cache I implemented is very efficient for this use case, only performing maintenance when keys expire. However it does not have a strict upper bounds in memory size. This is a complicated matter for vault secrets as they are of arbitrary size so having a max number of secrets only helps some in preventing large memory footprints.

It's also worth mentioning it looks like the original code re-implemented renewer and I followed suite. So that should probably be changed out.

}

// TODO: Should a cache have a max size to
// prevent unbounded growth?

This comment has been minimized.

Copy link
@EugenMayer

EugenMayer Dec 18, 2017

if we do that, does it then rather make sense going for redis in a term, that this is already offered there and its also a memory cache and flushed when we restart the services ( security reasons ).

@@ -17,6 +18,8 @@ type VaultManager struct {

PathPrefix string `long:"path-prefix" default:"/concourse" description:"Path under which to namespace credential lookup."`

Cache bool `bool:"cache" default:"false" description:"Cache returned secrets for their lease duration in memory"`

This comment has been minimized.

Copy link
@EugenMayer

EugenMayer Dec 18, 2017

Should we also add a adjustable value for secret.LeaseDuration - so people can adjust to their usecase? E.g in our case, those secrets are 99% read only and if we write, we would restart ATC. People mighr use vault differently with AWS and rotation, so setting this down a lot ( or disable the cache ). But being able to define the general aggressiveness would be a way to help people adjusting to their use-case very easy, with very little effort on our side. What do you think?

@jefferai

This comment has been minimized.

Copy link

commented Dec 18, 2017

Hi there,

A long while back Vault did in fact issue leases for the generic (now kv) backend but we took that out because it was causing problems with exactly the kind of behavior that Concourse displays -- refreshing from each client once a second was a really great way to generate a massive number of leases and bring Vault crashing down. We kept the ability for the writer to round-trip a lease duration, with the idea that the writer often knows how often they're going to write new values, so it can use that value as an expected lifetime hint for readers.

What we usually see is that people will set a value for the lease duration that is significantly less than the actual expiration, but large enough to not cause high load. For example, let's say you rotate a credential every 12 hours and keep the old one active for 2 hours to allow clients to adjust. The writer of the credential may set the lease duration to 90 minutes, which should be enough time for all clients to retrieve the new value (with 5399 less reads from Vault per client in the interim compared to checking every second :-D ).

Although the API could do local caching, you'd need to know how long the cache should hold that value anyways, and still be capable of retrieving new values when outside the caching period. So a built-in cache doesn't buy you much other than the ability to continue checking every second instead of simply setting your timer to check at the interval given by the secret.

Eventually we hope to add an eventing API, but it's not going to appear in a "soon" timeframe. If your underlying data store supports an eventing API you could do some mapping of Vault storage to data store storage and use an event on the underlying data store path to trigger a re-read from Vault clients, but I would generally avoid that unless absolutely necessary for your use-case.

@vito

This comment has been minimized.

Copy link
Member

commented Dec 18, 2017

@jefferai Thanks for helping us figure this out!

I think there's one thing to clarify: Concourse doesn't ever explicitly refresh anything or do anything special with leases, let alone refresh once a second. We fetch a credential any time we need it, and throw it away when we're done with it.

Here's what's happening on Concourse's side:

  1. Say a user has configured a resource in their Concourse pipeline (resources are just YAML config representing an integration point, i.e. a Git repo or an S3 bucket). Say this resource has a configuration pointing to ((private_key)).
  2. Concourse periodically performs a check operation on every pipeline resource, by default once per minute. This is to discover things like new commits to said Git repo.
  3. If a resource has a ((parameter)), every time we check we'll resolve those parameters by fetching them from Vault.
  4. We run the resource's check script, passing it the resource's configuration, with the parameterized credentials filled with the values fetched from Vault.

After running the check script, we don't do anything with the credentials - in fact, they're out of the ATC's head as soon as we've started running the script. Once the checking interval fires again, we'll just fetch the credentials anew. If they're rotated, we'll have the new values. Or, a user may have reconfigured the pipeline in the meantime, and we may be fetching a new set of credentials. Who knows!

So from Concourse's perspective, these credentials are short-lived. They aren't used throughout the duration of the Concourse server running, for example. The stack is stateless, so we don't have any 'refreshing' mechanism, we just don't use the credentials for very long in the first place.

Considering that, and given the existence of lease durations as a way for the credential writer to convey a safe validity window (separate from expiry), this feels like a generic client-side cache. I'm just worried about us being the ones to write it. ;)

...And quite worried that this may result in a single process that contains literally all of the credentials for all of the pipelines and all of the teams in its head. Not as a question of memory usage, but as a question of risk, and to what extent that defeats the point of using Vault in the first place.

@jefferai

This comment has been minimized.

Copy link

commented Dec 19, 2017

Hi @vito

Thanks for the explanation, that helps me understand what Concourse is doing here.

Not going in order:

...And quite worried that this may result in a single process that contains literally all of the credentials for all of the pipelines and all of the teams in its head. Not as a question of memory usage, but as a question of risk, and to what extent that defeats the point of using Vault in the first place.

I can't really help you assess whether that risk is acceptable or not, but it's certainly a better stance than persisting to disk. Using Vault helps sort out the "not persisting to disk in plaintext" part of the equation (and a lot of other functionality), so even if you are caching things on the client side you're still getting benefit.

The stack is stateless, so we don't have any 'refreshing' mechanism, we just don't use the credentials for very long in the first place.

Between this and the other discussion of client side caching, I'm a bit confused -- do you keep your API clients around? If so that's not really stateless, but if not, I don't really see how caching in the clients will help.

@vito

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

@jefferai

I can't really help you assess whether that risk is acceptable or not, but it's certainly a better stance than persisting to disk. Using Vault helps sort out the "not persisting to disk in plaintext" part of the equation (and a lot of other functionality), so even if you are caching things on the client side you're still getting benefit.

We're still getting a lot of benefit from Vault in plenty of ways, I'm just wary of any step back we may make in security. CI systems are an especially lucrative attack target. Either way it's on us to characterize the risk involved and figure out what we can/should do.

Between this and the other discussion of client side caching, I'm a bit confused -- do you keep your API clients around? If so that's not really stateless, but if not, I don't really see how caching in the clients will help.

Sorry, all I meant by stateless was that the credentials themselves aren't e.g. stashed in an object somewhere in need of periodic refreshing. They're fetched and then thrown away. Though I guess that's how a lot of Vault consumers work anyway. We do probably have a single API client, so caching would be fine.

@jefferai

This comment has been minimized.

Copy link

commented Dec 19, 2017

Hi @vito,

Thanks for the explanation. If you do work on a caching system I'd love for it to be in the API client since I think others could benefit. I have a couple thoughts around it:

  • One way might be to allow the Renewer function in the API to work on secrets that don't have a lease ID by providing a function that can be run to generate a new Secret. That way the renewer can look at the lease duration and know that after e.g. 95% of the duration has expired, run the function to fetch a new one. (It would also allow the renewer to fetch new secrets generally, rather than only renew them.)

  • Much of Consul-Template got pulled into a library a while back, which is how it's embedded into Nomad. Based on a template it knows how to fetch (and appropriately handle refetching) for a large variety of Vault secrets. The downside is that templates have to be written to use it.

Probably the first option would be the way to go for now. I do think there should be controls around disabling caching, perhaps on a per-secret (or per-path?) basis, in case the duration that comes back can't be trusted (or some secret shouldn't be kept around in memory).

@rfliam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

Some Design Thoughts:

Current Security Model and Risks

Currently concourse keeps secrets for all team accessible by a single login managed in the ATC. If an attacker achieves some sort of "escalation" he can pull that token, client key, and client cert and access all secrets for all teams. While this information is not kept in memory, the token is, and it would still be readily available.

Escalation for this token is somewhat difficult as it requires attacking the ATC. The "exploit vector" here is largely through the ATC API. The most common methodology here would be to dump the heap or the configuration of the ATC.

Exploits that escalate using containers in pipeline steps are prevented from attacking the ATC if it is run on physically isolated nodes (this is probably a best practice for the future). However, privilege escalations on a worker could be used to attack other pipelines secrets that happen to be local. If the ATC is local this could also be attacked.

The Impact of Caching

Maintaining secrets in memory offers an increased risk in "timeliness" of attack. If the client certificate is ip addressed pinned or the ATC but not the secrets back end is accessible this voids some of its benefits. With a cache a user must only dump the heap to access the secrets as opposed to dumping the heap repeatedly, as secrets are in flight.

In practice this additional risk is low as most heap dumps are the result of existing remote code execution which can be crafted to attack these secrets directly.

Possible Changes

Per-Pipeline Secrets Management

Maintaining a per team or per pipeline login to vault could help reduce the magnitude of any breach. The secrets configuration could be set on the pipeline rather then globally for the ATC. The trade off is this information would have to be persisted. However, the secrets back-end could be used for this. The ATC would be configured with a secrets back-end in which to store and access the pipeline specific secrets config.

Changing Where and How Secrets Access Works

Another increase in security would be from "splitting" the secrets access and decryption. In this model the access to the secret would reside with the ATC or Pipeline, but the secret itself would be further [encrypted with the private key of the worker]. The ATC would access the secret and forward it the worker which would then decrypt the secret for use. This adds some security as it requires a working exploit on both the worker and the atc.

Another possibility that is (probably) less work is for secrets resolution to happen in an entirely separate process. A "secrets resolver" could be placed in between the worker and the ATC. It's sole job would be to resolve secrets before handing them off to the worker. This would mean simply dumping the ATC's heap through some exploit would buy you nothing. Think the TSA, but for secrets (or possibly just as part of the TSA ;p).

Add Build Signing

One thing I would love to see is the addition of a build singing infrastructure. In this infrastructure tasks could request that concourse sign a resource with it's private keys so traceable builds could be used. Another usage would be requiring deployments to come from the build infrastructure itself, signing deployment requests with the build infrastructures private keys.

[1] More correctly the secret would be stored with a randomly generated AES encryption key, and that key would be encrypted with the workers public key.

Proposals

Add the Cache (in the Vault API Client Code)

Compared to today's model the cache is only a minor increase in risk. However, it is nearly required for most secrets backends. As evidenced by #233 and vault#3651 secrets without caching can cause significant problems.

I very much like jefferai's suggestion to disable caching on a per path basis.

Future

Look towards per-pipeline secrets and build signing first IMO. But that's up to you guys.

@jefferai

This comment has been minimized.

Copy link

commented Dec 19, 2017

A couple of comments:

The trade off is this information would have to be persisted. However, the secrets back-end could be used for this. The ATC would be configured with a secrets back-end in which to store and access the pipeline specific secrets config.

You may want to think about using transit for this -- either directly for encryption/decryption or to allow dissemination of an encrypted data key that a worker can decrypt via transit and hold locally in memory.

One thing I would love to see is the addition of a build singing infrastructure. In this infrastructure tasks could request that concourse sign a resource with it's private keys so traceable builds could be used. Another usage would be requiring deployments to come from the build infrastructure itself, signing deployment requests with the build infrastructures private keys.

Just in case you haven't seen it, transit also has three different key types that can be used for signing (rsa, ed25519, ecdsa-p256) :-)

@jefferai

This comment has been minimized.

Copy link

commented Dec 19, 2017

BTW, I just mention the signing since the text discusses workers' private keys; I'm not sure how those are disseminated/stored, but if they already have a Vault token, then you could just keep the keys in Vault and guard access to it.

@rfliam

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

Another thought here:

The cache can use memguard to help prevent buffer overflow attacks.

@jefferai

This comment has been minimized.

Copy link

commented Dec 20, 2017

Be careful with that line of thinking -- Go already prevents such attacks (subject to things working the way they're supposed to) by taking memory management out of your hands. memguard needs to advertise buffer overflow protection because it works around Go's memory management, which opens it up to a host of potential attacks that it then needs to mitigate.

I've been watching memguard for a while, and while I think it's an interesting library, as far as I can tell the main use of memguard is if you are concerned about values living for long periods in memory after you've given them up and don't want to try mitigating by periodically triggering garbage collections in Go. It won't stop someone that has root from getting your memory -- it will just make it harder via obfuscation.

Unless you feel super strongly about needing memguard, I'd just use something that supports expiry and/or running functions on eviction like https://github.com/patrickmn/go-cache or similar.

@william-tran

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2017

I considered trying to add a "meta" credentials cache, but I'm not sure that a single caching strategy/manager will be able to correctly abstract all possible credential stores expected caching behaviors. For example k8s already has a sophisticated caching mechanism available in its standard library.

I'm a k8s user. I'd be happy with a simple caching strategy that applied to all credential managers, with a configurable TTL defaulting to 60 seconds. k8s secrets don't convey anything around expiry, and giving watch or list permissions to implement anything fancier for caching should be avoided:

https://kubernetes.io/docs/concepts/configuration/secret/#best-practices

For these reasons watch and list requests for secrets within a namespace are extremely powerful capabilities and should be avoided, since listing secrets allows the clients to inspect the values if all secrets are in that namespace. The ability to watch and list all secrets in a cluster should be reserved for only the most privileged, system-level components.

There is also a PR to fix the k8s secret lookup so that access to all secrets in a namespace isn't assumed: #233.

@vito

This comment has been minimized.

Copy link
Member

commented Feb 13, 2018

Just chiming in to say we haven't forgotten about this, we're just working on getting 3.9 out the door before merging this in as the next release is already pretty huge. Thanks for your patience!

@vito
Copy link
Member

left a comment

Phew, ok, it's been a while since I've looked at this so having a bit of a refresher of where we landed may be necessary.

Here's my understanding:

  • We're OK with caching credentials in-memory, because if we can't trust memory that's a much bigger problem. There already exists a similar problem today in that the ATC stores its Vault token in-memory.
  • Ideally this caching could be implemented for all backends, but Vault at least has the clearest expiry semantics, so it's fine to just pull this in for now. If we come up with a general solution later we can always go back, since this is barely user-facing (just --cache).

So tl;dr we're all good with this change being merged in.

Except I totally threw a wrench into things by merging #256 which touches the Vault factory code and changes the login flow a bit. This will need to be rebased and consolidated with that PR.

I also see a few TODOs in the code as-is - I hunted each one down and added a comment. A couple of them are related to #256.

Thanks again for your patience, this one's just a bit complicated so it takes more time to digest. We're also just now getting back to PRs; the holidays and site revamp took a lot of time.

}

// TODO: What if that token becomes invalid (was deleted for
// example)? Shouldn't we try to login again...?

This comment has been minimized.

Copy link
@vito

vito Apr 11, 2018

Member

Yep! I think this is how we can consolidate with #256.

}

// TODO: Should a cache have a max size to
// prevent unbounded growth?

This comment has been minimized.

Copy link
@vito

vito Apr 11, 2018

Member

Hmm, yeah this could be really interesting for large multi-tenant Concourse installations. Some credentials can be fairly large (e.g. private keys), so it could add up quickly. You could always just not use caching, but chances are you probably want caching in that same scenario, so it becomes a question of which you sacrifice: ATC RAM vs. Vault CPU.

I think it's fine to ship without this, but maybe the thing to do here is just have a configurable max retention for each cache? This could either be time-based or usage based (i.e. each time a credential's cache is hit, we keep it for a bit longer, still limited by its expiry obviously).

return NewVaultFactory(logger, client, manager.Auth, manager.PathPrefix), nil
c := NewAPIClient(logger, client, manager.Auth)

// TODO: Configurable?

This comment has been minimized.

Copy link
@vito

vito Apr 11, 2018

Member

Yeah, might as well make these configurable. They'd be right at home next to the auth max duration flag from #256.

Also, why 64 seconds? :P

Add Vault Cache and Exponential Backoff for Login
Add a cache to the Vault credentials manager. The cache is flagable
and defaults to off. The cache uses the vault set lease duration and
re-fetches secrets halfway to the expiration time.

Add an exponential backoff for login and renewal of vault credentials.
. This slows down spam to vault in the event of a bad configuration or
during heavy load.

As part of the change refactor so to test both the cache and the retry
strategies. Add unit tests to this effect. It also adds more comments
in the code and cleanups up some go naming stuff.

@rfliam rfliam force-pushed the rfliam:master branch from e607e4f to 7d7717f May 2, 2018

Add MaxLeaseDuration For Vault Cache.
The vault cache now supports an overridable max lease duration to
force secret renewal before the normal deadline.
@rfliam

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2018

Addressed comments from code reviews. Merged work on re-logging in. Added that flagable max lease duration for the cache per @EugenMayer .

Params []template.VarKV `long:"auth-param" description:"Paramter to pass when logging in via the backend. Can be specified multiple times." value-name:"NAME=VALUE"`
Backend string `long:"auth-backend" description:"Auth backend to use for logging in to Vault."`
BackendMaxTTL time.Duration `long:"auth-backend-max-ttl" description:"Time after which to force a re-login. If not set, the token will just be continuously renewed."`
RetryMax time.Duration `long:"retry-max" description:"The maximum time between retries when logging in or re-authing a secret."`

This comment has been minimized.

Copy link
@vito

vito May 14, 2018

Member

Should this (and RetryInitial) have defaults? I think they're passing along as 0 at the moment. This can be done with default:"10s" or the like.

Add Sensible Defaults for Vault RetryMax and RetryInitial
Defaults previously used underlying library values. Add sensible
defaults for systems. Cleanup annotation formation.
@rfliam

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

Added.

@vito

vito approved these changes May 23, 2018

@vito

This comment has been minimized.

Copy link
Member

commented May 23, 2018

happening

thanks for hanging in there!

@vito vito merged commit 9b8977b into concourse:master May 23, 2018

2 checks passed

ci/pivotal-cla Thank you for signing the Contributor License Agreement!
Details
concourse-ci/status Concourse CI build success
Details
@vito

This comment has been minimized.

Copy link
Member

commented May 25, 2018

Found a couple problems.

First problem: code was blocking on <-loggedIn in the variables factory, because this channel would only be written to upon logging in. I've changed this to a channel that's closed the first time we successfully log in, so these channel reads will never block again.

Second problem: when copying the Vault client and setting a token, I observed a deadlock in the stack that happened because copying the client also copies its mutexes (they're non-pointer values, relying on the struct), and it happened to copy it while the mutex was locked. I'm gonna find a better way to do this since it already had a TODO with a warning on it. The warning was right!

@jefferai

This comment has been minimized.

Copy link

commented May 25, 2018


func clientWithToken(client *vaultapi.Client, token string) *vaultapi.Client {
// TODO: This is dangerous, we are relying on the vault
// client being ok with a shallow copy....

This comment has been minimized.

Copy link
@jefferai
@jefferai

This comment has been minimized.

Copy link

commented May 25, 2018

Can I ask -- is there a reason you wrote ReAuther instead of using https://godoc.org/github.com/hashicorp/vault/api#Renewer ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.