Ensure PKI's delegated_by_realm metadata respect run-as #91173
Conversation
When delegated PKI authentication is used, the delegatee's realm name is added as a metadata field. This realm name should be the effective subject's realm instead of that of the authenticating subject. This PR ensures this is the case.
|
Pinging @elastic/es-security (Team:Security) |
|
Hi @ywangd, I've created a changelog YAML for you. |
| @@ -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() | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Excellent, I'm on board with skipping the >breaking label given that any impact is extremely unlikely. Thanks for giving more context on this.
|
@elasticmachine update branch |
When delegated PKI authentication is used, the delegatee's realm name is added as a metadata field. This realm name should be the effective subject's realm instead of that of the authenticating subject. This PR ensures this is the case.
* main: (1300 commits) update c2id/c2id-server-demo docker image to support ARM (elastic#91144) Allow legacy index settings on legacy indices (elastic#90264) Skip prevoting if single-node discovery (elastic#91255) Chunked encoding for snapshot status API (elastic#90801) Allow different decay values depending on the score function (elastic#91195) Fix handling indexed envelopes crossing the dateline in mvt API (elastic#91105) Ensure cleanups succeed in JoinValidationService (elastic#90601) Add overflow behaviour test for RecyclerBytesStreamOutput (elastic#90638) More actionable error for ancient indices (elastic#91243) Fix APM configuration file delete (elastic#91058) Clean up handshake test class (elastic#90966) Improve H3#hexRing logic and add H3#areNeighborCells method (elastic#91140) Restrict direct use of `ApplicationPrivilege` constructor (elastic#91176) [ML] Allow NLP truncate option to be updated when span is set (elastic#91224) Support multi-intersection for FieldPermissions (elastic#91169) Support intersecting multi-sets of queries with DocumentPermissions (elastic#91151) Ensure TermsEnum action works correctly with API keys (elastic#91170) Fix NPE in auditing authenticationSuccess for non-existing run-as user (elastic#91171) Ensure PKI's delegated_by_realm metadata respect run-as (elastic#91173) [ML] Update API documentation for anomaly score explanation (elastic#91177) ... # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportRollupIndexerAction.java # x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java
When delegated PKI authentication is used, the delegatee's realm name is added as a metadata field. This realm name should be the effective subject's realm instead of that of the authenticating subject. This PR ensures this is the case.
Relates: #91104 (comment)