-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
Broke via ory/x#601
…via a channel The previous interface required passing two different contexts which is quite confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please always return the error if it is available. In a possible scenario, someone could accidentally overwrite the hook to not fatal, which would then end up in incorrectly returning a nil error, most likely causing a panic somewhere down the line. Thanks!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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