Skip to content
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

HLRC: Get SSL Certificates API #34135

Merged
merged 13 commits into from
Oct 15, 2018
Merged

Conversation

jkakavas
Copy link
Member

This change adds support for the get SSL certificate API to the high
level rest client.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security


import java.io.IOException;

public class EmptyBodyRequest implements Validatable, ToXContentObject {
Copy link
Member Author

Choose a reason for hiding this comment

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

Gracefully borrowed from #33552

return Objects.hash(certificates);
}

public static GetSslCertificatesResponse fromXContent(XContentParser parser) throws IOException {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can't use ConstructingObjectParser as this API returns an array of Objects

[[java-rest-high-security-get-certificates-execute-async]]
==== Asynchronous Execution

This request can be executed asynchronously:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion, ... using the security().getSslCertificatesAsync() method:

this.certificates = certificates;
}

private static final DeprecationHandler DEPRECATION_HANDLER = new DeprecationHandler() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be added to DeprecationHandler as NOOP_DEPRECATION_HANDLER just like we have THROW_UNSUPPORTED_OPERATION so if need be others can use it, WDYT?

}
List<CertificateInfo> certificates = new ArrayList<>();
for (Object cert : unparsedCerts) {
try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
Copy link
Member

Choose a reason for hiding this comment

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

I am confused, why can't we just parse using the passed in parser?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because ConstructingObjectParser expects a parser with Objects in its parse() method but we get an list of objects in the parser. So I had to get the list from the passed in parser and parse each one.
Does this make sense?

Copy link
Contributor

@tvernum tvernum Oct 4, 2018

Choose a reason for hiding this comment

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

But ConstructingObjectParser can just parse a single node within the parser stream, so you can do something like this (untested, minimal error checking):

XContentParserUtils.ensureExpectedToken( Token.START_ARRAY, parser.currentToken(), parser::getTokenLocation);
while (parser.nextToken() != Token.END_ARRAY) {
   certificates.add( CertificateInfo.PARSER.parse(parser, null) );
}

Copy link
Member

Choose a reason for hiding this comment

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

What Tim said is what I had in mind

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, thanks! I didn't know I could do that. Now that you mentioned it, I saw this being used elsewhere and it makes total sense too.

@jkakavas
Copy link
Member Author

@jaymode I have already addressed your earlier points, I just failed to notify you. Would you mind taking another look? @bizybot I'd appreciate another look too.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM

@jkakavas jkakavas merged commit 55eaf7a into elastic:master Oct 15, 2018
@jkakavas jkakavas deleted the get-certificates-hlrc branch October 15, 2018 16:26
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 15, 2018
* master:
  Do not update number of replicas on no indices (elastic#34481)
  Core: Remove two methods from AbstractComponent (elastic#34336)
  Use RoleRetrievalResult for better caching (elastic#34197)
  Revert "Search: Fix spelling mistake in Javadoc (elastic#34480)"
  Search: Fix spelling mistake in Javadoc (elastic#34480)
  Docs: improve formatting of Query String Query doc page (elastic#34432)
  Tests: Handle epoch date formatters edge cases (elastic#34437)
  Test: Fix running with external cluster
  Fix handling of empty keyword in terms aggregation (elastic#34457)
  [DOCS] Fix tag in SecurityDocumentationIT
  [Monitoring] Add additional necessary mappings for apm-server (elastic#34392)
  SCRIPTING: Move Aggregation Script Context to its own class (elastic#33820)
  MINOR: Remove Deadcode in  ExpressionTermSetQuery (elastic#34442)
  HLRC: Get SSL Certificates API (elastic#34135)
jkakavas added a commit that referenced this pull request Oct 16, 2018
This change adds support for the get SSL certificate API to 
the high level rest client.
kcm pushed a commit that referenced this pull request Oct 30, 2018
This change adds support for the get SSL certificate API to 
the high level rest client.
@colings86 colings86 removed the :Security/Security Security issues without another label label Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants