Deprecate cert gen without a CA and add self-signed option #64037

ywangd · 2020-10-22T08:20:22Z

Generating a CA on the fly is an attempt at workflow optimisation that was inherited from certgen. There are potential pitfalls with this approach. Overall it is recommended to separate the step of CA creation and mandate a CA to be specified when generating certificate.

This PR add a deprecation message if the cert command is used without specifying a CA. A follow up PR will throw error for this usage in 8.0.

For use case where we explicitly trust a certificate without needing a CA, e.g. SAML message signing, the PR adds a --self-signed option to the cert sub-command to generate self-signed certificate.

Relates: #61884

elasticmachine · 2020-10-22T08:20:25Z

Pinging @elastic/es-security (:Security/Security)

ywangd · 2020-10-22T10:12:36Z

Doc change preview: https://elasticsearch_64037.docs-preview.app.elstc.co/guide/en/elasticsearch/reference/master/certutil.html#certutil-cert

lcawl · 2020-10-22T22:53:02Z

Does the example in this section of the SAML guide need to be updated to use a CA too?: https://www.elastic.co/guide/en/elasticsearch/reference/master/saml-guide-authentication.html#_generating_certificates_and_keys

lcawl

One comment, otherwise LGTM

jkakavas · 2020-10-23T19:11:43Z

Does the example in this section of the SAML guide need to be updated to use a CA too?: https://www.elastic.co/guide/en/elasticsearch/reference/master/saml-guide-authentication.html#_generating_certificates_and_keys

In SAML we don't use PKI but explicit trust of certificates so keeping the CA keys around is not necessary. This was actually one of the cases where the behavior we are now deprecating and removing was helpful and made sense. Instead of doing a 2 step process just for the sake of using certutilhere, could we have an example with openssl ?

tvernum · 2020-10-26T02:19:05Z

Is it worth adding a --self-signed option to certutil cert to cater for the SAML use case?

ywangd · 2020-10-26T04:33:34Z

This was actually one of the cases where the behavior we are now deprecating and removing was helpful and made sense. Instead of doing a 2 step process just for the sake of using certutilhere, could we have an example with openssl ?

Thakns for this info. For me, replacing it with openssl has a few downsides:

Introduces another dependency. IIUC, elasticsearch-certutil was created to (at least partially) avoid having to use openssl. So would it feel backwards if we have to turn back to openssl?
Most other places (if not all) uses openssl, so it is inconsistent.
As I understand it, openssl has some inconsistency/installation issues across different OS (though it may not be an issue for the req command that we need here).
The existing saml-guide doc page needs some more wording changes to accomodate the openssl replacement

Is it worth adding a --self-signed option to certutil cert to cater for the SAML use case?

This is plausible when you need only a single cert. However, it could be problematic when creating multiple certificates. If users try to use it this for multiple nodes, it will at least cause confusion. We could forbid generating mulitple certs if --self-signed is requested. But even with that, it could still cause confusion in a TLS context, since our docs differentiate between cert and CA most of the times (if not all the time). There is also a minor difference between using two calls (ca and cert, or cert with on the fly CA) and self-signed certificate. The self-signed cert has TRUE value for the CA attribute while the other one has it as FALSE. I don't think it really matter in our use case. Just bring it up in case there are unobvious issues.

So given the above information, I wonder whether we should re-evaluate our decision on this issue, i.e. whether we should disallow generating CA on the fly entirely? By disallowing it, two-step process is always required to generate CA and certs. Does it go against "easier security setup" idea that we try to uphold? Or will "security-on-by-default" provide a different solution to make this worry obsolete?

PS: Another place we generate CA and certs together is for configuring TLS in docker.

jkakavas · 2020-10-26T09:13:15Z

Is it worth adding a --self-signed option to certutil cert to cater for the SAML use case?

This sounds like a good idea to me.

Introduces another dependency. IIUC, elasticsearch-certutil was created to (at least partially) avoid having to use openssl. So would it feel backwards if we have to turn back to openssl?

I don't see this as a dependency, but merely as a suggestion as to how one can generate a keypair (more?) easily if openssl is available. We can have instructions with certutil (only|too), it's just two commands instead of one.

The existing saml-guide doc page needs some more wording changes to accomodate the openssl replacement

Making changes is not a downside in my opinion.

As I understand it, openssl has some inconsistency/installation issues across different OS (though it may not be an issue for the req command that we need here).

I don't think there is any problematic behavior wrt to key/certificate generation ( especially for certificates that are merely public key containers )

However, it could be problematic when creating multiple certificates. If users try to use it this for multiple nodes, it will at least cause confusion

I think there is that much we can do to prevent misuse of the tools we create. In the same line of thinking, nothing prevents a user to generate a new CA and cert on each node if they are not familiar enough with PKI and certificates.

. But even with that, it could still cause confusion in a TLS context, since our docs differentiate between cert and CA most of the times (if not all the time)

Can you explain your concern?

The self-signed cert has TRUE value for the CA attribute while the other one has it as FALSE. I don't think it really matter in our use case

+1

So given the above information, I wonder whether we should re-evaluate our decision on this issue, i.e. whether we should disallow generating CA on the fly entirely? By disallowing it, two-step process is always required to generate CA and certs. Does it go against "easier security setup" idea that we try to uphold? Or will "security-on-by-default" provide a different solution to make this worry obsolete?

I don't think we should re-evaluate. Security on by default would probably not replace our existing tooling with other solutions and I don't see this as making it more difficult for users to set up TLS. The SAML use case affects a fraction of our users since a) request signing is not enabled by default and b) response encryption is not commonly used. I think that a --self-signed makes sense for the SAML case (or for a "I just want a quick key/certificate that I can set on my local node and I know what I'm doing" use case) but even if we don't add it, I expect that the users needing it might not depend on certutil to create their signing/encryption key pairs either way - or they will be ok issuing two certutil commands.

ywangd · 2020-10-26T11:29:57Z

But even with that, it could still cause confusion in a TLS context, since our docs differentiate between cert and CA most of the times (if not all the time)

Can you explain your concern?

What I mean is that our docs talk about separate CA and cert in most places (if not all places). When configuring TLS, the examples have different values (files) for certificate_authorities and certificate (except for p12 truststore where it's an all-in-one). Self-signed certificate will be a new thing for our docs. I was wondering whether this could be confusing users because 1) certificate_authorities and certificate will have the same value when configuring TLS and 2) if we do want to explain it, feels like we will be adding docs just for something that is rarely recommended. These are minor concerns. I am actually not too worried about them. Just raise it as discussion point.

Based on the discussion, I think a new --self-signed option is the way to go. I think it makes sense for it to be part of this PR. Will work on it.

…ert-deprecate-no-ca

ywangd · 2020-11-04T05:48:49Z

@tvernum @lcawl @jkakavas The latest push added the --self-signed option as discussed, also updated relevant docs. Could you please take another look? Thanks!

…ert-deprecate-no-ca

jkakavas

Just a few comments and suggestions. Can we also update the PR description to mention the self-signed option?

docs/reference/commands/certutil.asciidoc

jkakavas · 2020-11-13T15:30:29Z

x-pack/docs/en/security/authentication/saml-guide.asciidoc

@@ -505,11 +505,11 @@ support. Since PEM format is the most commonly supported format, the examples
 below will generate certificates in that format.

 Using the <<certutil,`elasticsearch-certutil` tool>>, you can generate a
-signing certificate with the following command:
+self-signed certificate with the following command:


The most important characteristic is that this is a signing certificate (used to sign SAML messages), not that it is a self-signed one. I'd suggest we revert this change

You are right. Reverted.

x-pack/docs/en/security/authentication/saml-guide.asciidoc

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

jkakavas · 2020-11-13T15:49:26Z

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

@@ -807,7 +819,8 @@ void generateAndWriteSignedCertificates(Path output, boolean writeZipFile, Optio
                        } else {
                            final String fileName = entryBase + ".p12";
                            outputStream.putNextEntry(new ZipEntry(fileName));
-                            writePkcs12(fileName, outputStream, certificateInformation.name.originalName, pair, caInfo.certAndKey.cert,
+                            writePkcs12(fileName, outputStream, certificateInformation.name.originalName, pair,


I also think we don't need to add a certificate entry too when we create a self-signed PKCS12, but did we check?

This is a good point and it turns out the check is missing ...
I now augumented testCreateCaAndMultipleInstances to test p12 generation for self-signed cert.

My point was actually about the construction of the PKCS#12 bundle and whether or not it suffices to add a private key entry with the key and certificate, or if we also need to add a trusted certificate entry with the certificate too. I believe we don't and other tooling ( keytool, openssl ) seem to agree too :)

tvernum

Sorry, It looks like I had these comments pending for a week or so & didn't submit them...

tvernum · 2020-11-04T06:38:52Z

docs/reference/commands/certutil.asciidoc

@@ -211,6 +217,9 @@ wish to password-protect your PEM keys, then do not specify
 `--pem`:: Generates certificates and keys in PEM format instead of PKCS#12. This
 parameter cannot be used with the `csr` parameter.

+`--self-signed`:: Generates self-signed certificates. This parameter is only
+applicable to the `cert` parameter.


I have a concern, which might not be a big deal, that some people will see this option and use it in places where they shouldn't.
For all TLS scenarios, we recommend having a separation between the CA and the node cert, and I think we want to keep recommending that.
So, are there words we can put here that discourage people from using it for TLS?

I added a note to state this option should be used with care.

tvernum · 2020-11-04T06:39:33Z

x-pack/docs/en/security/authentication/saml-guide.asciidoc

@@ -505,11 +505,11 @@ support. Since PEM format is the most commonly supported format, the examples
 below will generate certificates in that format.

 Using the <<certutil,`elasticsearch-certutil` tool>>, you can generate a
-signing certificate with the following command:
+self-signed certificate with the following command:


I don't think this should change. It is a signing certificate, it just happens to also be self-signed.

You are right. Ioannis said the same. It's reverted. Thanks.

tvernum · 2020-11-04T06:40:22Z

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

@@ -167,8 +167,9 @@ public static void main(String[] args) throws Exception {

    static final String CA_EXPLANATION =
        "    * All certificates generated by this tool will be signed by a certificate authority (CA).\n" +


This isn't strictly true anymore. Do we care?

Ioannis has kindly offered suggestion and it should be accurate now.

Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>

…ert-deprecate-no-ca

jkakavas

LGTM, I'll defer more scrutiny of the wording changes to Lisa and Tim

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

jkakavas · 2020-11-17T08:52:28Z

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

@@ -807,7 +819,8 @@ void generateAndWriteSignedCertificates(Path output, boolean writeZipFile, Optio
                        } else {
                            final String fileName = entryBase + ".p12";
                            outputStream.putNextEntry(new ZipEntry(fileName));
-                            writePkcs12(fileName, outputStream, certificateInformation.name.originalName, pair, caInfo.certAndKey.cert,
+                            writePkcs12(fileName, outputStream, certificateInformation.name.originalName, pair,


My point was actually about the construction of the PKCS#12 bundle and whether or not it suffices to add a private key entry with the key and certificate, or if we also need to add a trusted certificate entry with the certificate too. I believe we don't and other tooling ( keytool, openssl ) seem to agree too :)

…ck/security/cli/CertificateTool.java Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

lcawl · 2020-11-18T16:23:06Z

docs/reference/commands/certutil.asciidoc

+All certificates that are generated by this command are signed by a CA unless
+the `--self-signed` parameter is specified. You can provide your own CA with the
+`--ca` or `--ca-cert` and `--ca-key` parameters. Otherwise, the command automatically generates a new CA for you.
+deprecated:[7.11.0,"Generating certificates without specifying a CA certificate and key is deprecated. In the next major version you must provide a CA certificate unless the `--self-signed` option is specified."]


deprecated:[7.11.0,"Generating certificates without specifying a CA certificate and key is deprecated. In the next major version you must provide a CA certificate unless the --self-signed option is specified."]

I've combined this information into a single paragraph so that it's (hopefully) clearer what's deprecated.

…ck/security/cli/CertificateTool.java Co-authored-by: Lisa Cawley <lcawley@elastic.co>

tvernum · 2020-11-18T10:42:51Z

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

        GenerateCertificateCommand() {
            super("generate X.509 certificates and keys");
            acceptCertificateGenerationOptions();
+            selfSigned = parser.accepts("self-signed", "generate self signed certificates (takes precedence over the ca options)");


Why would this take precedence rather than being an error?

If a user enters

certutil cert --ca foo.p12 --self-signed

that's contradictory, and I would expect it to be an error, but it actually would generate a self signed cert, and ignore the CA. That seems wrong to me.

Good catch, Thanks. I misunderstood the existing behaviour of --ca and --ca-cert. The two plus the new --self-signed should be mutually exclusive. It is now fixed.

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java

…ck/security/cli/CertificateTool.java Co-authored-by: Tim Vernum <tim@adjective.org>

…ert-deprecate-no-ca

tvernum

LGTM

…4037) Generating a CA on the fly is an attempt at workflow optimisation that was inherited from certgen. There are potential pitfalls with this approach. Overall it is recommended to separate the step of CA creation and mandate a CA to be specified when generating certificate. This PR add a deprecation message if the cert command is used without specifying a CA. A follow up PR will throw error for this usage in 8.0. For use case where we explicitly trust a certificate without needing a CA, e.g. SAML message signing, the PR adds a --self-signed option to the cert sub-command to generate self-signed certificate.

…65575) Generating a CA on the fly is an attempt at workflow optimisation that was inherited from certgen. There are potential pitfalls with this approach. Overall it is recommended to separate the step of CA creation and mandate a CA to be specified when generating certificate. This PR add a deprecation message if the cert command is used without specifying a CA. A follow up PR will throw error for this usage in 8.0. For use case where we explicitly trust a certificate without needing a CA, e.g. SAML message signing, the PR adds a --self-signed option to the cert sub-command to generate self-signed certificate.

Deprecate cert genereation without specifying a CA

cc4f004

ywangd added >enhancement :Security/Security Security issues without another label v8.0.0 v7.11.0 labels Oct 22, 2020

ywangd requested a review from tvernum October 22, 2020 08:20

elasticmachine added the Team:Security Meta label for security team label Oct 22, 2020

ywangd added 2 commits October 22, 2020 20:15

Doc update

716a7c1

Update to use deprecated directive

91e6670

ywangd requested a review from lcawl October 22, 2020 10:12

ywangd added >deprecation and removed >enhancement labels Oct 22, 2020

lcawl approved these changes Oct 22, 2020

View reviewed changes

ywangd added 2 commits October 30, 2020 11:13

Merge remote-tracking branch 'origin/master' into es-61884-certutil-c…

6ae1906

…ert-deprecate-no-ca

Add self-signed option

efa9232

ywangd requested review from lcawl and jkakavas November 4, 2020 05:48

Merge remote-tracking branch 'origin/master' into es-61884-certutil-c…

3558a2c

…ert-deprecate-no-ca

jkakavas reviewed Nov 13, 2020

View reviewed changes

tvernum reviewed Nov 15, 2020

View reviewed changes

Apply suggestions from code review

cdc271c

Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>

ywangd added 3 commits November 16, 2020 22:25

Address feedback

1b01ddf

Merge remote-tracking branch 'origin/master' into es-61884-certutil-c…

e646bc1

…ert-deprecate-no-ca

mintor comments tweak

f4121b5

ywangd requested review from jkakavas and tvernum November 16, 2020 11:34

jkakavas approved these changes Nov 17, 2020

View reviewed changes

Update x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpa…

ae6ec18

…ck/security/cli/CertificateTool.java Co-authored-by: Ioannis Kakavas <ikakavas@protonmail.com>

tvernum changed the title ~~Deprecate certificate genereation without specifying a CA~~ Deprecate certificate generation without specifying a CA Nov 18, 2020

lcawl reviewed Nov 18, 2020

View reviewed changes

.../plugin/security/cli/src/main/java/org/elasticsearch/xpack/security/cli/CertificateTool.java Outdated Show resolved Hide resolved

[DOCS] Reformats deprecation details

09694d3

lcawl reviewed Nov 18, 2020

View reviewed changes

Update x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpa…

4698753

…ck/security/cli/CertificateTool.java Co-authored-by: Lisa Cawley <lcawley@elastic.co>

tvernum reviewed Nov 19, 2020

View reviewed changes

ywangd and others added 3 commits November 20, 2020 22:59

Update x-pack/plugin/security/cli/src/main/java/org/elasticsearch/xpa…

59329ed

…ck/security/cli/CertificateTool.java Co-authored-by: Tim Vernum <tim@adjective.org>

Merge remote-tracking branch 'origin/master' into es-61884-certutil-c…

55a7212

…ert-deprecate-no-ca

Address comments

3adf1ce

ywangd requested a review from tvernum November 20, 2020 12:34

tvernum approved these changes Nov 26, 2020

View reviewed changes

ywangd changed the title ~~Deprecate certificate generation without specifying a CA~~ Deprecate cert gen without a CA and add self-signed option Nov 29, 2020

ywangd merged commit bdd99b2 into elastic:master Nov 29, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate cert gen without a CA and add self-signed option #64037

Deprecate cert gen without a CA and add self-signed option #64037

ywangd commented Oct 22, 2020 •

edited

Loading

elasticmachine commented Oct 22, 2020

ywangd commented Oct 22, 2020

lcawl commented Oct 22, 2020

lcawl left a comment

jkakavas commented Oct 23, 2020

tvernum commented Oct 26, 2020

ywangd commented Oct 26, 2020

jkakavas commented Oct 26, 2020

ywangd commented Oct 26, 2020

ywangd commented Nov 4, 2020

jkakavas left a comment

jkakavas Nov 13, 2020

ywangd Nov 16, 2020

jkakavas Nov 13, 2020

ywangd Nov 16, 2020

jkakavas Nov 17, 2020

tvernum left a comment

tvernum Nov 4, 2020

ywangd Nov 16, 2020

tvernum Nov 4, 2020

ywangd Nov 16, 2020

tvernum Nov 4, 2020

ywangd Nov 16, 2020

jkakavas left a comment

jkakavas Nov 17, 2020

lcawl Nov 18, 2020

tvernum Nov 18, 2020

ywangd Nov 20, 2020

tvernum left a comment

		@@ -167,8 +167,9 @@ public static void main(String[] args) throws Exception {

		static final String CA_EXPLANATION =
		" * All certificates generated by this tool will be signed by a certificate authority (CA).\n" +

Deprecate cert gen without a CA and add self-signed option #64037

Deprecate cert gen without a CA and add self-signed option #64037

Conversation

ywangd commented Oct 22, 2020 • edited Loading

elasticmachine commented Oct 22, 2020

ywangd commented Oct 22, 2020

lcawl commented Oct 22, 2020

lcawl left a comment

Choose a reason for hiding this comment

jkakavas commented Oct 23, 2020

tvernum commented Oct 26, 2020

ywangd commented Oct 26, 2020

jkakavas commented Oct 26, 2020

ywangd commented Oct 26, 2020

ywangd commented Nov 4, 2020

jkakavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

ywangd commented Oct 22, 2020 •

edited

Loading