Add REST handler for PKI delegation #44561
Add REST handler for PKI delegation #44561albertzaharovits merged 33 commits intoelastic:proxied-pkifrom
Conversation
|
Pinging @elastic/es-security |
0f361c9 to
9758afb
Compare
| from({ zipTree(configurations.restSpec.singleFile) }) { | ||
| include 'rest-api-spec/api/**' | ||
| } | ||
| from(project(':client:rest-high-level').file('src/test/resources')) |
There was a problem hiding this comment.
I have added a certificate chain ( testClient.crt, testIntermediateCA.crt and testRootCA.crt files) as resources to these two projects.
| } | ||
|
|
||
| private X509Certificate[] certificates; | ||
| private List<X509Certificate> certificateChain; |
There was a problem hiding this comment.
made it a list instead of an array.
| * The request object for {@code TransportDelegatePkiAuthenticationAction} containing the certificate chain for the target subject | ||
| * distinguished name to be granted an access token. | ||
| */ | ||
| public final class DelegatePkiAuthenticationRequest extends ActionRequest implements ToXContentObject { |
There was a problem hiding this comment.
Implements ToXContentObject because it is easier on tests (instead of parsing it as a map, and iterating over an array).
|
Please review #44767 before this one :) |
|
@elasticmachine run elasticsearch-ci/2 (Failure is legit but is handled in #44767) |
bizybot
left a comment
There was a problem hiding this comment.
Overall LGTM.
It would have been better if we took HLRC changes in separate PR, just something that we can do in the future. Thank you.
| private static final ParseField X509_CERTIFICATE_CHAIN_FIELD = new ParseField("x509_certificate_chain"); | ||
|
|
||
| public static final ConstructingObjectParser<DelegatePkiAuthenticationRequest, Void> PARSER = new ConstructingObjectParser<>( | ||
| "delegate_pki_request", true, a -> { |
There was a problem hiding this comment.
we are parsing request here which accepts unknown field. We should be generally not accepting unknown fields for request, is there a reason to accept unknown fields?
There was a problem hiding this comment.
I was not aware that we have reached a guideline on this matter. My impression is we should always ignore unknown fields, unless there is a problem with it. Maybe @hub-cap is up to date with the recommendation, and can help us decide.
In the interest of time, I have pushed the changes as suggested, because I do not see the reason to ignore unknown fields, in the absence of a general imposition.
There was a problem hiding this comment.
My understanding was for the request we do not ignore unknown fields but for the response(ex. in HLRC) we would ignore unknown fields.
|
|
||
| @Override | ||
| protected boolean supportsUnknownFields() { | ||
| return true; |
There was a problem hiding this comment.
as per my comment on accepting unknown fields in the request, if it need not be true then this would become false.
….asciidoc Co-Authored-By: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com>
|
@elasticmachine run elasticsearch-ci/1 |
bizybot
left a comment
There was a problem hiding this comment.
LGTM, other than nits. Thank you.
|
|
||
| public RestDelegatePkiAuthenticationAction(Settings settings, RestController controller, XPackLicenseState xPackLicenseState) { | ||
| super(settings, xPackLicenseState); | ||
| controller.registerHandler(POST, "/_security/delegate_pki", this); |
There was a problem hiding this comment.
Is there any background to your choice of URL here?
It seems a bit weird to me, I'd expect something with a bit more structure to it so that we have space to add additional endpoints in the future.
There was a problem hiding this comment.
No serious thought, I admit. But I don't think it can be rooted under existing namespaces.
Rather than trying to anticipate future similar endpoints that this can be namespaced with, we can deprecate this endpoint path and add a new path once we know the namespace. What do you think? I don't really have a preference for this one either.
There was a problem hiding this comment.
Fair enough. I think /_security/pki/delegate would be neater, but you're right that it's likely that Kibana would be the only major client of this API so deprecate & replace could be a simple solution when we need it.
|
Thanks for the detailed review! |
|
|
didn't update the UI after my last push. I hope it comes to its senses.
Adds a REST action for the
TransportDelegatePkiAuthenticationAction,EDITED:
the corresponding HL Rest client methods (with tests) and a certificate chain for tests.
EDITED 2:
Please review #44767 before this one.
Follow-up of #44106
Relates #34396