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

Add pki_delegatee to User metadata #44873

Conversation

albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented Jul 25, 2019

This adds the pki_delegatee field to the User#metadata map. The field contains the authentication of the proxy client (eg. kibana_system). The intention is for the es-admin to be able to write role mapping rules that distinguish between users authenticated directly or by delegation.

Please review #44561 first, as this needs tests that require the HLRest client introduced there.

Relates #34396

@albertzaharovits albertzaharovits added >enhancement WIP :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Jul 25, 2019
@albertzaharovits albertzaharovits self-assigned this Jul 25, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@tvernum
Copy link
Contributor

tvernum commented Jul 26, 2019

I haven't reviewed the code, but I don't think pki_delegatee is the right name for this field. I don't think anyone would understand what that means without the docs.

May something like pki_delegated_to / _by / _from / _via

@albertzaharovits
Copy link
Contributor Author

Thank you for the feedback Tim!
It turned out that having the Authentication object in the metadata does not really work anyway (only simple types like Numbers, Strings and Collections are permitted).

I have went with one of your options, _from, and added the following fields pki_delegated_from_user and pki_delegated_from_realm to the user metadata.

I have also added tests for it. The tests will conflict with #44561 but it doesn't matter, I'll fix the merge conflict on whatever PR gets merge the second one.

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

I did leave a suggestion about preventing an accidental null delegate.
I also prefer delegated_by rather than _from but that's a bikeshed, so it's up to you.

}

public X509AuthenticationToken(X509Certificate[] certificates, boolean isDelegated) {
public X509AuthenticationToken(X509Certificate[] certificates, Authentication delegateeAuthentication) {
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT of making this constructor private, and adding a factory method like:

public static X509AuthenticationToken delegatedToken(X509Certificate[] certificates, Authentication delegateeAuthentication) {
  Objects.requireNotNull(delegateeAuthentication);
 return new X509AuthenticationToken(certificates, delegateeAuthentication);
}

I worry about constructors where passing in a null is allowed, but completely changes the behaviour. You can't guard against null (because it's allowed) but if someone accidentally passes in a null value then they will get a behaviour that doesn't do what they want (and could end up being a security issue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea to make creating delegated tokens more explicit.

Copy link
Contributor

@bizybot bizybot left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you.

@albertzaharovits
Copy link
Contributor Author

I went with delegated_by_* instead of delegated_from_*.

@albertzaharovits albertzaharovits merged commit e599560 into elastic:proxied-pki Aug 5, 2019
@albertzaharovits albertzaharovits deleted the security-pki-delegation-delegatee-authn-info branch August 5, 2019 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants