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

auth/cert: Add metadata to identity-alias #14751

Merged
merged 3 commits into from Aug 23, 2022

Conversation

peterverraedt
Copy link
Contributor

@peterverraedt peterverraedt commented Mar 29, 2022

Add metadata to logical.Alias (the identity alias) too, in addition
to the metadata added to logical.Auth. This is analogous to the
behaviour of the ldap and approle auth providers.

CC: @pmmukh

Fixes: #14418

Signed-off-by: Peter Verraedt peter.verraedt@kuleuven.be

peterverraedt added a commit to peterverraedt/vault that referenced this pull request Mar 29, 2022
Signed-off-by: Peter Verraedt <peter.verraedt@kuleuven.be>
peterverraedt added a commit to peterverraedt/vault that referenced this pull request Mar 29, 2022
Signed-off-by: Peter Verraedt <peter.verraedt@kuleuven.be>
@hghaf099
Copy link
Contributor

hghaf099 commented Apr 6, 2022

Thank you for your contribution. There has been an update on the original GH issue. I am going to post a link here for reference. #14418 (comment)

Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @peterverraedt ,
Thanks for submitting this PR!

This change makes sense to us as the right path forward for enabling using certificates metadata in ACL templating. However, we're worried about storage concerns regarding this change, so we'd like for this to be an opt-in feature. One option is to add this to the role config, but it might also be possible that an extra config endpoint is needed.
Thanks again!

peterverraedt added a commit to peterverraedt/vault that referenced this pull request May 11, 2022
Signed-off-by: Peter Verraedt <peter.verraedt@kuleuven.be>
peterverraedt added a commit to peterverraedt/vault that referenced this pull request May 11, 2022
Signed-off-by: Peter Verraedt <peter.verraedt@kuleuven.be>
@peterverraedt
Copy link
Contributor Author

@HridoyRoy I've added a config value to the existing configuration endpoint of the auth method mount. As the identity aliases are linked to the auth mount as a whole rather than to the specific role, this seems to be the most logical place (either all identity aliases have some metadata, or none have metadata).

The amount of stored metadata is limited to these five fields

"cert_name": matched.Entry.Name,
"common_name": clientCerts[0].Subject.CommonName,
"serial_number": clientCerts[0].SerialNumber.String(),
"subject_key_id": certutil.GetHexFormatted(clientCerts[0].SubjectKeyId, ":"),
"authority_key_id": certutil.GetHexFormatted(clientCerts[0].AuthorityKeyId, ":"),
and what the user specifies as "allowed_metadata_extensions". The latter is specified on role level as it could easily differ depending on the trusted CAs.

@peterverraedt
Copy link
Contributor Author

@HridoyRoy What is the current status of this PR?

@pmmukh
Copy link
Contributor

pmmukh commented Jul 7, 2022

Hey @peterverraedt ! So we discovered a potential entity bloat bug when it comes to adding data to the alias metadata, and there's currently a PR open for it that we're in the process of reviewing and merging, once that is done the plan is to review either this or the other open PR fixing the issue.

@pmmukh
Copy link
Contributor

pmmukh commented Aug 3, 2022

Hi @peterverraedt ! I'm currently taking a look at this PR, and i'll drop a proper review shortly, but off the bat I had a couple asks, if they make sense to you.

  1. Since the goal is to use the certificate info for policy templating, could we potentially have a test that exercises that code path i.e store a secret in a path with say the cert common name, create a policy with that template, and check that a token with the policy works to read the secret ?
  2. I think this PR should also remove the place where Metadata is currently being set i.e in auth.Metadata, as we can't use it for templating since its in the token metadata, and so I don't think there's a practical way of using it. wdyt ?

@peterverraedt
Copy link
Contributor Author

Hi @pmmukh, back from holidays now.

  1. Yes, that is a good idea, I'll try to implement during the next days.
  2. I'm following what is done in the ldap and approle auth providers: https://github.com/hashicorp/vault/blob/main/builtin/credential/approle/path_login.go#L339 and https://github.com/hashicorp/vault/blob/main/builtin/credential/ldap/path_login.go#L90. The auth.Metadata is used in the metadata associated with the token that the user will receive after login, I am not sure whether it is merged with the alias.Metadata there? But I guess it could be removed, it is up to you.

peterverraedt and others added 2 commits August 16, 2022 17:42
Add the possibility to include certificate metadata in the created
logical.Alias (the identity alias), in addition to the metadata added
to logical.Auth. This is analogous to the behaviour of the ldap and
approle auth providers.

This possibility can be configured by the config endpoint of the
auth method mount and is disabled by default. We added the read
operation on this config endpoint as well.

Fixes: hashicorp#14418

Signed-off-by: Peter Verraedt <peter.verraedt@kuleuven.be>
Signed-off-by: Peter Verraedt <peter.verraedt@kuleuven.be>
Signed-off-by: Peter Verraedt <peter@verraedt.be>
@peterverraedt
Copy link
Contributor Author

Hi @pmmukh, I added a test case for testing whether usage of metadata in acl policies works.

I can also confirm for your second question that the metadata that is set in Auth.Metadata ends up as metadata associtated with the token, and while this does not relate at all to the acl policy templates, it still might be useful.

Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 lgtm!

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.

Move certificate extensions to entity alias metadata
5 participants