Skip to content

Conversation

@albertzaharovits
Copy link
Contributor

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 that 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 under the expected field.

Closes #59425

@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jul 13, 2020
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM.

I noticed that the realm info is added to authentication metadata since v7.4 (#45897). But you are right, the field name is changed in v7.7 with my PR (#51305). So API keys created before v7.7 will have empty string as realm name in audit logs. We could go extra mileage to make it work with v7.4. But I don't think it's all that necessary. This is one bwc that we can afford to not have. Thanks!

@ywangd
Copy link
Member

ywangd commented Jul 14, 2020

One more thing: Could you please re-enable the muted test by deleting these two lines from the yaml test as part of this PR?

@albertzaharovits
Copy link
Contributor Author

Could you please re-enable the muted test by deleting these two lines from the yaml test as part of this PR?

My reflex would've been to push a separate PR to toggle tests back after this PR is merged and backported, but your suggestion would work just as well.

@ywangd
Copy link
Member

ywangd commented Jul 14, 2020

Could you please re-enable the muted test by deleting these two lines from the yaml test as part of this PR?

My reflex would've been to push a separate PR to toggle tests back after this PR is merged and backported, but your suggestion would work just as well.

Sorry I forgot this PR is for master and the test is for 7.x. The one for master is not enabled because I was waiting for the 7.x backport. In this case, sorry please ignore my previous suggestion and I will take care of enabling the tests for both master and 7.x.

@ywangd
Copy link
Member

ywangd commented Jul 14, 2020

I see that you have already enabled the test. It works this way as well. Sorry I was confused initially.

@albertzaharovits albertzaharovits merged commit 4f8ff43 into elastic:master Jul 14, 2020
@albertzaharovits albertzaharovits deleted the audit_log_null_realm_for_api_keys branch July 14, 2020 11:10
albertzaharovits added a commit that referenced this pull request Jul 14, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>bug :Security/Audit X-Pack Audit logging Team:Security Meta label for security team v7.9.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] Auditing log fails for API key realm name during BWC with version prior to v7.5

4 participants