Skip to content

Conversation

@gmjehovich
Copy link
Contributor

@gmjehovich gmjehovich commented Sep 12, 2025

This PR adds support for a certificate_identity field in cross-cluster API keys in order to enable linking these API keys to a certificate-based identity.

Overview of changes:

  • Add certificate_identity field to API key documents and requests
  • Support creation/updates of cross-cluster API keys with cert identity
  • Update serialization and response classes
  • Add mixed-cluster safety checks and Security Features
@gmjehovich gmjehovich requested a review from jfreden September 12, 2025 02:12
@gmjehovich gmjehovich self-assigned this Sep 12, 2025
@gmjehovich gmjehovich added >enhancement :Security/Security Security issues without another label Team:Security Meta label for security team v9.2.0 labels Sep 12, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @gmjehovich, I've created a changelog YAML for you.

Copy link
Contributor

@jfreden jfreden left a comment

Choose a reason for hiding this comment

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

Good job! I took an initial look and left some comments related to the mixed cluster/upgrade scenarios.

Looks like there are some compilation issues now and you need the new field in TransportUpdateCrossClusterApiKeyAction and BulkUpdateApiKeyRequest (pass null since adding the same identity to several api keys is not a use case yet).

An upgrade test in ApiKeyBackwardsCompatibilityIT would be nice, to make sure that the upgrade path works as expected. QueryableBuiltInRolesUpgradeIT uses node features here you might be able to copy/break out some of that logic for reuse.

Since we're touching the API Key interfaces it would be good to add the test-update-serverless label to make sure the changes to the constructors are compatible with serverless.

In the PKI Realm when the username_pattern is configured, we compile the pattern as part of the setting validation. I wonder if we should do that here too? See PKIRealm here.

@Nullable final Map<String, Object> metadata,
@Nullable final TimeValue expiration
@Nullable final TimeValue expiration,
@Nullable final String certificateIdentity
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is no other way to add certificateIndentity to the update request because of how the ApiKeyService is coupled with BaseBulkUpdateApiKeyRequest. It's out of scope for this PR to fix that coupling I think, but it gets a little awkward to have to include the new field in bulk just to be able to use the update path.

}, listener::onFailure))
)
);
} catch (MapperParsingException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When creating or updating an API Key document in the security index, the prepareIndexIfNeededThenExecute is used. That method makes sure that the minimum mapping version that's supported by the cluster is applied to the security index, see this.

If node features are used to guard against writing the new field, that means that we can guarantee that all nodes have a mapping that supports that node feature. So in other words, if the cluster has the new feature, you can assume the mapping is up to date so this exception handling is not needed.


private static final Logger logger = LogManager.getLogger(ApiKeyService.class);
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ApiKeyService.class);
private final SecurityFeatures securityFeatures = new SecurityFeatures();
Copy link
Contributor

Choose a reason for hiding this comment

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

SecurityFeatures is just holding the NodeFeature records that the security plugin manages. To check if a node has a feature, an instance of the FeatureService is needed.

Before writing the new field, I think all that's needed is a check using clusterHasFeature(CERTIFICATE_IDENTITY_FIELD_FEATURE) and throw an error if the feature is not supported yet.

@jfreden
Copy link
Contributor

jfreden commented Sep 12, 2025

As a follow up to this PR, the specification for create/update/query/get should be updated here. Here is an example PR

@cla-checker-service
Copy link

cla-checker-service bot commented Sep 18, 2025

💚 CLA has been signed

@github-actions
Copy link
Contributor

ℹ️ Important: Docs version tagging

👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version.

We use applies_to tags to mark version-specific features and changes.

Expand for a quick overview

When to use applies_to tags:

✅ At the page level to indicate which products/deployments the content applies to (mandatory)
✅ When features change state (e.g. preview, ga) in a specific version
✅ When availability differs across deployments and environments

What NOT to do:

❌ Don't remove or replace information that applies to an older version
❌ Don't add new information that applies to a specific version without an applies_to tag
❌ Don't forget that applies_to tags can be used at the page, section, and inline level

🤔 Need help?

@gmjehovich gmjehovich reopened this Sep 18, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @gmjehovich, I've created a changelog YAML for you.

public void testCertificateIdentityBackwardsCompatibility() throws Exception {
assumeTrue(
"certificate identity backwards compatibility only relevant when upgrading from pre-9.2.0",
UPGRADE_FROM_VERSION.before(Version.V_9_2_0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the correct version, or is this feature going to be added in 9.3.0?

Copy link
Contributor

Choose a reason for hiding this comment

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

The PR is labeled for 9.2 that is feature freeze on September 30th so depending on when it's merged it will be 9.2 or 9.3.

Copy link
Contributor

@jfreden jfreden left a comment

Choose a reason for hiding this comment

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

LGTM!

Great job on this! I only have a couple of non-blocking comments. Let me know if you want to discuss more in detail.

Pattern.compile(value);
} catch (PatternSyntaxException e) {
throw new IllegalArgumentException(
"Invalid certificate_identity format: [" + value + "]. Must be a valid regex name pattern.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Invalid certificate_identity format: [" + value + "]. Must be a valid regex name pattern.",
"Invalid certificate_identity format: [" + value + "]. Must be a valid regex.",
if (roleDescriptors == null && metadata == null && certificateIdentity == null) {
validationException = addValidationError(
"must update either [access] or [metadata] for cross-cluster API keys",
"must update [access] or [metadata] or [certificate_identity] for cross-cluster API keys",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is still unchanged.

PARSER.declareObject(constructorArg(), CrossClusterApiKeyRoleDescriptorBuilder.PARSER, new ParseField("access"));
PARSER.declareString(optionalConstructorArg(), new ParseField("expiration"));
PARSER.declareObject(optionalConstructorArg(), (p, c) -> p.map(), new ParseField("metadata"));
PARSER.declareField(
Copy link
Contributor

Choose a reason for hiding this comment

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

In the create flow there is no use case for explicit null vs implicit. I wonder if just using String for create would simplify getCertificateIdentityFromCreateRequest slightly. You still want to do the validation so you might have to extract to a static method if you decide to implement this. I'll leave it up to you.

assertThat(e2.getMessage(), containsString("owner user role descriptors must not include restriction"));
}

public void testMaybeBuildUpdatedDocumentCertificateIdentityHandling() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

}

@SuppressWarnings("unchecked")
private Map<Boolean, RestClient> getRestClientByCertificateIdentityCapability() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any updates to RolesBackwardsCompatibilityIT or AbstractUpgradeTestCase was it not possible to put that in shared code? No worries if it doesn't make sense :)

@gmjehovich gmjehovich merged commit ce18c5b into elastic:main Oct 3, 2025
40 checks passed
elasticsearchmachine pushed a commit that referenced this pull request Oct 10, 2025
This PR is a follow up to #134137 and #134893. It adds serialization and verification of the new header: - The signing configurations are now used to generate a signature that's passed as a header for cross cluster api keys. - The signature headers are deserializaed and validated on the server side and auth fails if the validation fails. This PR does not use the certificate identity that was added in #134604 to verify that the identity in the passed leaf certificate belongs to the signed cross cluster API key by matching it against the API key certificate identity pattern. That will be done in a follow up PR to keep the scope of this PR manageable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team test-update-serverless v9.3.0

3 participants