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.

Sign up for GitHub

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

Ensure PKI's delegated_by_realm metadata respect run-as #91173

Merged
merged 3 commits into from
Nov 1, 2022
Merged

Ensure PKI's delegated_by_realm metadata respect run-as #91173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49852ba
Ensure PKI's delegated_by_realm metadata respect run-as
ywangd Oct 28, 2022
c483690
Update docs/changelog/91173.yaml
ywangd Oct 28, 2022
353302d
Merge branch 'main' into pki-realm-delegatee-realm
elasticmachine Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/91173.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 91173
summary: Ensure PKI's `delegated_by_realm` metadata respect run-as
area: Authentication
type: bug
issues: []
3 changes: 1 addition & 2 deletions ...ck/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/pki/PkiRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,7 @@ private void buildUser(X509AuthenticationToken token, String principal, ActionLi
"pki_delegated_by_user",
token.getDelegateeAuthentication().getEffectiveSubject().getUser().principal(),
"pki_delegated_by_realm",
// TODO: this should be the realm of effective subject
token.getDelegateeAuthentication().getAuthenticatingSubject().getRealm().getName()
token.getDelegateeAuthentication().getEffectiveSubject().getRealm().getName()
Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly checking my own understanding here: pki_delegated_by_realm might be used in role mappings which might in turn break if we change the realm here. Is this the case?

Since this is strictly a bug fix, it's fine to make a non-BWC change without deprecation and involving the committee etc. I'm wondering though if we should add a breaking: note to the changelog. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good topic to discuss. Thanks for raising it.

I checked briefly on the past PRs that have both >bug and >breaking and they are pretty rare. I think the breakage should be quite visible to users and users might intentional rely on the bug for the >breaking label to apply. The one PR with both >bug and >breaking that I raised in the past is to always require a name to be specified when creating API keys (#59836). This one had >breaking because we knew solution teams were relying on this behaviour (alerting was granting API keys without names).

For this particular case, I don't think the "breakage" will really noticeable. The PKI delegation is designed to be used by the Kibana system user so that end-user can authenticate with the PKI realm when Kibana is terminating TLS. For this breakage to manifest, the user's environment must have the kibana system user run-as a different user, then call the DelegatePKI API. In addition, there must a role mapping that relies on the kibana system user's realm which is either reserved or _service_account. Such a configuration would be really exotic if realistic at all.

Therefore, I'd prefer to go without the >breaking label. Since the chance for anyone running into it is so minuscule, I'd avoid unnecessary concerns for users who read the breaking release notes and think it might apply to them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent, I'm on board with skipping the >breaking label given that any impact is extremely unlikely. Thanks for giving more context on this.

);
} else {
metadata = Map.of("pki_dn", token.dn());
Expand Down
20 changes: 20 additions & 0 deletions ...ugin/security/src/test/java/org/elasticsearch/xpack/security/authc/pki/PkiRealmTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,26 @@ public void testAuthenticationDelegationSuccess() throws Exception {
assertThat(result.getValue().roles().length, is(0));
assertThat(result.getValue().metadata().get("pki_delegated_by_user"), is("mockup_delegate_username"));
assertThat(result.getValue().metadata().get("pki_delegated_by_realm"), is("mockup_delegate_realm"));

// Delegatee is run-as
final Authentication runAsAuthentication = AuthenticationTestHelper.builder().realm().build(true);
assertThat(runAsAuthentication.isRunAs(), is(true));
delegatedToken = X509AuthenticationToken.delegated(new X509Certificate[] { certificate }, runAsAuthentication);
realmWithDelegation.expireAll(); // clear the cache so the user is built again
result = authenticate(delegatedToken, realmWithDelegation);
assertThat(result.getStatus(), equalTo(AuthenticationResult.Status.SUCCESS));
assertThat(result.getValue(), is(notNullValue()));
assertThat(result.getValue().principal(), is("Elasticsearch Test Node"));
assertThat(result.getValue().roles(), is(notNullValue()));
assertThat(result.getValue().roles().length, is(0));
assertThat(
result.getValue().metadata().get("pki_delegated_by_user"),
is(runAsAuthentication.getEffectiveSubject().getUser().principal())
);
assertThat(
result.getValue().metadata().get("pki_delegated_by_realm"),
is(runAsAuthentication.getEffectiveSubject().getRealm().getName())
);
}

public void testAuthenticationDelegationFailure() throws Exception {
Expand Down