Adds certificate identity field to cross-cluster API keys #134604
Conversation
…ssClusterAPIKey methods to account for new field
gmjehovich
added
>enhancement
:Security/Security
|
Hi @gmjehovich, I've created a changelog YAML for you. |
There was a problem hiding this comment.
Good job! I took an initial look and left some comments related to the mixed cluster/upgrade scenarios.
Looks like there are some complication 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.
| @@ -121,6 +121,8 @@ public VersionId<?> versionNumber() { | |||
| private final List<RoleDescriptor> roleDescriptors; | |||
There was a problem hiding this comment.
nit: certificateIdentity is missing in toString()
| @@ -26,9 +26,10 @@ public BaseBulkUpdateApiKeyRequest( | |||
| final List<String> ids, | |||
| @Nullable final List<RoleDescriptor> roleDescriptors, | |||
| @Nullable final Map<String, Object> metadata, | |||
| @Nullable final TimeValue expiration | |||
| @Nullable final TimeValue expiration, | |||
| @Nullable final String certificateIdentity | |||
There was a problem hiding this comment.
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.
| @@ -606,6 +612,16 @@ private void createApiKeyAndIndexIt( | |||
| }, listener::onFailure)) | |||
| ) | |||
| ); | |||
| } catch (MapperParsingException e) { | |||
There was a problem hiding this comment.
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.
| @@ -158,6 +161,7 @@ public class ApiKeyService implements Closeable { | |||
|
|
|||
| private static final Logger logger = LogManager.getLogger(ApiKeyService.class); | |||
| private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ApiKeyService.class); | |||
| private final SecurityFeatures securityFeatures = new SecurityFeatures(); | |||
There was a problem hiding this comment.
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.
|
As a follow up to this PR, the specification for create/update/query/get should be updated here. Here is an example PR |
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:
TODO:
This PR is currently a draft for early feedback.