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

add socket.getPeerCertificate to KibanaRequest #42929

Merged
merged 3 commits into from
Aug 9, 2019

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Aug 8, 2019

Summary

Security plugin needs to access request certificate to implement PKI authentication.
I added a separate abstraction for sockets in case we need to access other properties.
Don't want to expose the whole socket object as it provides too many low-level details.

blocker for #42606

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

Dev Docs

KibanaRequest object can provide peer certificate

const cert = request.socket.getPeerCertificate();

@mshustov mshustov added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes v7.4.0 labels Aug 8, 2019
@mshustov mshustov requested a review from a team as a code owner August 8, 2019 12:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov mshustov added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes labels Aug 8, 2019
@mshustov mshustov requested a review from azasypkin August 8, 2019 12:05
@elasticmachine
Copy link
Contributor

💔 Build Failed

export interface IKibanaSocket {
// (undocumented)
getPeerCertificate(detailed: true): DetailedPeerCertificate | null;
// (undocumented)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd expect api-extractor to merge all overloads in the one file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's kinda strange. You might be able to use the @inheritDoc directive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, neither @inheritDoc nor @link.

The @link reference could not be resolved: The reference is ambiguous because "getPeerCertificate" has more than one declaration; you need to add a TSDoc member reference selector^M

seems the problem is not solved yet microsoft/rushstack#881

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin
Copy link
Member

ACK: looking..

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the quick turnaround on this one! Tested with PKI auth provider locally and it seems everything works as expected.

* The returned object has some properties corresponding to the field of the certificate.
* If detailed argument is true the full chain with issuer property will be returned,
* if false only the top certificate without issuer property.
* If the peer does not provide a certificate, it returns null.
Copy link
Member

Choose a reason for hiding this comment

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

nit: it can also be null if socket has been destroyed (please ignore if it's the only change needed for merge).

getPeerCertificate(detailed?: boolean): PeerCertificate | DetailedPeerCertificate | null;

public getPeerCertificate(detailed?: boolean) {
if (this.socket instanceof TLSSocket) {
Copy link
Member

Choose a reason for hiding this comment

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

note: PKI authentication provider will consume certificate chain returned from this method, but it works on the assumption that rejectUnauthorized is set to true in the core server and hence we don't need to verify socket.authorized. rejectUnauthorized in the core server isn't configurable right now and hence relies on Node's default value which is true.

So everything is fine right now, but in case we decide to expose rejectUnauthorized as a configuration option for some reason (hopefully not) we may need to expose socket.authorized from IKibanaSocket. Just wanted to note.

/cc @kobelb

src/core/server/http/router/socket.ts Show resolved Hide resolved
@mshustov mshustov merged commit c30df8d into elastic:master Aug 9, 2019
@mshustov mshustov deleted the request-peer-certificate branch August 9, 2019 08:42
mshustov added a commit to mshustov/kibana that referenced this pull request Aug 9, 2019
* add socket.getPeerCertificate to KibanaRequest

* update request mocks

* update docs
mshustov added a commit that referenced this pull request Aug 9, 2019
* add socket.getPeerCertificate to KibanaRequest

* update request mocks

* update docs
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 9, 2019
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (306 commits)
  [ML] Adding job overrides to the module setup endpoint (elastic#42946)
  [APM] Fix missing RUM url (elastic#42940)
  close socket timeouts without message (elastic#42456)
  Upgrade elastic/charts to 8.1.6 (elastic#42518)
  [ML] Delete old AngularJS data visualizer and refactor folders (elastic#42962)
  Add custom formatting for Date Nanos Format (elastic#42445)
  [Vega] Shim new platform - vega_fn.js -> vega_fn.js , use ExpressionFunction (elastic#42582)
  add socket.getPeerCertificate to KibanaRequest (elastic#42929)
  [Automation] ISTANBUL PRESET PATH is not working fine with constructor(private foo) (elastic#42683)
  [ML] Data frames: Updated stats structure. (elastic#42923)
  [Code] fixed the issue that the repository can not be deleted in some cases. (elastic#42841)
  [kbn-es] Support for passing regex value to ES (elastic#42651)
  Connect to Elasticsearch via SSL when starting kibana with `--ssl` (elastic#42840)
  Add Elasticsearch SSL support for integration tests (elastic#41765)
  Fix duplicate fetch in Visualize (elastic#41204)
  [DOCS] TSVB and Timelion clean up (elastic#42953)
  [Maps] [File upload] Fix maps geojson upload hanging on index step (elastic#42623)
  [APM] Use rounded bucket sizes for transaction distribution (elastic#42830)
  [yarn.lock] consistent resolve domain (elastic#42969)
  [Uptime] [Test] Repurpose unit test assertions to avoid flakiness (elastic#40650)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants