Add certificate TLS verification mode #20293
Conversation
botelastic
bot
added
the
needs_team
| option for testing only. | ||
|
|
||
| The default is `full`. | ||
| Controls the verification of certificates. Valid values are: |
There was a problem hiding this comment.
This is cribbed from the ES docs: https://github.com/elastic/elasticsearch/blob/7.8/docs/reference/settings/common-defs.asciidoc
There was a problem hiding this comment.
It's also not clear to me where I should update this section of the example config. I see there's multiple tmpl files with identical content, do I edit each one?
There was a problem hiding this comment.
@andrewkroh wondering the same here. Do we collect the SSL settings from one common template, or do we need the templates to include SSL settings?
There was a problem hiding this comment.
In #20357 I created a template for the SSL options and then included it in each output config. With this change you'll only need to update the one template.
| @@ -80,15 +86,12 @@ func (c *TLSConfig) ToConfig() *tls.Config { | |||
| minVersion, maxVersion := extractMinMaxVersion(c.Versions) | |||
| insecure := c.Verification != VerifyFull | |||
| if insecure { | |||
| logp.NewLogger("tls").Warn("SSL/TLS verifications disabled.") | |||
| logp.NewLogger("tls").Warn("Some SSL/TLS verifications disabled.") | |||
There was a problem hiding this comment.
It's not clear to me that this is a useful log message, it is very spammy
There was a problem hiding this comment.
maybe log only if insecure && verifyPeerCertFn == nil
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Test errorsExpand to view the tests failures
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
|
The docs build is failing because of
but I didn't change any links (though added the issue link in the changelog), so it's not clear to me why this is breaking |
andresrc
added
the
Team:Services
|
Pinging @elastic/integrations-services (Team:Services) |
botelastic
bot
removed
the
needs_team
|
Thanks @anyasabo for the PR!! |
|
|
||
| // Time returns the current time as the number of seconds since the epoch. | ||
| // If Time is nil, TLS uses time.Now. | ||
| Time func() time.Time |
There was a problem hiding this comment.
Do we really need to export this setting? Looks like it is only needed for internal testing.
There was a problem hiding this comment.
It's exported in the crypto/tls.Config struct so I also exported it here for symmetry, but you're right it currently is only used for internal testing. I could see it being used by tests at higher levels, but that's something we could revisit in the future if someone ever wants to write tests like that. I'll go ahead and unexport it.
| continue | ||
| } | ||
| opts.Intermediates.AddCert(cert) | ||
| } |
There was a problem hiding this comment.
more idiomatic:
for _, cert := range certs[1:] {
opts.Intermeduates.AddCert(cert)
}
There was a problem hiding this comment.
Ah good catch, this part was a copy and paste job from our repo I should have caught. And it looks like upstream has it this way too so I don't know why we did it all silly https://github.com/golang/go/blob/master/src/crypto/tls/handshake_client.go#L842
There was a problem hiding this comment.
in case it is copied from the golang/go code base we need to update the file header licence info as well. maybe we can move it to a separate verify.go file?
There was a problem hiding this comment.
For this i just copied it from the ECK repo, which should be okay since we can relicense code we wrote without issue. That's a good catch though and I'm not sure of its provenance. It looks like we added it here elastic/cloud-on-k8s#929 but unfortunately the author is out
There was a problem hiding this comment.
Looking at it more closely, the requirements in the golang license are just a copyright notice, which is already included since this package imports crypto/tls, so I think even if we copied the code line by line (which I do not expect is the case) we are good as is? I am not a lawyer though.
There was a problem hiding this comment.
The go repo is BSD licensed. I think BSD requires copies to include a copyright notice.
There was a problem hiding this comment.
@urso it looks like having the golang header in verify.go causes the license header CI check to fail. It's not clear to me that the header is required. From the golang src:
// * Redistributions of source code must retain the above copyright
// notice, this list of conditions and the following disclaimer.
which I believe is covered by including it in the NOTICE.txt at the root of the repo.
|
|
||
| // defer to the default verification performed | ||
| chains, err := headCert.Verify(opts) | ||
| return certs, chains, errors.WithStack(err) |
There was a problem hiding this comment.
Do we need the full stack trace here? The error will be presented to users in log messages. Can we create/ensure a more informative message why validation failed?
There was a problem hiding this comment.
Verify provides useful messages as is -- you can change around the unit test cases' wantErr value so the tests fail and see some examples quickly -- so we may just be able to skip wrapping the error here. Adding stacks when calling out to libraries that don't implement it is habit for me.
| @@ -80,15 +86,12 @@ func (c *TLSConfig) ToConfig() *tls.Config { | |||
| minVersion, maxVersion := extractMinMaxVersion(c.Versions) | |||
| insecure := c.Verification != VerifyFull | |||
| if insecure { | |||
| logp.NewLogger("tls").Warn("SSL/TLS verifications disabled.") | |||
| logp.NewLogger("tls").Warn("Some SSL/TLS verifications disabled.") | |||
There was a problem hiding this comment.
maybe log only if insecure && verifyPeerCertFn == nil
| option for testing only. | ||
|
|
||
| The default is `full`. | ||
| Controls the verification of certificates. Valid values are: |
There was a problem hiding this comment.
@andrewkroh wondering the same here. Do we collect the SSL settings from one common template, or do we need the templates to include SSL settings?
|
@dedemorton any idea why the docs build is failing? |
| for i, asn1Data := range rawCerts { | ||
| cert, err := x509.ParseCertificate(asn1Data) | ||
| if err != nil { | ||
| return nil, nil, errors.Wrap(err, "tls: failed to parse certificate from server") |
There was a problem hiding this comment.
If the goal is to avoid stack traces here then this will also have them, can change it to use %w wrapping if that's preferable
|
I think I addressed all of the current code comments, waiting on #20357 to be merged and then can finish the doc updates. |
|
jenkins run the tests please |
|
@anyasabo Change Looks good. The Lint on the CI intake is still failing. Looks like a license file header is missing. Calling |
|
I updated the license header in Still waiting on #20357 to make docs updates |
|
#20357 is merged now. |
|
jenkins run the tests please |
|
jenkins run the tests please |
|
Did have a look at the failing tests. There are a few flaky ones on travis. It looks like all dashboard loading tests are failing (beats CI users Kibana snapshot). Either something is broken in Beats master, or there was a breaking change in Kibana. All in all, all tls tests and output tests that use TLS are green. Let's stop rerunning CI. |
|
Thanks a lot @anyasabo |
urso
added
v7.10.0
and removed
needs_backport
(cherry picked from commit 5050283)
) Co-authored-by: Anya Sabo <[email protected]>
|
The option 'certificate' is in the docs, but cannot be used, is it normal ? is there a release date ? |
What does this PR do?
Adds
certificateTLS verification mode similar to Kibana and Elasticsearch, essentially skipping the hostname match.Why is it important?
This is especially useful in k8s-like environments where users may have a certificate in use for their publicly accessible host name, but use a different host name for intra cluster communication. For environments like this in Beats today our only option is to disable TLS verification entirely, but this allows us to keep the majority of verifications enabled without disabling it entirely.
We had to implement essentially the same functionality in ECK for reference, the main difference is that beats also implements cert pinning so there's a little bit of extra logic to combine the two custom verifications: https://github.com/elastic/cloud-on-k8s/blob/master/pkg/utils/cryptutil/tls_verify.go#L18
Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Author's Checklist
How to test this PR locally
This is something I had difficulty with since it wasn't clear to me how to build a new beat and then use it. I imagine we will want to add additional tests, but I added unit test coverage to start. This is my first libbeat PR and could definitely use guidance here.
Related issues
Closes #8164
Use cases
Screenshots
Logs