feat: hot-reload TLS certificate #3265
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3265 +/- ##
==========================================
+ Coverage 76.80% 76.85% +0.05%
==========================================
Files 123 123
Lines 8847 8875 +28
==========================================
+ Hits 6795 6821 +26
Misses 1627 1627
- Partials 425 427 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
alnr
changed the title
Broke via ory/x#601
…via a channel The previous interface required passing two different contexts which is quite confusing.
| priv, err := jwk.GetOrGenerateKeys(ctx, d, d.SoftwareKeyManager(), TlsKeyName, uuid.Must(uuid.NewV4()).String(), "RS256") | ||
| if err != nil { | ||
| d.Logger().WithError(err).Fatal("Unable to fetch or generate HTTPS TLS key pair") | ||
| return nil // in case Fatal is hooked |
There was a problem hiding this comment.
I think we should instead return the error
| } else if !errors.Is(err, tlsx.ErrNoCertificatesConfigured) { | ||
| d.Logger().WithError(err).Fatalf("Unable to load HTTPS TLS Certificate") | ||
| d.Logger().WithError(err).Fatal("Unable to load HTTPS TLS Certificate") | ||
| return nil // in case Fatal is hooked |
There was a problem hiding this comment.
I think we should instead return the error
| } | ||
|
|
||
| if len(priv.Certificates) == 0 { | ||
| cert, err := tlsx.CreateSelfSignedCertificate(priv.Key) | ||
| if err != nil { | ||
| d.Logger().WithError(err).Fatalf(`Could not generate a self signed TLS certificate`) | ||
| d.Logger().WithError(err).Fatal(`Could not generate a self signed TLS certificate`) | ||
| return nil // in case Fatal is hooked |
There was a problem hiding this comment.
I think we should instead return the error
| } | ||
|
|
||
| AttachCertificate(priv, cert) | ||
| if err := d.SoftwareKeyManager().DeleteKey(ctx, TlsKeyName, priv.KeyID); err != nil { | ||
| d.Logger().WithError(err).Fatal(`Could not update (delete) the self signed TLS certificate`) | ||
| return nil // in case Fatal is hooked |
There was a problem hiding this comment.
I think we should instead return the error
| } | ||
|
|
||
| if err := d.SoftwareKeyManager().AddKey(ctx, TlsKeyName, priv); err != nil { | ||
| d.Logger().WithError(err).Fatalf(`Could not update (add) the self signed TLS certificate: %s %x %d`, cert.SignatureAlgorithm, cert.Signature, len(cert.Signature)) | ||
| return nil // in case Fatalf is hooked |
There was a problem hiding this comment.
I think we should instead return the error
| } | ||
|
|
||
| pemCert := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: priv.Certificates[0].Raw}) | ||
| pemKey := pem.EncodeToMemory(block) | ||
| ct, err := tls.X509KeyPair(pemCert, pemKey) | ||
| if err != nil { | ||
| d.Logger().WithError(err).Fatalf("Could not decode certificate") | ||
| d.Logger().WithError(err).Fatal("Could not decode certificate") | ||
| return nil // in case Fatal is hooked |
There was a problem hiding this comment.
I think we should instead return the error
| } | ||
|
|
||
| if len(priv.Certificates) == 0 { | ||
| d.Logger().Fatal("TLS certificate chain can not be empty") | ||
| return nil // in case Fatal is hooked |
There was a problem hiding this comment.
I think we should instead return the error
| } | ||
| } | ||
|
|
||
| block, err := jwk.PEMBlockForKey(priv.Key) | ||
| if err != nil { | ||
| d.Logger().WithError(err).Fatalf("Could not encode key to PEM") | ||
| d.Logger().WithError(err).Fatal("Could not encode key to PEM") | ||
| return nil // in case Fatal is hooked |
There was a problem hiding this comment.
I think we should instead return the error
I did not change the interface semantics of Without the We have two options:
|
|
Ah, my bad, I overlooked that we return the certificate here and not the error. Ok, then this behavior is acceptable :) |
Automatic TLS certificate hot-reloading when the cert changes on disk.
#2850