-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Add REST handler for PKI delegation #44561
Add REST handler for PKI delegation #44561
Conversation
Pinging @elastic/es-security |
0f361c9
to
9758afb
Compare
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, Thank you.
...n/java/org/elasticsearch/xpack/security/action/TransportDelegatePkiAuthenticationAction.java
Outdated
Show resolved
Hide resolved
@@ -83,6 +83,7 @@ processTestResources { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a certificate chain ( testClient.crt
, testIntermediateCA.crt
and testRootCA.crt
files) as resources to these two projects.
client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java
Show resolved
Hide resolved
|
||
private X509Certificate[] certificates; | ||
private List<X509Certificate> certificateChain; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
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.
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.
client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java
Show resolved
Hide resolved
client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java
Show resolved
Hide resolved
...n/java/org/elasticsearch/xpack/security/action/TransportDelegatePkiAuthenticationAction.java
Show resolved
Hide resolved
docs/java-rest/high-level/security/delegate-pki-authentication.asciidoc
Outdated
Show resolved
Hide resolved
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per my comment on accepting unknown fields in the request, if it need not be true then this would become false.
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.
Subsumed by #44561 (comment)
….asciidoc Co-Authored-By: Yogesh Gaikwad <902768+bizybot@users.noreply.github.com>
@elasticmachine run elasticsearch-ci/1 |
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, other than nits. Thank you.
...main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiAuthenticationRequest.java
Outdated
Show resolved
Hide resolved
...curity/src/main/java/org/elasticsearch/xpack/security/authc/pki/X509AuthenticationToken.java
Show resolved
Hide resolved
...-level/src/main/java/org/elasticsearch/client/security/DelegatePkiAuthenticationRequest.java
Outdated
Show resolved
Hide resolved
...high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java
Outdated
Show resolved
Hide resolved
...-high-level/src/test/resources/org/elasticsearch/client/security/delegate_pki/testClient.csr
Outdated
Show resolved
Hide resolved
...main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiAuthenticationRequest.java
Outdated
Show resolved
Hide resolved
...main/java/org/elasticsearch/xpack/core/security/action/DelegatePkiAuthenticationRequest.java
Outdated
Show resolved
Hide resolved
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
...n/java/org/elasticsearch/xpack/security/rest/action/RestDelegatePkiAuthenticationAction.java
Outdated
Show resolved
Hide resolved
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