return auto_encrypt cert for listeners #6489
Conversation
| return c.manual.cert, nil | ||
| cert := c.manual.cert | ||
| if cert == nil { | ||
| cert = c.autoEncrypt.cert |
There was a problem hiding this comment.
can c.autoEncrypt ever be nil? If it is this will panic.
There was a problem hiding this comment.
NewConfigurator initializes it:
Lines 169 to 176 in ef52147
There was a problem hiding this comment.
Couple of points in tests but the code looks good.
One inline but a meta one: the significant behaviour change here is in GetCertificate yet the tests have no diff that mentions that method? How are we testing the actual change here that GetCertificate now returns auto TLS?
tlsutil/config_test.go
Outdated
| require.NoError(t, err) | ||
| require.Equal(t, c.autoEncrypt.cert, cert) | ||
|
|
||
| c.manual.cert = &tls.Certificate{Certificate: [][]byte{}} |
There was a problem hiding this comment.
Should we add some actual payload to this Certificate that means it can't accidentally compare equal to the auto one above due to deep-equals rather than exact pointer checking?
I'm actually not sure this assertion prooves anything currently because I think it would still pass even if we were returning the auto cert not the manual cert after this since they both have equivalent values and require.Equal does a deep equality not a pointer match IIRC.
Making them obviously different would make the intent of this test clearer too.
|
Thanks for the review @banks! I addressed your points and made the tests more meaningful. And improved the code as well. |
| @@ -351,7 +351,7 @@ func (c *Config) baseVerifyIncoming() bool { | |||
|
|
|||
| func loadKeyPair(certFile, keyFile string) (*tls.Certificate, error) { | |||
| if certFile == "" || keyFile == "" { | |||
| return &tls.Certificate{}, nil | |||
| return nil, nil | |||
There was a problem hiding this comment.
Returning an empty cert here led to all kinds of strange things. This is gone now for good.
| require.NoError(t, err) | ||
| require.Equal(t, c.autoEncrypt.cert, cert) | ||
|
|
||
| c2, err := loadKeyPair("../test/key/ourdomain.cer", "../test/key/ourdomain.key") |
There was a problem hiding this comment.
nit: this test looks good now but it might not be obvious to future us exactly what the signficance f this behaviour is.
Maybe add comments that adding one cert as the auto_encrypt one gets returned as the regular server cert above and then that loading a different one as well as the "manual" cert means we still server the manual one not the auto-encrypt one.
mikemorris
force-pushed
the
auto_encrypt_get_cert
branch
from
4ce1363
to
d28773e
Compare
for empty string certFile or keyFile arg
mikemorris
force-pushed
the
auto_encrypt_get_cert
branch
from
9528c02
to
9f18bc4
Compare
* fix cert check * fix lock * add tests * test: add comments describing expected behavior for auto-encrypt and manual certificates * test: expect nil *tls.Certificate for empty string certFile or keyFile arg
|
Hey there, This issue has been automatically locked because it is closed and there hasn't been any activity for at least 30 days. If you are still experiencing problems, or still have questions, feel free to open a new one 👍. |
ghost
locked and limited conversation to collaborators
Fixes #6403