/v1/agent/connect/ca/leaf/web endpoint returns CertPEM without a trailing newline when using vault ca provider

[lkysow]

Overview of the Issue

When the Vault CA provider is used, the response from the leaf endpoint, e.g. /v1/agent/connect/ca/leaf/web has a key CertPEM. This key does not have a trailing newline.

When the built in Consul CA provider is used, this key does have a trailing newline.

The responses should be consistent (either no trailing newline or trailing newline).

Built-in CA Response:

{
  "SerialNumber": "11",
  "CertPEM": "-----BEGIN CERTIFICATE-----\nMIICPTCCAeOgAwIBAgIBETAKBggqhkjOPQQDAjAwMS4wLAYDVQQDEyVwcmktaG5x\naDgxNi5jb25zdWwuY2EuZjI5Mjc4NDYuY29uc3VsMB4XDTIwMDYyMzIzMDAxMloX\nDTIwMDYyNjIzMDAxMlowKjEoMCYGA1UEAxMfd2ViLnN2Yy5kZWZhdWx0LmYyOTI3\nODQ2LmNvbnN1bDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABKQzmdcNg0qqFN1p\n4B43RZRWy+/xxairaUdUNwCtsbNDa5A5t5xd+xwdBkzuMUO3cXqCmwkAiSgo2WEE\n4U58Y6ijgfMwgfAwDgYDVR0PAQH/BAQDAgO4MB0GA1UdJQQWMBQGCCsGAQUFBwMC\nBggrBgEFBQcDATAMBgNVHRMBAf8EAjAAMCkGA1UdDgQiBCBPQBOTl3zgdTzTAK2o\nj6pvMonO2nBvV2e4Gq0/5W9YXzArBgNVHSMEJDAigCCDGFQDyEFUTA4I/ykoSI5h\nu1Z3iAMCmbHh0QXy1/mfNDBZBgNVHREEUjBQhk5zcGlmZmU6Ly9mMjkyNzg0Ni04\nM2MyLWQ1ZTMtYWZjYi0yZTg3YzFhNzJhYjUuY29uc3VsL25zL2RlZmF1bHQvZGMv\nZ2tlL3N2Yy93ZWIwCgYIKoZIzj0EAwIDSAAwRQIhAI65YMVQN+kZv5yCA/fsznLK\n8kMuqC8W+7Xe6UdAJFBDAiA1myRiutr08K4m1/St05926sygtbfiKY4r5zOhvyi/\nkA==\n-----END CERTIFICATE-----\n",
  "PrivateKeyPEM": "-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIJdfnKBqAYcylBqC7lPVDGkCPGbehbTJRZ2VHw2BRDDcoAoGCCqGSM49\nAwEHoUQDQgAEpDOZ1w2DSqoU3WngHjdFlFbL7/HFqKtpR1Q3AK2xs0NrkDm3nF37\nHB0GTO4xQ7dxeoKbCQCJKCjZYQThTnxjqA==\n-----END EC PRIVATE KEY-----\n",
  "Service": "web",
  "ServiceURI": "spiffe://f2927846-83c2-d5e3-afcb-2e87c1a72ab5.consul/ns/default/dc/gke/svc/web",
  "ValidAfter": "2020-06-23T23:00:12Z",
  "ValidBefore": "2020-06-26T23:00:12Z",
  "CreateIndex": 139471,
  "ModifyIndex": 139471
}

Vault CA Response:

{
  "SerialNumber": "11:43:31:66:2f:2f:3f:08:a9:8d:1f:9a:1b:5c:85:8e:87:32:68:7f",
  "CertPEM": "-----BEGIN CERTIFICATE-----\nMIICSzCCAfGgAwIBAgIUEUMxZi8vPwipjR+aG1yFjocyaH8wCgYIKoZIzj0EAwIw\nMDEuMCwGA1UEAxMlcHJpLWRkaHVneWFqLnZhdWx0LmNhLmZkZWEwZTFjLmNvbnN1\nbDAeFw0yMDA2MjMyMjQxNThaFw0yMDA2MjYyMjQyMjhaMCoxKDAmBgNVBAMTH3dl\nYi5zdmMuZGVmYXVsdC5mZGVhMGUxYy5jb25zdWwwWTATBgcqhkjOPQIBBggqhkjO\nPQMBBwNCAASX8Jf0VUHewpF5YOsBXbhpixoD5j1Kw84ydIVJ275zDYxUP3n29n/2\ncc+VHCS2Y9qUlxGwmJ5ChLX2bPT0m7bGo4HuMIHrMA4GA1UdDwEB/wQEAwIDqDAd\nBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwHQYDVR0OBBYEFEU7XhcLFA9l\n4kLBU4Qe2n36cDb/MB8GA1UdIwQYMBaAFIcoX+VqYXPQc4abedzlelzHvoZTMHoG\nA1UdEQRzMHGCH3dlYi5zdmMuZGVmYXVsdC5mZGVhMGUxYy5jb25zdWyGTnNwaWZm\nZTovL2ZkZWEwZTFjLTMwNTktMzcxYi01MmM5LTBlNTYzODMzODdhYy5jb25zdWwv\nbnMvZGVmYXVsdC9kYy9kYzEvc3ZjL3dlYjAKBggqhkjOPQQDAgNIADBFAiEA6Phw\nMBfL+zGaetYa4I9eOzdpe6C6FIi5YOWyBKoCwmQCIDN2zh1h96I723ZTNHWQX8I6\n7nohJUZDqaQwchdYs6qv\n-----END CERTIFICATE-----\n-----BEGIN CERTIFICATE-----\nMIICMDCCAdWgAwIBAgIUKbnVhLfR1QqZbsrR36GEvVfD/qcwCgYIKoZIzj0EAwIw\nMDEuMCwGA1UEAxMlcHJpLTEzejZjaW53LnZhdWx0LmNhLjQ4NWM1NmYxLmNvbnN1\nbDAeFw0yMDA2MTkxOTM4NTVaFw0yMDA3MjExOTM5MjVaMDAxLjAsBgNVBAMTJXBy\naS1kZGh1Z3lhai52YXVsdC5jYS5mZGVhMGUxYy5jb25zdWwwWTATBgcqhkjOPQIB\nBggqhkjOPQMBBwNCAASdM6luo5s0SD8fqMDFfAiqJK+2dA1K3u3+OQWnAueGFHEN\nFs7Efpg1J3uBJ1Ocn7adbcezkv74Lf9i60uraeUfo4HMMIHJMA4GA1UdDwEB/wQE\nAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSHKF/lamFz0HOGm3nc5Xpc\nx76GUzAfBgNVHSMEGDAWgBRp8wq+AlA8yv0eJiVTMCtXHOIUoTBmBgNVHREEXzBd\ngiVwcmktZGRodWd5YWoudmF1bHQuY2EuZmRlYTBlMWMuY29uc3VshjRzcGlmZmU6\nLy9mZGVhMGUxYy0zMDU5LTM3MWItNTJjOS0wZTU2MzgzMzg3YWMuY29uc3VsMAoG\nCCqGSM49BAMCA0kAMEYCIQDCzyBirCUvzuiN8b6E5ThH3szcCHdumHMUbUAhVVIc\ndwIhAI8IytsXoQSYRk/wRvlO811Gs2QXhA4+eVVyq062qVl9\n-----END CERTIFICATE-----\n-----BEGIN CERTIFICATE-----\nMIICMDCCAdWgAwIBAgIUKbnVhLfR1QqZbsrR36GEvVfD/qcwCgYIKoZIzj0EAwIw\nMDEuMCwGA1UEAxMlcHJpLTEzejZjaW53LnZhdWx0LmNhLjQ4NWM1NmYxLmNvbnN1\nbDAeFw0yMDA2MTkxOTM4NTVaFw0yMDA3MjExOTM5MjVaMDAxLjAsBgNVBAMTJXBy\naS1kZGh1Z3lhai52YXVsdC5jYS5mZGVhMGUxYy5jb25zdWwwWTATBgcqhkjOPQIB\nBggqhkjOPQMBBwNCAASdM6luo5s0SD8fqMDFfAiqJK+2dA1K3u3+OQWnAueGFHEN\nFs7Efpg1J3uBJ1Ocn7adbcezkv74Lf9i60uraeUfo4HMMIHJMA4GA1UdDwEB/wQE\nAwIBBjAPBgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBSHKF/lamFz0HOGm3nc5Xpc\nx76GUzAfBgNVHSMEGDAWgBRp8wq+AlA8yv0eJiVTMCtXHOIUoTBmBgNVHREEXzBd\ngiVwcmktZGRodWd5YWoudmF1bHQuY2EuZmRlYTBlMWMuY29uc3VshjRzcGlmZmU6\nLy9mZGVhMGUxYy0zMDU5LTM3MWItNTJjOS0wZTU2MzgzMzg3YWMuY29uc3VsMAoG\nCCqGSM49BAMCA0kAMEYCIQDCzyBirCUvzuiN8b6E5ThH3szcCHdumHMUbUAhVVIc\ndwIhAI8IytsXoQSYRk/wRvlO811Gs2QXhA4+eVVyq062qVl9\n-----END CERTIFICATE-----",
  "PrivateKeyPEM": "-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIN+PbyPwBYVceu3QaC35u0biIeICVSNctOE6y6izRwujoAoGCCqGSM49\nAwEHoUQDQgAEl/CX9FVB3sKReWDrAV24aYsaA+Y9SsPOMnSFSdu+cw2MVD959vZ/\n9nHPlRwktmPalJcRsJieQoS19mz09Ju2xg==\n-----END EC PRIVATE KEY-----\n",
  "Service": "web",
  "ServiceURI": "spiffe://fdea0e1c-3059-371b-52c9-0e56383387ac.consul/ns/default/dc/dc1/svc/web",
  "ValidAfter": "2020-06-23T22:41:58Z",
  "ValidBefore": "2020-06-26T22:42:28Z",
  "CreateIndex": 140001,
  "ModifyIndex": 140001
}

Reproduction Steps

1. Stand up vault and unseal it
2. Run consul with the config

   {
     "connect": {
       "ca_provider": "vault",
       "ca_config": {
         "address": "http://localhost:8200",
         "token": "<your-token>",
         "root_pki_path": "connect-root",
         "intermediate_pki_path": "connect-intermediate"
       }
     }
   }

3. curl http://127.0.0.1:8500/v1/agent/connect/ca/leaf/web

[mikemorris]

Went digging to see if trailing newline was defined in the spec for how keys should be formatted, found

Generators MUST wrap the base64-encoded lines so that each line
consists of exactly 64 characters except for the final line, which
will encode the remainder of the data (within the 64-character line
boundary), and they MUST NOT emit extraneous whitespace.
https://tools.ietf.org/html/rfc7468#section-2

"whitespace" means any character or series of characters that
represent horizontal or vertical space in typography

And also the note that "most extant parsers ignore blanks at the ends of lines".

I think the proper implementation would be to always trim trailing whitespace at the point of generation (would that be in Vault itself or is Consul inserting additional whitespace here?) and the consumer should trim any whitespace from keys received (Consul could do this, but downstream consumers of Consul's API should too) and always insert newlines between keys when concatenating together, similar to the implementation in nginx/kubernetes-ingress#3

Keys without trailing whitespace should be expected, but trimming unexpected whitespace when present should be acceptable.

[dnephin]

The trailing newline comes from the go stdlib pem.Encode(): https://github.com/golang/go/blob/go1.14.4/src/encoding/pem/pem.go#L325. The implementation has not changed since 2014, and I don't see any issues about it, which makes me believe that it is correct. Maybe "extraneous" means, "more than the expected single newline"?

The Vault CA provider does not add a trailing newline to the data it gets from vault:

https://github.com/hashicorp/consul/blob/v1.8.0/agent/connect/ca/provider_vault.go#L358

Another difference between these 2 cases is that the built-in CA example has only a single cert. The vault example has 3 certificates. If some of those are intermediate certs, it could be this code: https://github.com/hashicorp/consul/blob/v1.8.0/agent/consul/connect_ca_endpoint.go#L544-L559, which correctly trims to only a single newline between certs, but does not add a trailing newline.

[mikemorris]

Sooo, this appears to become more of a problem when we're the consumer and concatenating these certs together at https://github.com/hashicorp/consul/blob/master/agent/connect_ca_endpoint.go#L41, such as when migrating from the built-in Consul CA to a Vault CA, resulting in a line -----END CERTIFICATE----------BEGIN CERTIFICATE----- that Envoy then fails to parse causing a fatal error during config load time 😞

Refs #8774

[roberteckert]

Not to crash the conversation, but I brought this up with a few Hashicorp folks recently, and figured I would throw in my $.02.

I think Mike has the right idea here. My reading of the relevant RFCs surrounding the PEM format (8555#9.1 and 7468#3 for ABNF spec) implies that there exists at least one newline between each root's body. When provided a cross-signed certificate, I expect it to conform to the following standard (lifted from the RFCs):

  ; From 7468#3
  stricttextualmsg = preeb eol
                     strictbase64text
                     posteb eol

  ; From 8555#9.1
  certchain = stricttextualmsg *(eol stricttextualmsg)

This would imply, to me, that there must exist a terminal newline (CRLF/CR/LF) at the end of each certificate, and another when concatenating. Mike linked to the line that I found (and blamed) during my debugging; but, at the time I could not tell if that was indeed the place from whence Envoy was getting its problematic configuration. It would seem that around https://github.com/hashicorp/consul/blob/master/agent/connect_ca_endpoint.go#L41, ensuring that there is a terminal newline with a concatenating newline would be to spec. Ensuring that there's exactly one newline will probably work for most parsers, but wouldn't exactly be to spec, if I were to hazard a guess.