- Notifications
You must be signed in to change notification settings - Fork 25.6k
Adds certificate identity field to cross-cluster API keys #134604
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.
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
Conversation
| Hi @gmjehovich, I've created a changelog YAML for you. |
There was a problem hiding this 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.
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKey.java Show resolved Hide resolved
| @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.
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) { |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
| As a follow up to this PR, the specification for create/update/query/get should be updated here. Here is an example PR |
| 💚 CLA has been signed |
ℹ️ 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 overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
7af055d to 3523c09 Compare | Hi @gmjehovich, I've created a changelog YAML for you. |
…ssClusterAPIKey methods to account for new field
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
jfreden left a comment
There was a problem hiding this 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.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "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", |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 :)
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.
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: