Skip to content

Commit

Permalink
[Heartbeat] Handle TLS certs missing notBefore/notAfter (#9566) (#9758)
Browse files Browse the repository at this point in the history
* [Heartbeat] Handle TLS certs missing notBefore/notAfter

Some certs in the wild don't set these standard fields and can cause an NPE

Fixes #9556

* Add changelog entry

(cherry picked from commit 337113e)
  • Loading branch information
andrewvc committed Jan 2, 2019
1 parent 55bf40e commit 1bf7558
Show file tree
Hide file tree
Showing 3 changed files with 171 additions and 22 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
Expand Up @@ -73,6 +73,7 @@ https://github.com/elastic/beats/compare/v6.6.0...6.x[Check the HEAD diff]
*Heartbeat*

- Heartbeat now always downloads the entire body of HTTP endpoints, even if no checks against the body content are declared. This fixes an issue where timing metrics would be incorrect in scenarios where the body wasn't used since the connection would be closed soon after the headers were sent, but before the entire body was. {pull}8894[8894]
- Fixed rare issue where TLS connections to endpoints with x509 certificates missing either notBefore or notAfter would cause the check to fail with a stacktrace. {pull}9566[9566]

*Journalbeat*

Expand Down
69 changes: 47 additions & 22 deletions heartbeat/monitors/active/dialchain/tls.go
Expand Up @@ -19,12 +19,14 @@ package dialchain

import (
cryptoTLS "crypto/tls"
"crypto/x509"
"fmt"
"net"
"time"

"github.com/elastic/beats/heartbeat/look"
"github.com/elastic/beats/libbeat/beat"
"github.com/elastic/beats/libbeat/common"
"github.com/elastic/beats/libbeat/outputs/transport"
)

Expand Down Expand Up @@ -60,30 +62,53 @@ func TLSLayer(cfg *transport.TLSConfig, to time.Duration) Layer {
timer.stop()
event.PutValue("tls.rtt.handshake", look.RTT(timer.duration()))

// Pointers because we need a nil value
var chainNotValidBefore *time.Time
var chainNotValidAfter *time.Time

// Here we compute the minimal bounds during which this certificate chain is valid
// To do this correctly, we take the maximum NotBefore and the minimum NotAfter.
// This *should* always wind up being the terminal cert in the chain, but we should
// compute this correctly.
for _, chain := range tlsConn.ConnectionState().VerifiedChains {
for _, cert := range chain {
if chainNotValidBefore == nil || chainNotValidBefore.Before(cert.NotBefore) {
chainNotValidBefore = &cert.NotBefore
}

if chainNotValidAfter == nil || chainNotValidAfter.After(cert.NotAfter) {
chainNotValidAfter = &cert.NotAfter
}
}
}

event.PutValue("tls.certificate_not_valid_before", *chainNotValidBefore)
event.PutValue("tls.certificate_not_valid_after", *chainNotValidAfter)
addCertMetdata(event.Fields, tlsConn.ConnectionState().VerifiedChains)

return conn, nil
}), nil
}
}

func addCertMetdata(fields common.MapStr, chains [][]*x509.Certificate) {
// The behavior here might seem strange. We *always* set a notBefore, but only optionally set a notAfter.
// Why might we do this?
// The root cause is that the x509.Certificate type uses time.Time for these fields instead of *time.Time
// so we have no way to know if the user actually set these fields. The x509 RFC says that only one of the
// two fields must be set. Most tools (including openssl and go's certgen) always set both. BECAUSE WHY NOT
//
// In the wild, however, there are certs missing one of these two fields.
// So, what's the correct behavior here? We cannot know if a field was omitted due to the lack of nullability.
// So, in this case, we try to do what people will want 99.99999999999999999% of the time.
// People might set notBefore to go's zero date intentionally when creating certs. So, we always set that
// field, even if we find a zero value.
// However, it would be weird to set notAfter to the zero value. That could invalidate a cert that was intended
// to be valid forever. So, in that case, we treat the zero value as non-existent.
// This is why notBefore is a time.Time and notAfter is a *time.Time
var chainNotValidBefore time.Time
var chainNotValidAfter *time.Time

// We need the zero date later
var zeroTime time.Time

// Here we compute the minimal bounds during which this certificate chain is valid
// To do this correctly, we take the maximum NotBefore and the minimum NotAfter.
// This *should* always wind up being the terminal cert in the chain, but we should
// compute this correctly.
for _, chain := range chains {
for _, cert := range chain {
if chainNotValidBefore.Before(cert.NotBefore) {
chainNotValidBefore = cert.NotBefore
}

if cert.NotAfter != zeroTime && (chainNotValidAfter == nil || chainNotValidAfter.After(cert.NotAfter)) {
chainNotValidAfter = &cert.NotAfter
}
}
}

fields.Put("tls.certificate_not_valid_before", chainNotValidBefore)

if chainNotValidAfter != nil {
fields.Put("tls.certificate_not_valid_after", *chainNotValidAfter)
}
}
123 changes: 123 additions & 0 deletions heartbeat/monitors/active/dialchain/tls_test.go
@@ -0,0 +1,123 @@
// Licensed to Elasticsearch B.V. under one or more contributor
// license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright
// ownership. Elasticsearch B.V. licenses this file to you under
// the Apache License, Version 2.0 (the "License"); you may
// not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

package dialchain

import (
"crypto/x509"
"crypto/x509/pkix"
"math/big"
"testing"
"time"

"github.com/stretchr/testify/assert"

"github.com/elastic/beats/libbeat/common"
)

func Test_addCertMetdata(t *testing.T) {
goodNotBefore := time.Now().Add(-time.Hour)
goodNotAfter := time.Now().Add(time.Hour)
goodCert := x509.Certificate{
SerialNumber: big.NewInt(1),
Subject: pkix.Name{
Organization: []string{"Acme Co"},
},
NotBefore: goodNotBefore,
NotAfter: goodNotAfter,
KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
BasicConstraintsValid: true,
}

missingNotBeforeCert := x509.Certificate{
SerialNumber: big.NewInt(1),
Subject: pkix.Name{
Organization: []string{"Acme Co"},
},
NotAfter: goodNotAfter,
KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
BasicConstraintsValid: true,
}

missingNotAfterCert := x509.Certificate{
SerialNumber: big.NewInt(1),
Subject: pkix.Name{
Organization: []string{"Acme Co"},
},
NotBefore: goodNotBefore,
KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
BasicConstraintsValid: true,
}

// notBefore is intentionally not a pointer type because go certificates don't have nullable time types
// we cheat a bit and make not after nullable because there's no valid reason to create a cert with go's zero
// time.
// see the addCertMetadata function for more info on this.
type expected struct {
notBefore time.Time
notAfter *time.Time
}
tests := []struct {
name string
chains [][]*x509.Certificate
expected expected
}{
{
"Valid cert",
[][]*x509.Certificate{{&goodCert}},
expected{
notBefore: goodNotBefore,
notAfter: &goodNotAfter,
},
},
{
"Missing not before",
[][]*x509.Certificate{{&missingNotBeforeCert}},
expected{
notAfter: &goodNotAfter,
},
},
{
"Missing not after",
[][]*x509.Certificate{{&missingNotAfterCert}},
expected{
notBefore: goodNotBefore,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
event := common.MapStr{}
addCertMetdata(event, tt.chains)
v, err := event.GetValue("tls.certificate_not_valid_before")
assert.NoError(t, err)
assert.Equal(t, tt.expected.notBefore, v)

if tt.expected.notAfter != nil {
v, err := event.GetValue("tls.certificate_not_valid_after")
assert.NoError(t, err)
assert.Equal(t, *tt.expected.notAfter, v)
} else {
ok, _ := event.HasKey("tls.certificate_not_valid_after")
assert.False(t, ok, "event should not have not after %v", event)
}
})
}
}

0 comments on commit 1bf7558

Please sign in to comment.