Add separation between keys when exporting CAs

[marcoandredinis]

Adding a new line was sometimes not enough to separate the keys.

This was causing our tests to sometimes fail.

$ go test ./lib/client/ -run TestExportAuthorities/windows_exportSecrets --count 2000 -race
ok      github.com/gravitational/teleport/lib/client    46.648s

Fixes #18309

[zmb3]

I do not understand how this could fix the flakiness.

[marcoandredinis]

I do not understand how this could fix the flakiness.

It doesn't. At least not 100%.
But 99.9999% of the times it won't be flaky 😅

So, the output is two blobs and this adds a little mark between them.
Hopefully, there's a really low probability to have \n----------\n in those blobs.

[zmb3]

Sorry @marcoandredinis I still don't understand.

Isn't this the equivalent of changing strings.Split("a.b", ".") to strings.Split("a-b", "-")? How does that change anything?

[marcoandredinis]

Almost the same, yes.
The probability of a binary outputting \n\n is lower than outputting \n----------\n.

I agree this does not ensure that it won't happen again

However, IMO having a low probability is enough.

> go test ./lib/client/ -run TestExportAuthorities/windows_exportSecrets --count 20_000 -race
ok  	github.com/gravitational/teleport/lib/client	469.146s

I'm ok with going a step further

We can parse the keys with asn1.Unmarshal which returns the rest.
The downside is that we must bring the private x509.pkcs1PrivateKey structure into our code.

So, we would remove the write ret.WriteString("\n----------\n") and then change the test to this:

	// pkcs1PrivateKey is a structure which mirrors the PKCS #1 ASN.1 for an RSA private key.
	type pkcs1PrivateKey struct {
		Version int
		N       *big.Int
		E       int
		D       *big.Int
		P       *big.Int
		Q       *big.Int
	}

	validatePrivateKeyDERFunc := func(t *testing.T, s string) {
		bs := []byte(s)
		var priv pkcs1PrivateKey
		rest, err := asn1.Unmarshal(bs, &priv)
		require.NoError(t, err)

		privateKeyBytes, err := asn1.Marshal(priv)
		require.NoError(t, err)

		privKey, err := x509.ParsePKCS1PrivateKey(privateKeyBytes)
		require.NoError(t, err, "x509.ParsePKCS1PrivateKey failed")
		require.NotNil(t, privKey, "x509.ParsePKCS1PrivateKey returned a nil certificate")

		validateTLSCertificateDERFunc(t, string(rest))
	}

What do you think?

[strideynet]

So to clarify as I don't think this is very clear: am I right in saying the test currently flakes because there is a chance that that the generated secret keys binary output may contain the equivalent of \n\n which matches the separator we are using. And hence by increasing the length of the separator, it becomes far less likely that the binary output of the secret key will collide with it ?

I see two issues with this fix:

• It doesn't actually correct the code - just reduces the chance by which this bug occurs. This bug could theoretically occur in a user's actual environment.
• It changes the output output format.

The first issue is relatively minor, but the second issue is more interesting to me. Are we trying to match some prescribed format here so that something else can ingest it ? If so, changing the separator is not appropriate and will break whatever is trying to read this output.

If this output format is something of our choosing - then why are we outputting binary data raw like this at all ???

Surely we could encode the key data using base64 or similar, then output a newline, and then output the certificate encoded as base64 and this would completely avoid the possibility of an erroneous newline slipping in.

[marcoandredinis]

That is correct 👍

We currently have a command that outputs the windows certificate (der format):

tctl auth export --type windows

It can also output the key (so it prints two blobs):

tctl auth export --type windows --keys

In a previous PR I added the ability to do the same using an HTTP endpoint (it doesn't offer the --keys).
When changing this, I added tests (there was none) and added an empty line when printing two keys.

If you try to use the command tctl auth export --type windows --keys in v10/v11 it will print two blobs and you won't be able to distinguish where one ends and the other starts.
To prevent this, I added an empty line.

This is ok for most types (as in --type): openssh for users and servers and TLS certificates in PEM format.
The output of windows is in DER format, and it might contain any arbitrary value.

I'm ok in testing this using another method like I mentioned in the previous comment
I'm also ok with converting this into base64, but then we lose the ability to use a simple command like
tctl auth export --type windows > ca.der
tctl auth export --type tls >ca.pem
curl https://.../ca/export > ca.pem
And would need to add an extra base64 -d or something like that.

[zmb3]

The output of windows is in DER format, and it might contain any arbitrary value.

This is what I was missing. Now I understand.

I think if you're requesting a binary format like DER, it's really not safe to combine multiple different pieces of data in the same file. Could we instead write the different pieces to different files? Maybe the DER-encoded cert could be written to stdout, and we make --keys a flag with a path to file --keys=windows.key?

[strideynet]

Could we instead write the different pieces to different files? Maybe the DER-encoded cert could be written to stdout, and we make --keys a flag with a path to file --keys=windows.key?

Even simpler, could we have it so --keys only outputs the key, and not the certificate as well ? This is already how tctl export --type user and tctl export --type host already works ?

It would be nice to have some consistency between the types, it seems pretty hit or miss whether --keys gives the cert and the key or just the key.

[marcoandredinis]

@strideynet @zmb3
Thank you for the review.
Can you please take another look?

[strideynet]

I'm happy with this change in behaviour - but I think we need to make sure that any docs referencing this command have been updated :) We may need to consider releasing this in a major release since it's effectively a breaking change in the CLI interface.

[marcoandredinis]

I'm happy with this change in behaviour - but I think we need to make sure that any docs referencing this command have been updated :) We may need to consider releasing this in a major release since it's effectively a breaking change in the CLI interface.

Agree 👍
I've changed the cli reference
I couldn't find any other doc using the --keys flag.

I'm not backporting this change, so this will go out on a major release.
Is there a way to track breaking changes? Maybe a label?

[zmb3]

I put the release-notes label on this so we remember to include a note in the v12 release notes.

[marcoandredinis]

To ease the creation of the release notes, the breaking change is:

• tctl auth export only exports the private key when passing the --keys flag

(feel free to adapt the wording if necessary)