Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support token's metadata in template #10682

Closed
wants to merge 1 commit into from
Closed

Conversation

UXabre
Copy link

@UXabre UXabre commented Jan 11, 2021

use-case
So, I've implemented this as a way to have very dynamic policy creation possibilities without having to know either the entity, alias or group up front. This way I can define a policy once and use it for serveral auth backends. In my case this needed as I have users which are allowed to create (oidc/jwt) auth backends but with a limited set of possible policies that the roles attached can apply.
Currently, however, I'd also need to create a unique policy per auth backend to limit which resources can be accessed; something that IMO is a huge no-no as this would allow a corrupted user to give access to the entire vault (via this auth backend); this helps to solve the security principle of least privilege for this use-case. (reference: #9763)
I read that this might also solve an issue with approle, in which the entity is updated with different authorizations by two different apps; which could provide deeper access into vault; since the token is really passed each time a call is made, there is no discussion on the metadata that was set for that token. (reference case: #11798)

usage
Currently only metadata is supported via this syntax:

-- interpolate all metadata at once
{{token.metadata}}
-- interpolate specific metadata for a token
{{token.metadata.<key>}}

@hashicorp-cla
Copy link

hashicorp-cla commented Jan 11, 2021

CLA assistant check
All committers have signed the CLA.

@UXabre
Copy link
Author

UXabre commented Jan 11, 2021

So, this starts of painful as I'm not a Go Guru at all :-( (still learning),
It seems the error is generally the same:

vault/policy.go:289:5: unknown field 'Token' in struct literal of type identitytpl.PopulateStringInput

This field is, however, declared and exported, also, I'm not able to reproduce this error in my development machine using:

make dev-ui

What am I missing here?

@UXabre UXabre changed the title Support token's metadata in template #10460 Support token's metadata in template Feb 4, 2021
@ncabatoff
Copy link
Collaborator

Hi @UXabre,

You need to run go mod vendor. Changes under sdk and api are only visible to the build the process once they make it to the vendor/ dir.

This may not be an issue for you locally as our go.mod has replace directives and your Go version / local env vars may not enforce vendoring like our CI does.

@UXabre
Copy link
Author

UXabre commented Feb 9, 2021

Hi @ncabatoff, thanks for your input, I didn't realize that. However, I'm unsure how I can make it to the vendor dir, as it seems this struct is defined inside the SDK files itself, I did a search in the codebase but didn't find any connection in the vendor folder.

If I could find it, I'd be able to go to that library and make that adjustment, but there seems to be a missing link in my head on this topic

EDIT: Nevermind, I did some research and I think I figured it out :-) adapting PR to match what I've learned
EDIT2: Seems to be working! I feel ashamed that I overlooked this the first time...

@vercel vercel bot temporarily deployed to Preview – vault February 10, 2021 12:53 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 10, 2021 12:53 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 10, 2021 12:55 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 10, 2021 12:55 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 10, 2021 13:09 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 10, 2021 13:09 Inactive
@UXabre
Copy link
Author

UXabre commented Mar 16, 2021

If anybody could have a look at this one? Seems that the current build errors are not related:

#3 ERROR: failed to copy: httpReaderSeeker: failed open: could not fetch content descriptor sha256:de85b2f3a3e8a2f7fe48e8e84a65f6fdd5cd5183afa6412fff9caa6871649c44 (application/vnd.docker.distribution.manifest.list.v2+json) from remote: not found

Thanks in advance!

@UXabre
Copy link
Author

UXabre commented Oct 18, 2021

So, I've finally found some time to continue my work on this subject.

  • Rebased on top of main
  • Added changelog entry
  • Added docs
  • Added testing

I guess this covers most of the subjects that need to be tackled but any input is ofcourse more than welcome

@UXabre
Copy link
Author

UXabre commented Oct 28, 2021

Hi, Thanks for reviewing. Is there anything from my end I need to do to get this merged? I can imagine an old PR like this being easily overlooked? Or is there something expected from my end except being patient? :-)

@heatherezell
Copy link
Contributor

Hi @UXabre! Loann reviewed the docs portion - I'd like to have an engineer review the code. I'll reach out and get someone to take a peek. Thanks!

@UXabre UXabre force-pushed the master branch 2 times, most recently from 2861e94 to e9c3ed0 Compare November 24, 2021 12:46
@UXabre
Copy link
Author

UXabre commented Nov 25, 2021

Fixed merge conflict and rebased on master (so as up to date as possible)

@UXabre
Copy link
Author

UXabre commented Dec 22, 2021

Hi @hsimon-hashicorp, sorry for this friendly reminder but... seeing this one merged would be an awesome early christmas gift 😄

@heatherezell
Copy link
Contributor

@UXabre Sorry we couldn't get it to you for Christmas! (Supply chain issues, yeah, that's it...) I'll poke an engineer again for review. :)

@ncabatoff
Copy link
Collaborator

Hi @UXabre,

Thank you for the contribution. Unfortunately I'm afraid that we have decided to refuse it. We view this change as dangerous, since a user can create child tokens with whatever arbitrary metadata they like. This means policy templating using token metadata is vulnerable to any attacker who's aware of what policies are in use. I expect that you could come up with safeguards in your environment to protect against this (e.g. not granting access to the create token endpoints), but we as vault devs have to be vigilant against introducing features that allow for insecure by default behaviour.

The right approach is to use identity metadata. I realize that doesn't address your use case for creating policies "without having to know either the entity, alias or group up front." I would like to find a way to allow that, there have been internal requests along the same lines. Sadly we haven't yet come up with a solution that allows this without introducing new problems we're unwilling to accept.

Do you want to provide more background on your higher level use case? Why are you allowing users to create auth backends?

@UXabre
Copy link
Author

UXabre commented Jan 6, 2022

Hi @ncabatoff , thanks for your elaborate answer on this subject. I actually didn't realize I could create subtokens (and thus indeed expose custom metadata); that would indeed reduce the security level to "security by obscurity" and one who knows what to target could do just about anything. I wholly agree, this is indeed not the way to go.

As for my higher level use-case, I'll try to explain it to the best of my ability:

  1. In our system, we have superusers; godlike creatures that have access to right about everything (kinda like root in linux). These superusers can create administrator accounts.
  2. These administrator accounts don't have much power in or system, only really the power to create "companies". And a administrator can also, in our system, only manage "companies" created in their name
  3. A "company", in our case, gets a keycloak realm (which is basically the same as providing a separate IDP) - we do this as a company can manage users within this realm, configure specifics like logo's etc.
  4. It is the administrators that I'd like to give just and only the power to create an auth backend in vault and attach a "company" policy to this auth backend (this is in turn protected by field level security in another policy). The policy can use the variable interpolation to restrict a user within a company to be restricted to secrets that are shared among other users that belong to the company. However, some secrets are also needed to be accessible system-wide.

I like the concept that the OIDC auth backend provides for copying data from the JWT token to an entity but that seems to require me to create policies as I'd need to create a policy per auth backend.
And what I want to prevent, is that an administrator has the ability to create a policy; as an malicious administrator could then achieve root access to vault.

But wait, there's more: currently there is the "identity.entity.metadata." to be interpolated, but OIDC/JWT does not copy anything from the JWT-token there, but only to an alias. Why is this only copied to alias an not to identity.entity? For me that would be equally great but I'm guessing there's a reason that I'm overlooking why this is not already implemented for the JWT/OIDC backend.

@ncabatoff
Copy link
Collaborator

Ok, that makes sense, though I don't have much in the way of concrete suggestions yet. If you were on enterprise I'd say to use namespaces.

But wait, there's more: currently there is the "identity.entity.metadata." to be interpolated, but OIDC/JWT does not copy anything from the JWT-token there, but only to an alias. Why is this only copied to alias an not to identity.entity? For me that would be equally great but I'm guessing there's a reason that I'm overlooking why this is not already implemented for the JWT/OIDC backend.

Well, alias metadata is alias-specific. If an entity can login using multiple auth methods, and each method generates its own metadata, and each login to a different auth method means overwriting the entity's metadata with the auth-specific alias metadata, that's going to be (1) expensive in terms of I/O and (2) bewildering for the user. Suppose I login using ldap auth, do some work with that token, and then before the token expires I do another login as the same entity via oidc auth. If that second login results in overwriting my entity metadata, suddenly the first token may stop working for policy paths that were interpolating that entity metadata, since now it contains the metadata from the oidc auth instead of the ldap auth.

@UXabre
Copy link
Author

UXabre commented Jan 6, 2022

Thanks for this elaboration, reading this it makes sense.
I'll have to ponder a bit on this subject as well, knowing all this now.
I'll need to further look at the current code in-depth to check if it is possible to figure out but, what if we limited token metadata interpolation to just parent tokens and "ignore" metadata from child tokens (not sure what to do with orphan tokens and what a token generated from an auth backend is deemed...is it an orphan?)?

Also, not sure what the use-case is of storing metadata in a child-token, I mean, in terms of authorization (as I would deem a child token "less" secure than a "parent"); as we established before, at least its metadata should not be trusted.

Sorry if my thinking is narrow on this subject; I'm very much into my own small problem bubble and would tend to forget the bigger picture.

@ncabatoff
Copy link
Collaborator

what if we limited token metadata interpolation to just parent tokens and "ignore" metadata from child tokens

I don't think we want to treat child tokens any different from tokens without any parent. Depending on the deployment pattern in use, child tokens may be all that anyone ever uses, e.g. if an orchestrator is creating and distributing tokens to applications.

not sure what to do with orphan tokens and what a token generated from an auth backend is deemed...is it an orphan

I guess you could call a token produced by a login an orphan, insofar as it has no parent, but we usually only speak of orphan tokens in the context of explicitly created tokens.

Also, not sure what the use-case is of storing metadata in a child-token, I mean, in terms of authorization (as I would deem a child token "less" secure than a "parent"); as we established before, at least its metadata should not be trusted.

I'm not sure how universal this perspective is at HashiCorp, but on the Core team we're starting to view token metadata as a mistake. We're not planning on getting rid of it, but we are steering people towards using identity metadata instead for most purposes.

Sorry if my thinking is narrow on this subject; I'm very much into my own small problem bubble and would tend to forget the bigger picture.

I struggle to keep the bigger picture in mind and it's my job! Thank you for recognizing that the bigger picture exists and being understanding, it's very much appreciated.

I'm going to close this PR: in hindsight we should've had this discussion in an issue instead, where others are more likely to encounter it. Please feel free to tag me if you'd like to continue the discussion in an existing or new issue.

@celesteking
Copy link

This is a terrible misarchitecture on Hashicorp side. You should've named user supplied cruft not as metadata but as notes, and of course, you should be putting authentication info into special map attached to a token and accessible in templates, call it authinfo, and never let user edit it. The very idea of letting the user shove his data in a token is redundant as you've already got a KV cubbyhole store for that.
You're forcing us into entities, but that's unmanageable. Imagine 1k hosts having to auth against vault -- I have to create 1k entities and 1k aliases, 2k steps and all for what? We already have roles in auth methods that set necessary policies, why create another layer of useless abstraction? We don't know who's going to auth against a TLS auth specifically, but we know the right role will catch them and do the required.
Why complicate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants