Skip to content

Commit

Permalink
[Heartbeat] Handle TLS certs missing notBefore/notAfter (elastic#9566)
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 elastic#9556

* Add changelog entry
  • Loading branch information
andrewvc committed Dec 21, 2018
1 parent 2e92a04 commit 337113e
Show file tree
Hide file tree
Showing 3 changed files with 173 additions and 22 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d

*Heartbeat*

- 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*

*Metricbeat*
Expand Down
69 changes: 47 additions & 22 deletions heartbeat/monitors/active/dialchain/tls.go
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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 337113e

Please sign in to comment.