RFC: Filter cubbyhole responses based on cidr_block

[hashbrowncipher]

The cubbyhole is designed (in part) for entities to securely exchange secrets, by sharing a token that they use to PUT data in and GET data from the cubbyhole. This change enhances cubbyhole security by allowing the entity which writes into the cubbyhole to specify cidr blocks of clients that are allowed to read a given path. The idea is that the writer, knowing the IP address to which it will send its cubbyhole token, will choose cidr blocks such that only is intended recipient can then read the value out of the cubbyhole.

By doing this, we supplement the cubbyhole security model. Currently, an eavesdropper must use the cubbyhole before its intended recipient, within the lease duration, and prevent any monitoring from noticing that the cubbyhole has been used by an attacker. This change makes it so that an eavesdropper must also be able to spoof packets coming from the intended recipient in order to use the Cubbyhole.

The code in this diff is taken nearly verbatim from the app-id backend.

cc @EvanKrall

[jefferai]

I have to think more about whether this is functionality that we want to support here. I think the answer is "not yet": app-id added it originally because of problems with its flawed model of secret generation, but since then we've seen too much confusion from people wondering why it doesn't work (or being frustrated that they can't use it) because of a lack of support for X-Forwarded-For, X-Real-IP, PROXY, and friends, so I'd really like to see support for those be added before we expand use of CIDR restrictions.

One comment on the code itself: the CIDR should be validated at write time, so that if it's invalid the writer knows about it before passing the token to the reader.

[jefferai]

@hashbrowncipher We had a discussion about this internally.

We've already been looking into expanding the policy system to allow more controls, including CIDR access -- but we deferred CIDR access until we have a better story around trusting forwarding headers. So my initial thinking here was to defer this as well. However...

...If there's anywhere in Vault where the policy system is likely not the right way to go, it's probably cubbyhole, because it's designed to be controlled by a specific token. It's still subject to policy ACL checks, but usually the client won't be able to adjust these at-will. In addition, just like how even with a policy CIDR check mechanism we'd want to allow backends to implement further restrictions, a cubbyhole user may also want to further restrict access to a value before passing it along.

So, I think we'll accept this, but with some extra work on top of what's here. I'm guessing that's not really an issue since you marked this RFC, so I'm assuming you were looking for initial feedback:

• Validating the CIDR at write time
• Unit tests of the new functionality
• Updates to the backend documentation, both to describe the new functionality, and also to warn the user about the current lack of a mechanism for determining the authenticity of the IP address
• On write, a warning should be added onto the response with the same information about the lack of ability to determine authenticity of the IP address

I say "authenticity of the IP address" because part of what makes it more complicated than it would at first seem to simply parse X-Forwarded-For and friends is that you really need a whitelist mechanism to determine what source IPs to trust when you see those headers. Otherwise a client could simply add such headers, spoofing a "real" authorized IP. (If you're interested in adding this support, I'd be happy to provide guidance; part of why it doesn't exist yet is the Vault team hasn't had time and nobody from the community has stepped up to write it.)

[hashbrowncipher]

That's great!

I'll make the changes suggested by your initial feedback.

[hashbrowncipher]

Ok. Time for a discussion of IP addresses. I've looked over #291, and it sounds like the matter of what to do isn't well settled yet.

In my experience, correctly processing X-Forwarded-For headers is about as fun as unboxing a freshly Fedexed bobcat. I've seen it screwed up enough times at $EMPLOYER (using Apache's mod_rpaf and nginx's remoteip) that the idea of using it for security critical code scares me. In most of the cases I've seen, configuration errors arise when humans are tasked with keeping the IP whitelists up to date. When I have a choice, I have my load balancers decide which clients to trust by their TLS certificates, and skip IP whitelisting entirely.

I initially thought PROXY protocol might not be quite so bad. Typically PROXY protocol is used by a Layer 4 load balancer, which can totally provide value for an application like Vault. I think the best case scenario would be to not give Vault's private keys to the L4LBs, and have my load balancer TLS encapsulate the (already encrypted) TCP stream. The proxy protocol header would be inside the LB's TLS encapsulation, but outside the client->Vault TLS encapsulation. I tested to see if this would be possible with HAProxy, but unfortunately HAProxy's implementation of PROXY protocol puts the PROXY header outside the TLS encapsulation. This leaves us with no method other than IP whitelists to determine whose PROXY or X-Forwarded-For headers to trust.

You can tell I'm a fan of the end-to-end principle here. I think there's real value in having an uninterrupted TLS stream between the client and Vault. I'm not a fan of putting a load-balancer in front of Vault, but I can see why others might be.

So the second-best case scenario, IMO, would be a load-balancer operating in TCP mode and sending PROXY protocol headers up to Vault. Operators would have to specify a whitelist of IP addresses that are allowed to send PROXY headers to Vault, but (fortunately) requests would outright fail when the configuration is wrong, providing fast-feedback. In AWS, this could be implemented by putting the ELB instances in their own /28 subnets.

And now to add the warnings and documentation you requested.

[hashbrowncipher]

@jefferai I believe I've now addressed all of your initial feedback, thanks!

This is my first open source Go code, so your critique is heartily appreciated.

[jefferai]

@hashbrowncipher Gosh, it's like you've been sitting in on our own conversations around headers, PROXY, and why I'm always encouraging people on mailing lists to not use load balancers, or if they must, do it in TCP mode so at least the TLS connection is end-to-end.

But this is why I'm so lukewarm on cidr support. I have no problem in theory, but it's very easy for people to not understand why it doesn't work, and why it's not just a simple matter of parsing X-Forwarded-For. I'll take a look at this soon and may do some massaging of the exact message that goes back.

[jefferai]

I hate to do this but I have thought of another request:

Can you make cidr_block into cidr_blocks and take in a comma-separated list of CIDRs? I can totally see people that want to use this feature on a trusted network wanting to be able to authorize specific groups of machines by CIDR but not others.

[hashbrowncipher]

Sure.

[jefferai]

Hi @hashbrowncipher ,

Any update on this? Thanks!

[hashbrowncipher]

@jefferai last week was just awful for me, but I'll try to work on it this week. Sorry for the wait! Is there a deadline I'm trying to hit?

[jefferai]

Ish... We're gearing up for release (not this week) but could certainly address this in the release after if it doesn't make it in time!

[jefferai]

@hashbrowncipher For the moment I'm going to shift this to 0.6.1 but if it lands soon then I can shift it back. No rush!

[jefferai]

Can you move this string to a const in the package?

[jefferai]

Also a period at the end of the second sentence :-D

[jefferai]

@hashbrowncipher Are you still interested in getting this in?

[hashbrowncipher]

Not as much as I once was. Would you like to close it?

[jefferai]

The other way around -- I was going to suggest that we could get it merged in, but if your interest is dropping off, I'd be interested in knowing why. I don't want to end up with a feature without a use-case :-)

[hashbrowncipher]

The use-case I imagined it for was wholly superseded by the EC2 auth backend, once I figured out how feature-full the EC2 auth backend was. I've also now changed jobs, which doubly obviates the use-case.

[jefferai]

Oh hah. OK. I'll close this for now then.