Repository: knative/pkg
Pull request #1998 (2021-01-22 02:07:00)

Switch Webhook to ed25519 and expiration of the certificate to seven days
# Changes

<!-- 
Describe your changes here- ideally you can get that description straight from
your descriptive commit message(s)! 

- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic
-->

- :broom: Webhook uses `ed25519` rather than RSA 2048.
- :broom: Change the expiration of the certificate to seven days and the recreation time to one day before expiration.

<!--
In addition, categorize the changes you're making using the "/kind" Prow command, example:

/kind <kind>

Supported kinds are: api-change, bug, cleanup, deprecation, removal, documentation, enhancement, performance

-->
/kind enhancement


<!-- Please include the 'why' behind your changes if no issue exists -->

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->
```release-note
NONE
```

**Docs**

<!--
:book: If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [knative/docs]: <issue or pr link>
- [Feature Track]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```


knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1998#" title="Author self-approved">Harwayne</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/1998?src=pr&el=h1) Report
> Merging [#1998](https://codecov.io/gh/knative/pkg/pull/1998?src=pr&el=desc) (c3379f9) into [master](https://codecov.io/gh/knative/pkg/commit/32a3248a71216b96ff79a52fdafc8d26e9cbdb4a?el=desc) (32a3248) will **decrease** coverage by `0.06%`.
> The diff coverage is `69.23%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/1998/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO)](https://codecov.io/gh/knative/pkg/pull/1998?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1998      +/-   ##
==========================================
- Coverage   67.29%   67.23%   -0.07%     
==========================================
  Files         214      214              
  Lines        9061     9065       +4     
==========================================
- Hits         6098     6095       -3     
- Misses       2693     2697       +4     
- Partials      270      273       +3     
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/1998?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [webhook/certificates/certificates.go](https://codecov.io/gh/knative/pkg/pull/1998/diff?src=pr&el=tree#diff-d2ViaG9vay9jZXJ0aWZpY2F0ZXMvY2VydGlmaWNhdGVzLmdv) | `69.69% <0.00%> (ø)` | |
| [webhook/certificates/resources/certs.go](https://codecov.io/gh/knative/pkg/pull/1998/diff?src=pr&el=tree#diff-d2ViaG9vay9jZXJ0aWZpY2F0ZXMvcmVzb3VyY2VzL2NlcnRzLmdv) | `60.71% <72.72%> (-1.79%)` | :arrow_down: |
| [webhook/certificates/resources/secret.go](https://codecov.io/gh/knative/pkg/pull/1998/diff?src=pr&el=tree#diff-d2ViaG9vay9jZXJ0aWZpY2F0ZXMvcmVzb3VyY2VzL3NlY3JldC5nbw==) | `85.71% <100.00%> (ø)` | |
| [test/gcs/mock/mock.go](https://codecov.io/gh/knative/pkg/pull/1998/diff?src=pr&el=tree#diff-dGVzdC9nY3MvbW9jay9tb2NrLmdv) | `90.32% <0.00%> (-2.16%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/1998?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/1998?src=pr&el=footer). Last update [32a3248...6dfdf99](https://codecov.io/gh/knative/pkg/pull/1998?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1998#pullrequestreview-574582648" title="Approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/1998#" title="Author self-approved">Harwayne</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dprotaso: /lgtm
/approve
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1998#issuecomment-765656522" title="Approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/1998#" title="Author self-approved">Harwayne</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/webhook/certificates/certificates.go b/webhook/certificates/certificates.go
index 95a9633b1..5239279e5 100644
--- a/webhook/certificates/certificates.go
+++ b/webhook/certificates/certificates.go
@@ -36,7 +36,7 @@ import (
 
 const (
 	// Time used for updating a certificate before it expires.
-	oneWeek = 7 * 24 * time.Hour
+	oneDay = 24 * time.Hour
 )
 
 type reconciler struct {
@@ -89,7 +89,7 @@ func (r *reconciler) reconcileCertificate(ctx context.Context) error {
 			certData, err := x509.ParseCertificate(cert.Certificate[0])
 			if err != nil {
 				logger.Errorw("Error parsing certificate", zap.Error(err))
-			} else if time.Now().Add(oneWeek).Before(certData.NotAfter) {
+			} else if time.Now().Add(oneDay).Before(certData.NotAfter) {
 				return nil
 			}
 		}
diff --git a/webhook/certificates/resources/certs.go b/webhook/certificates/resources/certs.go
index 3b1486469..c7ab8f6a3 100644
--- a/webhook/certificates/resources/certs.go
+++ b/webhook/certificates/resources/certs.go
@@ -18,8 +18,8 @@ package resources
 
 import (
 	"context"
+	"crypto/ed25519"
 	"crypto/rand"
-	"crypto/rsa"
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/pem"
@@ -62,7 +62,7 @@ func createCertTemplate(name, namespace string, notAfter time.Time) (*x509.Certi
 			Organization: []string{organization},
 			CommonName:   commonName,
 		},
-		SignatureAlgorithm:    x509.SHA256WithRSA,
+		SignatureAlgorithm:    x509.PureEd25519,
 		NotBefore:             time.Now(),
 		NotAfter:              notAfter,
 		BasicConstraintsValid: true,
@@ -112,9 +112,9 @@ func createCert(template, parent *x509.Certificate, pub, parentPriv interface{})
 	return
 }
 
-func createCA(ctx context.Context, name, namespace string, notAfter time.Time) (*rsa.PrivateKey, *x509.Certificate, []byte, error) {
+func createCA(ctx context.Context, name, namespace string, notAfter time.Time) (ed25519.PrivateKey, *x509.Certificate, []byte, error) {
 	logger := logging.FromContext(ctx)
-	rootKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	publicKey, privateKey, err := ed25519.GenerateKey(rand.Reader)
 	if err != nil {
 		logger.Errorw("error generating random key", zap.Error(err))
 		return nil, nil, nil, err
@@ -126,12 +126,12 @@ func createCA(ctx context.Context, name, namespace string, notAfter time.Time) (
 		return nil, nil, nil, err
 	}
 
-	rootCert, rootCertPEM, err := createCert(rootCertTmpl, rootCertTmpl, &rootKey.PublicKey, rootKey)
+	rootCert, rootCertPEM, err := createCert(rootCertTmpl, rootCertTmpl, publicKey, privateKey)
 	if err != nil {
 		logger.Errorw("error signing the CA cert", zap.Error(err))
 		return nil, nil, nil, err
 	}
-	return rootKey, rootCert, rootCertPEM, nil
+	return privateKey, rootCert, rootCertPEM, nil
 }
 
 // CreateCerts creates and returns a CA certificate and certificate and
@@ -148,7 +148,7 @@ func CreateCerts(ctx context.Context, name, namespace string, notAfter time.Time
 	}
 
 	// Then create the private key for the serving cert
-	servKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	publicKey, privateKey, err := ed25519.GenerateKey(rand.Reader)
 	if err != nil {
 		logger.Errorw("error generating random key", zap.Error(err))
 		return nil, nil, nil, err
@@ -160,13 +160,18 @@ func CreateCerts(ctx context.Context, name, namespace string, notAfter time.Time
 	}
 
 	// create a certificate which wraps the server's public key, sign it with the CA private key
-	_, servCertPEM, err := createCert(servCertTemplate, caCertificate, &servKey.PublicKey, caKey)
+	_, servCertPEM, err := createCert(servCertTemplate, caCertificate, publicKey, caKey)
 	if err != nil {
 		logger.Errorw("error signing server certificate template", zap.Error(err))
 		return nil, nil, nil, err
 	}
+	privKeyBytes, err := x509.MarshalPKCS8PrivateKey(privateKey)
+	if err != nil {
+		logger.Errorw("error marshaling private key", zap.Error(err))
+		return nil, nil, nil, err
+	}
 	servKeyPEM := pem.EncodeToMemory(&pem.Block{
-		Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(servKey),
+		Type: "PRIVATE KEY", Bytes: privKeyBytes,
 	})
 	return servKeyPEM, servCertPEM, caCertificatePEM, nil
 }
diff --git a/webhook/certificates/resources/secret.go b/webhook/certificates/resources/secret.go
index 48e57e870..6fad6629f 100644
--- a/webhook/certificates/resources/secret.go
+++ b/webhook/certificates/resources/secret.go
@@ -32,6 +32,8 @@ const (
 	// CACert is the name of the key associated with the certificate of the CA for
 	// the keypair.
 	CACert = "ca-cert.pem"
+
+	oneWeek = 7 * 24 * time.Hour
 )
 
 // MakeSecret synthesizes a Kubernetes Secret object with the keys specified by
@@ -41,7 +43,7 @@ var MakeSecret = MakeSecretInternal
 
 // MakeSecretInternal is only public so MakeSecret can be restored in testing.  Use MakeSecret.
 func MakeSecretInternal(ctx context.Context, name, namespace, serviceName string) (*corev1.Secret, error) {
-	serverKey, serverCert, caCert, err := CreateCerts(ctx, serviceName, namespace, time.Now().AddDate(1, 0, 0))
+	serverKey, serverCert, caCert, err := CreateCerts(ctx, serviceName, namespace, time.Now().Add(oneWeek))
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/webhook/certificates/certificates_test.go b/webhook/certificates/certificates_test.go
index 423985cb0..a6369081a 100644
--- a/webhook/certificates/certificates_test.go
+++ b/webhook/certificates/certificates_test.go
@@ -126,16 +126,16 @@ func TestReconcile(t *testing.T) {
 	}, {
 		Name: "certificate expiring soon",
 		Key:  key,
-		// 6 days falls inside of the grace period of 7 days so the secret will be updated
-		Objects: []runtime.Object{secretWithCertData(t, time.Now().Add(6*24*time.Hour))},
+		// 23 hours  falls inside of the grace period of 1 day so the secret will be updated.
+		Objects: []runtime.Object{secretWithCertData(t, time.Now().Add(23*time.Hour))},
 		WantUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: secret,
 		}},
 	}, {
 		Name: "certificate not expiring soon",
 		Key:  key,
-		// 8 days falls outside of the grace period of 7 days so the secret will not be updated
-		Objects: []runtime.Object{secretWithCertData(t, time.Now().Add(8*24*time.Hour))},
+		// 25 hours falls outside of the grace period of 1 day so the secret will not be updated.
+		Objects: []runtime.Object{secretWithCertData(t, time.Now().Add(25*time.Hour))},
 	}}
 
 	table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler {
diff --git a/webhook/certificates/resources/certs_test.go b/webhook/certificates/resources/certs_test.go
index e2fc3f881..49c976f0f 100644
--- a/webhook/certificates/resources/certs_test.go
+++ b/webhook/certificates/resources/certs_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package resources
 
 import (
+	"crypto/ed25519"
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
@@ -29,22 +30,22 @@ import (
 )
 
 func TestCreateCerts(t *testing.T) {
-	sKey, serverCertPEM, caCertBytes, err := CreateCerts(TestContextWithLogger(t), "got-the-hook", "knative-webhook", time.Now().AddDate(1, 0, 0))
+	sKey, serverCertPEM, caCertBytes, err := CreateCerts(TestContextWithLogger(t), "got-the-hook", "knative-webhook", time.Now().AddDate(0, 0, 7))
 	if err != nil {
 		t.Fatal("Failed to create certs", err)
 	}
 
 	// Test server private key
 	p, _ := pem.Decode(sKey)
-	if p.Type != "RSA PRIVATE KEY" {
-		t.Fatal("Expected the key to be RSA Private key type")
+	if p.Type != "PRIVATE KEY" {
+		t.Fatal("Expected the key to be Private key type")
 	}
-	key, err := x509.ParsePKCS1PrivateKey(p.Bytes)
+	key, err := x509.ParsePKCS8PrivateKey(p.Bytes)
 	if err != nil {
 		t.Fatal("Failed to parse private key", err)
 	}
-	if err := key.Validate(); err != nil {
-		t.Fatalf("Failed to validate private key")
+	if _, ok := key.(ed25519.PrivateKey); !ok {
+		t.Fatalf("Key is not ed25519 format, actually %t", key)
 	}
 
 	// Test Server Cert
@@ -98,7 +99,7 @@ func validCertificate(cert []byte, t *testing.T) (*x509.Certificate, error) {
 	if err != nil {
 		return nil, fmt.Errorf("Failed to parse cert %w", err)
 	}
-	if parsedCert.SignatureAlgorithm != x509.SHA256WithRSA {
+	if parsedCert.SignatureAlgorithm != x509.PureEd25519 {
 		return nil, fmt.Errorf("Failed to match signature. Got: %s, want: %s", parsedCert.SignatureAlgorithm, x509.SHA256WithRSA)
 	}
 	return parsedCert, nil