Expose API key name to the ingest pipeline #51305

ywangd · 2020-01-22T12:47:06Z

API key name is an important piece of information for traceability when
data are being ingested from multiple agents.
This becomes more relevant with the incoming support of required
pipelines (#46847)

Resolves: #49106

Working code, pending tests and check

elasticmachine · 2020-01-22T12:47:09Z

Pinging @elastic/es-security (:Security/Authentication)

ywangd · 2020-01-22T12:48:37Z

This PR is WIP. I'd like to have an early PR to increase visibility and easier discussion.

...security/src/main/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessor.java

tvernum · 2020-01-24T06:29:08Z

...security/src/main/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessor.java

+                    Object apiKeyName = authentication.getMetadata().get(ApiKeyService.API_KEY_NAME_KEY);
+                    if (apiKeyName != null) {
+                        userObject.put("api_key_name", apiKeyName);
+                    }


See my other comment about wanting the id etc as well.
I think this should be a nested object inside the user object.

user: { username: "...", api_key: { name: "...", id: "...", "realm": "..." } }

tvernum · 2020-01-24T06:30:28Z

@ywangd for WIP PRs, you can create them as a Draft PR when you open it in GitHub, which makes it more obvious to other observers, and also prevents them from being accidentally merged.

ywangd · 2020-01-29T08:58:08Z

@elasticmachine run elasticsearch-ci/default-distro

ywangd · 2020-01-29T12:33:37Z

x-pack/plugin/core/src/main/resources/security-index-template-7.json

@@ -194,6 +194,9 @@
            },
            "realm" : {
              "type" : "keyword"
+            },
+            "realm_type" : {


Add a new field for realm type as discussed

ywangd · 2020-01-31T01:48:34Z

This is now a formal PR ready for reivew. I removed the WIP label two days ago but somehow left the announcement in another PR ... Anyway, it's ready and please bring it on :)

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

tvernum · 2020-02-03T05:35:49Z

...security/src/main/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessor.java

@@ -85,6 +86,39 @@ public IngestDocument execute(IngestDocument ingestDocument) throws Exception {
                        userObject.put("metadata", user.metadata());
                    }
                    break;
+                case API_KEY:
+                    final HashMap<String, Object> apiKey = new HashMap<>();


Per #51454, if userObject has an api_key field already, and it's an object (Map) then we should add new fields to that object, not overwrite it.
Likewise for realm.

Updated for api_key and realm. Also added test to make sure they are not overwritten.

tvernum · 2020-02-03T05:43:28Z

docs/reference/ingest/processors/set-security-user.asciidoc

+through API key. It is an object containing two fields, `id` and `name`.
+When using API key authentication, the `realm` property refers to the realm
+from which the API key is created. It is also an object with two fields,
+`name` and `type`.


I think describing the realm property in the space of API Keys would lead some users to read it as only applying to API Keys. It would be better to say something like:

The `api_key` property is only applicable when the authentication is through API key. It is an object containing two fields, `id` and `name`. The `realm` property is also an object with two fields, `name` and `type`. When using API key authentication, the `realm` property refers to the realm from which the API key is created.

That said, I think we ought to actually expand this whole section and give examples, etc.
I'd be happy to see that in a followup PR though.

Updated. I would also prefer to have the example as a separate PR. The example code may need to be added as part of doc tests which require some work. Should I create an issue for it?

Should I create an issue for it?

Yes please.

Issue created: #51907

tvernum · 2020-02-03T05:49:03Z

...security/src/main/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessor.java

+                    break;
+                case REALM:
+                    final Object realmName = ApiKeyService.getCreatorRealmName(authentication);
+                    final Object realmType = ApiKeyService.getCreatorRealmType(authentication);


I think there's a mismatch here.

The API Key service populates the API key creator realm based on the looked-up-by.
But the getCreatorRealm* methods return the authenticating realm, not the looked up realm.

So, if I do curl -u elastic -H "es-security-runas-user: foo" PUT /index/_doc/1 the pipeline will record the realm as reserved

But if I do curl -u elastic -H "es-security-runas-user: foo" PUT /_security/api_key ... and then curl -H "Authorization: ApiKey .... PUT /index/_doc/1 it will have a realm of native.

It's not a big deal, but it's inconsistent. I think we want to always use the looked-up-by in the ingest processor, maybe with a flag of lookup: true or something like that if the authenticated realm was different.

You are right. I think the first case should report realm as native as well. This should be easily achieved once we augument the Authentication class with a new method to resolve the actual realm.

As for the lookup: true flag, do you mean add it as part of the SetSecurityUserProcessor configuration?

Updated for both name and type.

tvernum · 2020-02-03T05:54:25Z

What would people say about renaming existing fields, to better align to the ECS specification?

I think it would be good to introduce an ECS mode to this processor like we did in the user-agent processor and gradually move to ECS being the default and, eventually, the only mode.

@ywangd, would you open a new issue for that, and tag it both :Security and :Core/Features/Ingest so we can get some discussion on that?

Edit: I just read a bit further and realised that it might only be a single field that we need to change. I still think it's worth doing - the username can be a pretty important field in ECS - but it might mean that we don't prioritise it straight away.

ywangd · 2020-02-03T11:56:14Z

would you open a new issue for that, and tag it both :Security and :Core/Features/Ingest so we can get some discussion on that?

Issue created: #51799

ywangd · 2020-02-03T14:44:05Z

@elasticmachine run elasticsearch-ci/2

docs/reference/ingest/processors/set-security-user.asciidoc

albertzaharovits · 2020-02-04T21:51:39Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java

@@ -76,6 +76,10 @@ public RealmRef getLookedUpBy() {
        return lookedUpBy;
    }

+    public RealmRef getNominalRealm() {


Suggested change

public RealmRef getNominalRealm() {

public RealmRef getRealm() {

I think getRealm is a bit too general - I worry about someone just deciding to call getRealm() without thinking about the fact that there's 2 possible realms that might be in play and they need to be intentional about which one to use.

Maybe getSourceRealm()?

The better long term fix would actually be to make getLookedUpBy() always return a realm (that is, is uses this implementation) and then have a isUserLookup() if you need to know whether there was a specific (separate) lookup realm.

I agree getRealm is too general. I am OK with getSourceRealm and will go with it.

Is User#isRunAs functionally equivelent to the isUserLookup method you propose?

Logically equivalent yes, but it's implemented by looking at different fields, so I'd rather have both implementations so that the one in Authentication is specifically about whether lookupRealm is populated.

albertzaharovits

LGTM 👍

tvernum · 2020-02-05T05:43:40Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

@@ -886,7 +884,7 @@ public static String getCreatorRealmName(final Authentication authentication) {
        if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
            return (String) authentication.getMetadata().get(API_KEY_CREATOR_REALM_NAME);
        } else {
-            return authentication.getAuthenticatedBy().getName();
+            return authentication.getNominalRealm().getName();


I think we need to really consider whether this is the change we want.
It's a fairly fundamental change to the ApiKey service, and it affects how the manage_own_api_key privilege would work.
It might be right, I'd need to think about it, but if it is, then we might consider it a breaking change.

Here is my understanding:

In generaly, I feel the changed behaviour is correct, though you are right about it is a breaking change.

The above code is only used in TransportInvalidateApiKeyAction and TransportGetApiKeyAction. The else part will be called when the authentication is not of ApiKey auth type. Since ApiKey auth cannot perform runAs, this means theelse part will only makes a difference when userA runs as userB. In this case, the existing code will return userA realm, while the changes will return userB realm.

I can think of two reasons to support the changed behaviour:

When apikey is created, it records the runas user (if there is one) and runas/lookedUpBy realm. It feels correct that the same runas/lookedUpBy info should be checked when retrieve/invalidate the apikey.

When you run as someone else, all permissions and stuff should be checked/recorded as someone else.

I don't quite understand how manage_own_api_key work, the document says

All security-related operations on Elasticsearch API keys that are owned by the current authenticated user. The operations include creating new API keys, retrieving information about API keys, and invalidating API keys.

Does this mean you will not be able to create an API key for yourself unless you have this priviledge? If this is the case, I don't see how this is related to the change. Could you please elaborate on it?

Tim saved the day, I completely missed this 😞 .

@yang I think what you're describing is mostly correct.

Does this mean you will not be able to create an API key for yourself unless you have this priviledge?

Yes

If this is the case, I don't see how this is related to the change. Could you please elaborate on it?

Let's say you run-as and create an API key. The user that you run as only has the manage_own_api_key privilege. The creation would work. But then invalidation (and get), also using the run-as, won't work because the API Key Service would search the APi Key as incorrectly belonging to a user in the realm that authenticated the caller, whereas the API Key is created under the run-as user. (Note, I haven't actually tested this scenario).

In the interest of this PR, I would revert to the previous behavior in the TransportInvalidateApiKeyAction and TransportGetApiKeyAction , and, assuming you agree this is a bug, you should open a bug issue with your finding.

I tried to simulate the scenario you described with two users foo and bar, where foo is a superuser and bar only has manage_own_api_key for cluster and read for all indices. ES version 7.5.2

It behaves pretty much as you described when requests are made with owner=true. Given foo creates an ApiKey while runas bar, the request
curl -u foo:password -H 'es-security-runas-user: bar' 'localhost:9200/_security/api_key?id=keyId&owner=true' returns empty results.

While I do believe the correct behaviour of above request should be returning the api key, I also think it is better to have a separate PR for this as it is not related to the current issue.

PS: I also found if bar has only manage_own_api_key priviledge, it is not possible to get or delete the api key. I got error says action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [bar]. Once I add manage_api_key to bar, everything works.

I reverted the logic and created #51975 to track it. Thanks

Thank you Yang! But currently I don't think we want to set the authenticating realm in the case of the set security user processor, see my other comment: https://github.com/elastic/elasticsearch/pull/51305/files?file-filters%5B%5D=.java#r375923902

Agreed. Please see my other comment.

albertzaharovits · 2020-02-06T16:00:45Z

...security/src/main/java/org/elasticsearch/xpack/security/ingest/SetSecurityUserProcessor.java

+                    @SuppressWarnings("unchecked")
+                    final Map<String, Object> realmField =
+                        existingRealmField instanceof Map ? (Map<String, Object>) existingRealmField : new HashMap<>();
+                    final Object realmName = ApiKeyService.getCreatorRealmName(authentication);


In case you authenticate without an API Key, this gets you the authentication realm, although it should get the lookup realm, otherwise the set security user processor won't work as expected for run-as users.

This is a good point. No need to let the existing questionable logic propagate to here. I'll implement the logic locally and this also helps get rid of the other newly added getCreatorRealmType method. Thanks!

Updated Thanks again for catching this.

ywangd · 2020-02-06T23:46:48Z

@elasticmachine run elasticsearch-ci/bwc run elasticsearch-ci/default-distro run elasticsearch-ci/packaging-sample-matrix-windows

tvernum

LGTM.

The changes add more granularity for identiying the data ingestion user. The ingest pipeline can now be configure to record authentication realm and type. It can also record API key name and ID when one is in use. This improves traceability when data are being ingested from multiple agents and will become more relevant with the incoming support of required pipelines (elastic#46847) Resolves: elastic#49106

The changes add more granularity for identiying the data ingestion user. The ingest pipeline can now be configure to record authentication realm and type. It can also record API key name and ID when one is in use. This improves traceability when data are being ingested from multiple agents and will become more relevant with the incoming support of required pipelines (#46847) Resolves: #49106

The `Authentication` object that gets built following an API Key authentication contains the realm name of the owner user that created the key (which is audited), but the specific field used for storing it changed in #51305 . This PR makes it so that auditing tolerates an "unfound" realm name, so it doesn't throw an NPE, because the owner realm name is not found in the expected field. Closes #59425

WIP expose ApiKey name to ingest processor

88ff001

Working code, pending tests and check

ywangd added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.7.0 labels Jan 22, 2020

ywangd requested review from tvernum and albertzaharovits January 22, 2020 12:47

jrodewig added the WIP label Jan 22, 2020

tvernum reviewed Jan 24, 2020

View reviewed changes

ywangd added 4 commits January 29, 2020 14:54

Merge remote-tracking branch 'origin/master' into es-49106-apikey-name

d1592f8

WIP: expose more info about auth object

620554a

Use a more backwards compatible schema

f2ac3f6

Fix broken tests

d481a56

ywangd added 2 commits January 29, 2020 21:48

Tidy up for tests

29f33ce

Merge remote-tracking branch 'origin/master' into es-49106-apikey-name

b81c6e3

ywangd removed the WIP label Jan 29, 2020

ywangd requested a review from tvernum January 29, 2020 12:32

ywangd commented Jan 29, 2020

View reviewed changes

ywangd added 2 commits January 31, 2020 22:55

Merge branch 'master' into es-49106-apikey-name

deb7ca9

Update docs

4d508a3

tvernum reviewed Feb 3, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into es-49106-apikey-name

919edb1

Address feedback by Tim

9f7ca68

ywangd requested a review from tvernum February 3, 2020 13:22

albertzaharovits reviewed Feb 4, 2020

View reviewed changes

docs/reference/ingest/processors/set-security-user.asciidoc Show resolved Hide resolved

albertzaharovits reviewed Feb 4, 2020

View reviewed changes

docs/reference/ingest/processors/set-security-user.asciidoc Outdated Show resolved Hide resolved

albertzaharovits reviewed Feb 4, 2020

View reviewed changes

albertzaharovits approved these changes Feb 4, 2020

View reviewed changes

tvernum reviewed Feb 5, 2020

View reviewed changes

ywangd added 3 commits February 5, 2020 18:00

Address feedbacks

ac2efff

Merge remote-tracking branch 'origin/master' into es-49106-apikey-name

d488471

Revert the logic of getCreatorRealm for backwards compatibility

cc63be7

albertzaharovits reviewed Feb 6, 2020

View reviewed changes

Address feedback to set lookedup realm for non-api auth

e7af393

ywangd added 2 commits February 7, 2020 11:13

Merge remote-tracking branch 'origin/master' into es-49106-apikey-name

1e1d801

Merge remote-tracking branch 'origin/master' into es-49106-apikey-name

44a77d3

ywangd requested a review from tvernum February 8, 2020 14:19

tvernum approved these changes Feb 10, 2020

View reviewed changes

ywangd merged commit 5c9f795 into elastic:master Feb 10, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

albertzaharovits mentioned this pull request Jul 13, 2020

Audited API Key authentication might not have a realm name #59470

Merged

albertzaharovits mentioned this pull request Jul 14, 2020

BACKPORT 7x Fix auditing of API Key authn without the owner realm name (#59470) #59521

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	public RealmRef getNominalRealm() {
	public RealmRef getRealm() {

Expose API key name to the ingest pipeline #51305

Expose API key name to the ingest pipeline #51305

Conversation

ywangd commented Jan 22, 2020 • edited Loading

elasticmachine commented Jan 22, 2020

ywangd commented Jan 22, 2020

Choose a reason for hiding this comment

tvernum commented Jan 24, 2020

ywangd commented Jan 29, 2020

Choose a reason for hiding this comment

ywangd commented Jan 31, 2020

Choose a reason for hiding this comment

ywangd Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum commented Feb 3, 2020 • edited Loading

ywangd commented Feb 3, 2020 • edited Loading

ywangd commented Feb 3, 2020

Choose a reason for hiding this comment

tvernum Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Feb 6, 2020

tvernum left a comment

Choose a reason for hiding this comment

ywangd commented Jan 22, 2020 •

edited

Loading

ywangd Feb 3, 2020 •

edited

Loading

tvernum commented Feb 3, 2020 •

edited

Loading

ywangd commented Feb 3, 2020 •

edited

Loading

tvernum Feb 5, 2020 •

edited

Loading

tvernum Feb 10, 2020 •

edited

Loading

albertzaharovits Feb 6, 2020 •

edited

Loading