Add API helper for renewing a secret

[sethvargo]

This adds a client API helper for renewing a secret with a configurable grace period. I really got sick of writing the same code over and over again, and figured the API client could benefit here. This isn't an end-all-be-all renewal method; clients which want more control should implement this logic themselves. However, for the normal basic cases, it suffices.

This is really hard to test. There's no way to spin up a Vault server in the api package because the vault core depends on the API package, creating a circular dependency. I didn't want to do a huge refactor and shuffle things around for this tiny PR.

Thanks to @ryanuber, I have a relatively good way to test this!

[jefferai]

Is there a reason to invoke the binary rather than simply launching three test cores with listeners using TestCluster? There are a number of benefits -- it includes HA, fully-set-up clients using TLS, and more.

[jefferai]

One of the issues with doing it this way for example is having to coordinate with API vs. changes in other packages, so if this test is run without first compiling and placing a new version of vault in your path then you'll have problems.

[ryanuber]

We followed a similar pattern in Consul - the main reason to use this style was to avoid a cyclic dependency. If TestCluster doesn't create one, then great! Maybe we can use that instead.

For actually running these tests, what we did with Consul was to just compile the "dev" target prior to running the "test" target in the Makefile. That way we could easily launch the binary from $PWD/bin/vault using the current HEAD.

[briankassouf]

To avoid a cyclic dependency in the tests you could make the test file's package name package api_test.

[jefferai]

@ryanuber See my next comment about using a helpers/testing package. Just put the tests elsewhere and there is no concern with a cyclic dependency.

@bk You could but you can't have two packages in one directory, so it'd have to live elsewhere anyways.

[briankassouf]

@jefferai go has an exception for _test packages. you can have package api and package api_test in the same directory. we do it in the database plugin tests

[jefferai]

Oh, neat! I didn't know that. That is very very useful.

[jefferai]

We already have another place where we invoke a postgres database using dockertest. Rather than put this in the API package it probably would be good to pick one of the two implementations and put it in a helper package that can be called by both.

Using a helpers/testing package to hold both testing common code and as a launchpoint for tests that can't easily be run in-package (e.g. ones using vault.TestCluster) might be the right approach.

[sethvargo]

I ran into issues trying to import helpers/testing because of a circular import cycle 🐸. Lots of things depend on the api package.

[jefferai]

What issues? There should be absolutely no issues importing both api and vault from a separate package.

[sethvargo]

From a separate package no, but I didn't know about being able to create a "test" package separately. I'm going to try that now. I would like to not have to spin up a separate cluster and just use the shared helpers 😄

[jefferai]

What problem did you have when creating a separate package?

[jefferai]

I think RenewTokenAsSelf may be less confusing

[jefferai]

Probably better to make it a time.Duration since this is a Go API.

[jefferai]

This (and stopCh) should be unbuffered. They should be closed when done/stopped, since a closed channel always returns immediately.

[jefferai]

OK, I see what you're doing with doneCh and it probably shouldn't be closed when done but probably also doesn't need to be buffered since you'll only read from it once.

[jefferai]

Rather than an empty struct, I think it would be useful to instead just push down the Auth info, or some way for the user to be able to introspect the new information without then having to perform a lookup on the lease or token. I haven't thought deeply about what information should be sent, but at a minimum maybe a struct with the new TTL?

[ryanuber]

Would it make sense to divide by 3 instead? I have done this before, thinking that if there is some issue during token renewal (Vault outage, network problems, etc.), we will get another chance at renewing prior to the hard expiration.

[jefferai]

There is some more detailed logic in VSI that Seth said he was planning to integrate.

[ryanuber]

@jefferai what is VSI? I am also not sure what you mean - is the simple division on the leaseDuration a hold-over while we add something better? 🤔

[jefferai]

@ryanuber An internal something, ping me separately if you want info.

What I mean is, we have code that has less naive logic. Seth said he was going to look at integrating it, but wanted an initial run through of the proposed API/behavior first.

[sethvargo]

I'd like to discuss pulling the functions in this file into a testing package which other Go tools that leverage Vault can use to stand up a Vault cluster and client in tests.

[ryanuber]

I'm +1 for this, I would definitely use it. Consul has a similar thing in its testutil package. I'm not married to the testutil name but a dedicated package with all of the test helpers exported (public) would be huge. For reference, the consul test server stuff is here.

[ryanuber]

I would also vote for tackling this in a separate PR though to minimize scope.

[jefferai]

Vault already has a bunch of these. I would suggest not adding such a PR right now as you're likely to duplicate work.

[ryanuber]

Since this is intended to be imported by other projects, should we do this? It's a bit odd because the importer may have already seeded from another source. I wonder if there is any real downside to using the predictable math/rand for this anyways.

[sethvargo]

I thought about this, and I really don't have a good answer. If consumers of the client library don't randomize, we completely lose the point of randomization, since they'll randomize on the same randomness. I'm not opposed to removing or keeping it. If there's a pre-defined pattern, I'd love to learn more about it.

[ryanuber]

Given the high security context of Vault I'd be interested in hearing from @jefferai and team on this one, since it will affect all importers and might cause unintended side-effects. For most practical cases though I agree it's probably fine to just re-seed like you are doing already. We should probably switch it to a UnixNano() to slim the chances of a common seed down.

[briankassouf]

You could store a Rand object in the Renewer struct so that you can independently seed each Renewer and avoid the side effect on import. https://golang.org/pkg/math/rand/#Rand

[sethvargo]

I just pushed that up. I like that API because we can use a different random per renewer, but people who really care can pass in their own randomizer.

[ryanuber]

Nice! I like this. 👍

[armon]

This will make the client not thread safe, since any call to RenewTokenAsSelf will change the token that is being used in another thread. Might be worth copying the underlying client to avoid the global modification.

[sethvargo]

Copying the client is really challenging due to the way we setup the underlying TLS and connection stuff 😦

[sethvargo]

Also note this is setting the token on the request, not the client (it's r.ClientToken, not c.Token). Since each call to NewRequest makes a new struct, I don't see this actually mutating state. Am I misinterpreting the way NewRequest works? It does not seem to modify the actual client.

[jefferai]

@sethvargo Yes, I believe that should be fine. FYI there is a tls.Config.Clone() method (https://golang.org/pkg/crypto/tls/#Config.Clone) but it's only semi-safe in that underlying pointers are copied as-is so you can't use that and then say modify the CA pools without affecting other clients.

[armon]

Probably want doneCh at least size 1 to not block

[sethvargo]

There's a comment in the Renew() function that states the function will block if nothing is reading from DoneCh. I think that's the correct behavior, since it seems silly to fire off the function and then not wait for a result. What do you think?

I updated the code to not block if we are closed.

[armon]

renewCh of size 5 seems sort of arbitrary, what happens when its full? Maybe the renew functions should guard on that and avoid blocking.

[sethvargo]

It's totally arbitrary, but seemed like a sane default. I suppose it could be a configurable. All the writes to the renewCh are non-blocking.

[armon]

I would make the lock private, unless the caller is ever going to Lock this explicitly

[armon]

@sethvargo I think I missed the part about the request! I would consider just buffering doneCh to simplify it.

[jefferai]

Any reason not to just use the client from cores[0].Client rather than all of this boilerplate?

[sethvargo]

I wasn't aware that existed. I was copying code from elsewhere in Vault that built their own client.

[jefferai]

It'd be nice to know when this happens, would using a non-nil logger help figure it out?

[sethvargo]

I can't reproduce it with Vault on master, but it occurred back in 0.5 occasionally. There are Consul Template issues about it. I put it in there because we can't guarantee someone using the client library is on the latest version of Vault.

[jefferai]

Is this meant to be renewer.RenewCh()? Same for DoneCh above.

[jefferai]

It feels like the previous function and this function could be pretty trivially combined with a couple of switch statements, instead of having mostly duplicate code.

[sethvargo]

I felt that way too, but I cannot think of a better way to write it.

This works, but it's tricky. There's a lot of boolean logic that's well tested, but challenging to follow.

// renewIt is a helper for renewing authentication.
func (r *Renewer) renewIt(renewable, auth bool, thing string) error {
    if !renewable || thing == "" {
        return ErrRenewerNotRenewable
    }

    client := r.client

    for {
        // Check if we are stopped.
        select {
        case <-r.stopCh:
            return nil
        default:
        }

        // Renew the auth.
        var renewal *api.Secret
        var err error
        if auth {
            renewal, err = client.Auth().Token().RenewTokenAsSelf(thing, 0)
            if err != nil {
                return err
            }
        } else {
            renewal, err = client.Sys().Renew(thing, 0)
            if err != nil {
                return err
            }
        }

        // Push a message that a renewal took place.
        select {
        case r.renewCh <- &RenewOutput{time.Now().UTC(), renewal}:
        default:
        }

        // Somehow, sometimes, this happens.
        if renewal == nil || (auth && renewal.Auth == nil) {
            return ErrRenewerNoSecretData
        }

        // Do nothing if we are not renewable
        if (auth && !renewal.Auth.Renewable) || !renewal.Renewable {
            return ErrRenewerNotRenewable
        }

        // Grab the lease duration and sleep duration - note that we grab the auth
        // lease duration, not the secret lease duration.
        var leaseDuration time.Duration
        if auth {
            leaseDuration = time.Duration(renewal.Auth.LeaseDuration) * time.Second
        } else {
            leaseDuration = time.Duration(renewal.LeaseDuration) * time.Second
        }
        sleepDuration := r.sleepDuration(leaseDuration)

        // If we are within grace, return now.
        if leaseDuration <= r.grace || sleepDuration <= r.grace {
            return nil
        }

        select {
        case <-r.stopCh:
            return nil
        case <-time.After(sleepDuration):
            continue
        }
    }
}

I tried writing something that accepted functions, but the fields you have to check for a successful auth are different from a successful renewal (which is one of the main reasons why I wrote this renewer).

[jefferai]

Made a couple of comments, looks good otherwise!

[chrishoffman]

Looks good. Just one small documentation issue.

[chrishoffman]

You should remove the "(in seconds)" since it can take any time.Duration. Also s/returring/returning/.