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

Support PKCS#12 encoded certificates #17261

Merged
merged 6 commits into from
Apr 4, 2018

Conversation

legrego
Copy link
Member

@legrego legrego commented Mar 19, 2018

⚠️ This PR has been reverted

The implementation here was flawed, in that we were not able to correctly handle requests made by the Kibana server on behalf of an end-user. There are many features within Kibana that require the kibana server to essentially proxy the end-users request to elasticsearch. When this is done, the Kibana server cannot attach client certificates to the proxied request. The libraries provided by nodejs for dealing with PKCS12 certificates do not allow us to extract the CA from the bundle -- it's either an all or nothing deal.

@kobelb summarizes the issue in the revert for this PR: #17801


This PR adds new configuration properties to support PKCS#12 encoded certificates. This allows the certificate, key, and CA chain to be specified in a single file, which simplifies administrating Kibana installations.

New kibana.yml Properties

server.ssl.keystore.path

This is an alternative to server.ssl.certificate and server.ssl.key. If a CA chain is not included in this file, then PEM encoded certificates can be specified using the existing server.ssl.certificateAuthorities property.

server.ssl.keystore.password

Password to to decrypt the key within the PKCS#12 file. Analogous to server.ssl.keyPassphrase for server.ssl.key

elasticsearch.ssl.keystore.path

This is an alternative to elasticsearch.ssl.certificate and elasticsearch.ssl.key. If a CA chain is not included in this file, then PEM encoded certificates can be specified using the existing elasticsearch.ssl.certificateAuthorities property.

elasticsearch.ssl.keystore.password

Password to to decrypt the key within the PKCS#12 file. Analogous to elasticsearch.ssl.keyPassphrase for elasticsearch.ssl.key

Details

If the [...].ssl.keystore.path property is set, then the corresponding [...].ssl.certificate and [...].ssl.key properties must be left unset.

Closes #17039

@legrego legrego added WIP Work in progress Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Mar 19, 2018
@legrego legrego requested a review from kobelb March 19, 2018 21:09
@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego legrego self-assigned this Mar 20, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego
Copy link
Member Author

legrego commented Mar 21, 2018

@kobelb, I backed out the Joi upgrade and schema validations per our slack chat. The new SSL configuration is instead validated by throwing errors where appropriate. Can you review when you get a chance?

@legrego legrego removed the WIP Work in progress label Mar 21, 2018
@kobelb kobelb requested a review from jbudz March 26, 2018 14:55
@jbudz
Copy link
Member

jbudz commented Mar 26, 2018

Thoughts on aligning with elasticsearch's format, specifically with a type flag that gets set to PKCS12?

@jbudz
Copy link
Member

jbudz commented Mar 26, 2018

Related, are there any other formats we might support (i.e I should do some research, are there other (un)commonly common formats?)

@kobelb
Copy link
Contributor

kobelb commented Mar 26, 2018

Thoughts on aligning with elasticsearch's format, specifically with a type flag that gets set to PKCS12?

To be honest, I didn't realize that Elasticsearch already supported PKCS#12, and we generally try to keep our SSL settings inline with theirs, good call @jbudz .

It also looks like Elasticsearch is differentiating between a keyStore and a trustStore, with the keyStore storing the equivalent of ssl.certificate and ssl.key and the trustStore being used for the certificate authorities. @legrego does it seem feasible for us to implement a similar approach?

@kobelb
Copy link
Contributor

kobelb commented Mar 26, 2018

Related, are there any other formats we might support (i.e I should do some research, are there other (un)commonly common formats?)

I haven't heard of any others at this time, but I'd prefer to follow what the Elasticsearch team does to make sure we stay consistent.

@legrego
Copy link
Member Author

legrego commented Mar 26, 2018

It also looks like Elasticsearch is differentiating between a keyStore and a trustStore, with the keyStore storing the equivalent of ssl.certificate and ssl.key and the trustStore being used for the certificate authorities. @legrego does it seem feasible for us to implement a similar approach?

@kobelb @jbudz I'm not sure if we can do that or not, but I'll do some testing and get back to you. I'd prefer to be consistent with Elasticsearch if possible, but I'm a little hesitant to use the same configuration properties. It's my understanding that keystore and truststore are Java-specific terms, and I'm worried that users will think they can use the JKS file format with these properties, when in reality they have to use PKCS#12. The Java keytool can export to PKCS#12, but it's not the default behavior.

@legrego
Copy link
Member Author

legrego commented Mar 27, 2018

@kobelb @jbudz
Update:
Node's HTTPS Agent allows multiple PKCS#12 files to be passed, but each one needs to have both a cert and key pair. The SecureContext cannot be created with a PKCS#12 file that does not contain a key.

The only way to get the certificate chain ("truststore") in PKCS#12 format is to include it within the file that contains the server cert and key. This is done using OpenSSL's -certfile option when creating a PKCS#12 file.

So the end result in that scenario is a single PKCS#12 file that contains:

  • server certificate
  • certificate private key
  • certificate authorities chain

Given this, users have a few options:

  1. A single PKCS#12 file containing everything to use and verify the certificate
  2. A single PKCS#12 file containing the cert and key, and a separate list of PEM-encoded certificates used to verify the certificate, via the existing server.ssl.certificateAuthorities property.
  3. A single PKCS#12 file containing the cert and key, and no certificate authorities specified. This only works if the cert is already signed by a built-in trusted authority

@kobelb
Copy link
Contributor

kobelb commented Mar 27, 2018

@jaymode @tvernum @jkakavas we're looking at adding PKCS#12 support in Kibana, and generally like to keep our SSL settings inline with Elasticsearch. Currently, Elasticsearch's SSL settings are differentiating between the keyStore and the trustStore, while we were hoping to specify a single PKCS#12 certificate that contains everything to use and verify the certificate. Do you all have any intent on implementing this, and if so do you have any qualms with referring to this setting as .pfx, so we'd have something like xpack.security.http.ssl.pfx: /some/path/here.pfx ?

@jkakavas
Copy link
Member

When we generate node certificates with certutil, we end up with a PKCS#12 keystore that can be used both as a keystore and as a truststore ( as it contains the node's private key and certificate, as well as the CA certificate ) .

So, we already do support a single PKCS#12 certificate that contains everything to use and verify the certificate. Is this about the setting name only ? That is, instead of having :

xpack.security.http.ssl.keystore.path: certs/elastic-certificates.p12 
xpack.security.http.ssl.truststore.path: certs/elastic-certificates.p12

to be able to support

xpack.security.http.ssl.pfx: certs/elastic-certificates.p12

?

@kobelb
Copy link
Contributor

kobelb commented Mar 28, 2018

@jkakavas yup!

@jaymode
Copy link
Member

jaymode commented Mar 28, 2018

@kobelb I personally do not like the use of pfx in the setting name. PFX was introduced by Microsoft and superceded by pkcs12, which cleaned up some issues in the PFX format. I see that node uses pfx as a option name, so the name makes sense in the kibana context from a developer perspective. I'd like to have a term that will make sense for all components of the stack.

One thought is that we've adopted the notion of keystore across the stack for secure settings; I wonder if it makes sense to use this here? I do have a concern that keystore then becomes somewhat of an overloaded term, which it already is on the elasticsearch side today. I'd really like to see what @tvernum thinks as well.

@kobelb
Copy link
Contributor

kobelb commented Mar 28, 2018

@kobelb I personally do not like the use of pfx in the setting name. PFX was introduced by Microsoft and superceded by pkcs12, which cleaned up some issues in the PFX format. I see that node uses pfx as a option name, so the name makes sense in the kibana context from a developer perspective. I'd like to have a term that will make sense for all components of the stack.

That makes sense, and we aren't intimately tied to calling it .pfx, we just wanted a setting that could provide the pkcs12 certificate chain.

Does referring to it as a keystore alone cause any confusion with the way we separate the keystore and truststore on the Elasticsearch side?

@jaymode
Copy link
Member

jaymode commented Mar 28, 2018

Does referring to it as a keystore alone cause any confusion with the way we separate the keystore and truststore on the Elasticsearch side?

Going back to Shield, we allowed users to define a keystore in the settings and if the truststore setting was not set, we'd use the keystore as the truststore. I personally feel like a single store is the easiest to reason about and having two settings makes it confusing.

@jkakavas
Copy link
Member

I agree with a single configuration option as a fallback but maybe not enforce it:

  • Node apparently can't support a PKCS#12 store with only certificates in it ( acting as a truststore ) while Java can ( introduced in Java 8 ).
    That would probably allow Elasticsearch to support
xpack.security.http.ssl.keystore.path: certs/store_with_keypair.p12 
xpack.security.http.ssl.truststore.path: certs/store_with_certs_only.p12

I'm not sure how commonly used that is though and if we want to support it though.

  • It would make deployments where there are pre-existing JKS truststores and keystores less flexible

@kobelb
Copy link
Contributor

kobelb commented Mar 28, 2018

So if we were to add the following settings for Kibana, they seem to fall in line with what's supported via elasticsearch now:

server.ssl.keystore.type: PKCS12
server.ssl.keystore.path: /some/path/here.p12
server.ssl.keystore.password: 'supersecret'

and they would provide the certs, key and certs for the certificate authorities.

Is that correct?

@jaymode
Copy link
Member

jaymode commented Mar 28, 2018

That's correct. Do you need the server.ssl.keystore.type: PKCS12 type setting? My understanding is there is node only supports for PFX and PKCS12 and no value needs to be specified.

@kobelb
Copy link
Contributor

kobelb commented Mar 28, 2018

I was only proposing using server.ssl.keystore.type: PKCS12 to mirror what Elasticsearch does, if we're comfortable leaving that setting out and just assuming it's PKCS12, I'm fine with that as well.

@legrego
Copy link
Member Author

legrego commented Mar 28, 2018

I think we can get away without the server.ssl.keystore.type property - I don't envision supporting the JKS format in Kibana, but if we ever do, then we can add it at that point.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego legrego added v6.4.0 and removed review labels Apr 4, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@legrego legrego merged commit de91bd0 into elastic:master Apr 4, 2018
legrego added a commit that referenced this pull request Apr 5, 2018
* [6.x] Backport Support PKCS#12 Certificates
kobelb added a commit to kobelb/kibana that referenced this pull request Apr 19, 2018
kobelb added a commit that referenced this pull request Apr 19, 2018
* Revert "Support PKCS#12 encoded certificates (#17261)"

This reverts commit de91bd0.

* Fixing tests
kobelb added a commit to kobelb/kibana that referenced this pull request Apr 19, 2018
…c#17801)

* Revert "Support PKCS#12 encoded certificates (elastic#17261)"

This reverts commit de91bd0.

* Fixing tests
kobelb added a commit that referenced this pull request Apr 19, 2018
* Revert "Support PKCS#12 encoded certificates (#17261)"

This reverts commit de91bd0.

* Fixing tests
@kobelb kobelb removed the v6.4.0 label Apr 19, 2018
@epixa epixa added the reverted label Oct 19, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@legrego legrego deleted the support-pkcs12-17039 branch October 19, 2018 12:40
@jportner jportner assigned jportner and legrego and unassigned legrego and jportner Nov 21, 2019
@jportner jportner mentioned this pull request Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reverted Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PKCS #12 Certificates
9 participants