Adding x509_cert input plugin by jtyr · Pull Request #3768 · influxdata/telegraf

jtyr · 2018-02-08T00:23:06Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

This PR is adding new input plugin which allows to monitor SSL certificates.

sepetrov

@jtyr , thanks for the contribution. As requested, I have reviewed the PR and my comments are mostly related to coding standards. Hope the maintainers don't mind that.

sepetrov · 2018-02-08T10:46:14Z

plugins/inputs/ssl_cert/ssl_cert.go

@@ -0,0 +1,155 @@
+package ssl_cert


I can see that some packages in the repository do not conform with the standards described in Effective Go - Package names , but I would suggest to drop the underscore.

~~ssl_cert~~ -> sslcert

I will keep the underscore in the package name because the name appears in the config file and I believe that it improves readability of the config file.

sepetrov · 2018-02-08T10:52:41Z

plugins/inputs/ssl_cert/ssl_cert_test.go

+AoQUXWIiFBoFfpNVcvUgHyGGc1hv7TX8Eh8KGo2+VuPzxnFuRrbbYCs=
+-----END RSA PRIVATE KEY-----`
+
+func getTestPrefix(testN int) string {


Seems like this function is not used.

sepetrov · 2018-02-08T11:01:40Z

plugins/inputs/ssl_cert/ssl_cert.go

+	for _, file := range sc.Files {
+		cert, err := getLocalCert(file)
+		if err != nil {
+			return errors.New(fmt.Sprintf("Cannot get local SSL cert: %s", err))


You could use fmt.Errorf.

Here is another alternative, which is not using fmt: errors.New("my error message: " + err.Error()). In this case you could remove the fmt package.

sepetrov · 2018-02-08T11:02:11Z

plugins/inputs/ssl_cert/ssl_cert.go

+	for _, server := range sc.Servers {
+		cert, err := getRemoteCert(server, sc.Timeout*time.Second, sc.CloseConn)
+		if err != nil {
+			return errors.New(fmt.Sprintf("Cannot get remote SSL cert: %s", err))


You could use fmt.Errorf

Error messages should not be treated as sentences:

should not start with uppercase unless the first word has special meaning, i.e. exported method/struct name etc.

should end with ., !, ? etc.

Think about the recommended error message style: [caller 2 error message]:[caller 1 error message]:[original error message]

See here

sepetrov · 2018-02-08T11:05:07Z

plugins/inputs/ssl_cert/ssl_cert.go

@@ -0,0 +1,155 @@
+package ssl_cert


The package description is missing. The exported package members also do not have any comments. Please see here or here.

I think this is not that important as it's quite clear what the package is (it's an input plugin) and further description is provided by the Description function.

sepetrov · 2018-02-08T11:09:28Z

plugins/inputs/ssl_cert/ssl_cert.go

+	// For tests
+	CloseConn bool
+}
+


I would recommend adding a check, which makes sure SSLCert implements telegraf.Input.

// make sure SSLCert implements telegraf.Input var _ telegraf.Input = &SSLCert{}

I would also consider it as a valid point, if someone prefers to have this check performed only in the _test.go file.

sepetrov · 2018-02-08T11:29:06Z

plugins/inputs/ssl_cert/ssl_cert.go

+	for _, server := range sc.Servers {
+		cert, err := getRemoteCert(server, sc.Timeout*time.Second, sc.CloseConn)
+		if err != nil {
+			return errors.New(fmt.Sprintf("Cannot get remote SSL cert: %s", err))


The error message could provide a little more information: consider including the server details. Currently neither all getRemoteCert possible errors include the server, nor the Gather method.

sepetrov · 2018-02-08T12:51:24Z

plugins/inputs/ssl_cert/ssl_cert.go

+	Timeout time.Duration `toml:"timeout"`
+
+	// For tests
+	CloseConn bool


Do not define interfaces on the implementor side of an API "for mocking"; instead, design the API so that it can be tested using the public API of the real implementation.

See here

I see no reason to test if the connection has been closed on your end. Instead I would test the scenarios under which a connection error could occur.

func TestSSLCert_Gather_ConnectionErrors(t *testing.T) { pair, err := tls.X509KeyPair([]byte(testCert), []byte(testKey)) if err != nil { t.Fatal(err) } config := &tls.Config{ Certificates: []tls.Certificate{pair}, } t.Run("server connection refused", func(t *testing.T) { ln, err := tls.Listen("tcp", ":0", config) if err != nil { t.Fatal(err) } svr := ln.Addr().String() ln.Close() sc := SSLCert{ Servers: []string{svr}, Timeout: 5, } acc := testutil.Accumulator{} err = sc.Gather(&acc) if err == nil { t.Error("got nil; want error") } }) t.Run("server closes the connection", func(t *testing.T) { ln, err := tls.Listen("tcp", ":0", config) if err != nil { t.Fatal(err) } defer ln.Close() go func() { sconn, err := ln.Accept() if err != nil { return } sconn.Close() }() sc := SSLCert{ Servers: []string{ln.Addr().String()}, Timeout: 5, } acc := testutil.Accumulator{} err = sc.Gather(&acc) if err == nil { t.Error("got nil; want error") } }) t.Run("server closes without handshake", func(t *testing.T) { ln, err := tls.Listen("tcp", ":0", config) if err != nil { t.Fatal(err) } defer ln.Close() go func() { sconn, err := ln.Accept() if err != nil { return } serverConfig := config.Clone() srv := tls.Server(sconn, serverConfig) srv.Close() }() sc := SSLCert{ Servers: []string{ln.Addr().String()}, Timeout: 5, } acc := testutil.Accumulator{} err = sc.Gather(&acc) if err == nil { t.Error("got nil; want error") } }) }

sepetrov · 2018-02-08T13:06:24Z

plugins/inputs/ssl_cert/ssl_cert_test.go

+
+	ln, err := tls.Listen("tcp", ":0", config)
+	if err != nil {
+		t.Error(err)


If there is an error, the test can not continue. You should call t.Fatal(err) instead

sepetrov · 2018-02-08T13:06:38Z

plugins/inputs/ssl_cert/ssl_cert_test.go

+
+	pair, err := tls.X509KeyPair([]byte(testCert), []byte(testKey))
+	if err != nil {
+		t.Error(err)


If there is an error, the test cannot continue. You should call t.Fatal(err) instead

danielnelson · 2018-02-09T21:24:54Z

Thanks for the pull request @jtyr and also the review @sepetrov (definitely appreciate non-maintainer reviews!). We have a pretty large backlog, but I'll try to take a look at this for 1.7, please nudge me if I don't get to it sometime in the next month. I'll also mention that there is a similar pull request #1762, it may be useful to read over it for ideas.

jtyr · 2018-02-12T20:59:16Z

@sepetrov Thanks a lot for your review! I have implemented most of your feedback. Please see the latest commit.

sepetrov · 2018-03-18T09:05:25Z

plugins/inputs/ssl_cert/ssl_cert.go

+	Timeout time.Duration `toml:"timeout"`
+
+	// For tests
+	CloseConn  bool


As I said in #3768 (comment) I would prefer to see black box tests where your tests live in separate package ssl_cert_test, and therefore you test only the public API. But if you have to have access to parts of the package, which change the behaviour, so that more test cases can be covered, then I would recommend the following:

Move the test-specific code outside the configuration struct SSLCert

// SSLCert holds the configuration of the plugin. type SSLCert struct { Servers []string `toml:"servers"` Files []string `toml:"files"` Timeout time.Duration `toml:"timeout"` } // For tests var ( closeConn bool unsetCerts bool )

This way the test-related code does not become part of the public API.

The disadvantage of having global state modifiers for the package by using this approach is that you will not be able to run tests in parallel, which you do not do here anyway.

Thanks for your review. It should be fixed in the last commit.

jtyr · 2018-03-22T00:06:50Z

@danielnelson Please could you review it once you have time?

danielnelson · 2018-03-22T00:14:07Z

Will do, sorry about the delay... once 1.6 is released please ping me again if I still haven't reviewed yet

sepetrov · 2018-03-22T11:14:49Z

LGTM 👍

jtyr · 2018-05-08T09:52:24Z

@danielnelson Ping. Please could you review?

danielnelson · 2018-04-12T01:27:03Z

plugins/inputs/ssl_cert/README.md

+# Reads metrics from a SSL certificate
+[[inputs.ssl_cert]]
+  ## List of local SSL files
+  #files = []


Let's add a bit more detail about what certificates can be loaded (PEM encoded). Perhaps we can use /etc/ssl/certs/ssl-cert-snakeoil.pem as the default path.

danielnelson · 2018-04-12T01:31:25Z

plugins/inputs/ssl_cert/README.md

+  ## List of local SSL files
+  #files = []
+  ## List of servers
+  #servers = []


We should have examples of the how this variable could look, use example.org as the hostname.

danielnelson · 2018-04-12T01:34:26Z

plugins/inputs/ssl_cert/ssl_cert.go

+		InsecureSkipVerify: true,
+	}
+
+	ipConn, err := net.DialTimeout("tcp", server, timeout)


I think we should allow the user to specify the network in the configuration, it would then look like servers = ["tcp://example.org:8080"]

danielnelson · 2018-04-12T01:41:24Z

plugins/inputs/ssl_cert/ssl_cert.go

+
+func getRemoteCert(server string, timeout time.Duration) (*x509.Certificate, error) {
+	tlsCfg := &tls.Config{
+		InsecureSkipVerify: true,


Do you think the tls.Config could be configurable? We have a feature request for client authentication in #3854 so perhaps we could use the standard set of SSL options:

telegraf/plugins/inputs/apache/apache.go

Lines 43 to 48 in 851efc9

## Optional TLS Config

# tls_ca = "/etc/telegraf/ca.pem"

# tls_cert = "/etc/telegraf/cert.pem"

# tls_key = "/etc/telegraf/key.pem"

## Use TLS but skip chain & host verification

# insecure_skip_verify = false

I think we would need to check what happens if we ignore the error from Handshake(), perhaps we can still get the PeerCertificates? We could also explore a custom tls.Config.VerifyPeerCertificate func.

danielnelson · 2018-04-12T01:43:03Z

plugins/inputs/all/all.go

 	_ "github.com/influxdata/telegraf/plugins/inputs/socket_listener"
 	_ "github.com/influxdata/telegraf/plugins/inputs/solr"
 	_ "github.com/influxdata/telegraf/plugins/inputs/sqlserver"
+	_ "github.com/influxdata/telegraf/plugins/inputs/ssl_cert"


Should we rename the plugin? We don't always do SSL/TLS (in the case of files), maybe it should be x509_cert or certificate?

danielnelson · 2018-04-12T01:52:41Z

plugins/inputs/ssl_cert/ssl_cert.go

+
+	certs := conn.ConnectionState().PeerCertificates
+
+	if unsetCerts {


I'm not a big fan of this testing method but I guess it's acceptable. It would be better IMO to use an interface for tls.Client and a mock during tests or to implement the behavior in the tests.

danielnelson · 2018-05-08T23:56:00Z

plugins/inputs/ssl_cert/ssl_cert.go

+	age := int(now.Sub(cert.NotBefore).Seconds())
+	expiry := int(cert.NotAfter.Sub(now).Seconds())
+	startdate := int(cert.NotBefore.Unix())
+	enddate := int(cert.NotAfter.Unix())


For startdate, enddate no need to convert to int

.Seconds() returns float64 so it's necessary to convert it to int to get only the integer part of the number.

Just make the change for startdate, enddate. (All integer fields added are converted to int64 or uint64 in the call to AddFields)

danielnelson · 2018-05-08T23:57:14Z

plugins/inputs/ssl_cert/ssl_cert.go

+type SSLCert struct {
+	Servers []string      `toml:"servers"`
+	Files   []string      `toml:"files"`
+	Timeout time.Duration `toml:"timeout"`


Use internal.Duration so we can specify timeout using a string like "5s"

danielnelson · 2018-05-09T00:26:37Z

plugins/inputs/ssl_cert/ssl_cert.go

+		"expiry":    expiry,
+		"startdate": startdate,
+		"enddate":   enddate,
+	}


Ideally we could also have metrics for if the certificate is valid, including values for common errors such as the hostname not matching the certificate. This is implemented in the related pr #3829, though ideally we could leverage the standard library code for this.

danielnelson · 2018-05-09T00:32:12Z

plugins/inputs/ssl_cert/ssl_cert_test.go

+	"github.com/influxdata/telegraf/testutil"
+)
+
+const testCert = `-----BEGIN CERTIFICATE-----


Can you update to use the testing certs in testutil/pki, check plugins/inputs/socket_listener/socket_listener_test.go for examples.

jtyr · 2018-06-26T11:23:17Z

@danielnelson I have implemented all your suggestions. Please review.

jtyr · 2018-07-20T10:43:09Z

@danielnelson Ping. Please could you review?

glinton · 2018-07-20T19:24:23Z

I made some slight modifications based on my review (see here). I know the first commit is fine, I agree with sepetrov regarding the test specific variables. Unifying the two get[Local|Remote]Certs is where I feel like danielnelson would have useful input. Feel free to cherry pick those commits, or i can directly push them to your branch, up to you.

glinton · 2018-07-20T19:25:00Z

plugins/inputs/x509_cert/x509_cert.go

+	"crypto/tls"
+	"crypto/x509"
+	"encoding/pem"
+	"errors"


Replace errors.New with fmt.Errorf

glinton · 2018-07-20T19:27:15Z

plugins/inputs/x509_cert/x509_cert.go

+
+// For tests
+var (
+	closeConn  bool


There isn't a need for test only variables in here, the same can be accomplished without them. (see this commit)

jtyr · 2018-07-22T20:41:30Z

Thanks for your review, @glinton. I have merged your changes and added few more. Any idea how to further improve test coverage (current coverage is only 92.7%)?

glinton · 2018-07-23T14:44:29Z

😄 I guess one thing you could do is remove this check if it never gets called (i believe there would be an error on the handshake if there were no certs), but i think you've got plenty of coverage.

jtyr · 2018-07-23T15:07:28Z

Thanks for the tip, @glinton. I have removed the check but the coverage is still only 94.3%! 😞

danielnelson · 2018-07-23T23:24:09Z

plugins/inputs/x509_cert/x509_cert.go

+	expiry := int(cert.NotAfter.Sub(now).Seconds())
+	startdate := cert.NotBefore.Unix()
+	enddate := cert.NotAfter.Unix()
+	valid := expiry > 0


I think we should drop this field for now. There are various aspects of the certificate which currently we do not take into account, such as hostname and chain of trust. We can add it back in if/when we include these features.

danielnelson · 2018-07-23T23:31:21Z

plugins/inputs/x509_cert/x509_cert.go

+		return certs, nil
+	case "file":
+		fallthrough
+	default:


I think default should be an error, but "" could be file, this would avoid schemes like "foo://".

danielnelson · 2018-07-23T23:32:10Z

plugins/inputs/x509_cert/x509_cert.go

+		fallthrough
+	case "udp":
+		fallthrough
+	case "tcp":


Let's add support for tcp4, tcp6, udp4, udp6?

danielnelson · 2018-07-23T23:37:03Z

plugins/inputs/x509_cert/x509_cert.go

+		conn := tls.Client(ipConn, tlsCfg)
+		defer conn.Close()
+
+		hsErr := conn.Handshake()


I'm curious about what happens when a certificate has an invalid hostname or the cert chain is invalid? What I think would be ideal is if we still called acc.AddFields( but with valid=false and potentially with more detailed information. If this is possible, Gather would only return an error if the network connection fails or we can't get any certificates.

I have removed the valid metric as per your request bellow (line 116).

danielnelson · 2018-07-23T23:39:09Z

plugins/inputs/x509_cert/x509_cert_test.go

+		{mode: 0640, content: pki.ReadServerCert(), error: false},
+	}
+
+	for i, test := range tests {


Not required, but would be a nice touch if we used subtests like here https://github.com/influxdata/telegraf/blob/master/plugins/inputs/docker/docker_test.go#L720

danielnelson · 2018-07-23T23:42:47Z

plugins/inputs/x509_cert/x509_cert.go

+
+const sampleConfig = `
+  ## List certificate sources
+  # sources = ["/etc/ssl/certs/ssl-cert-snakeoil.pem", "tcp://example.org:443"]


Let's uncomment this one line since it will almost always need to be changed.

danielnelson · 2018-07-23T23:43:17Z

plugins/inputs/x509_cert/x509_cert.go

+  # sources = ["/etc/ssl/certs/ssl-cert-snakeoil.pem", "tcp://example.org:443"]
+
+  ## Timeout for SSL connection
+  # timeout = 5


Since this is an internal.Duration, it should be like timeout = "5s"

danielnelson · 2018-07-23T23:46:59Z

plugins/inputs/x509_cert/x509_cert.go

+
+func getFields(cert *x509.Certificate, now time.Time) map[string]interface{} {
+	age := int(now.Sub(cert.NotBefore).Seconds())
+	expiry := int(cert.NotAfter.Sub(now).Seconds())


Aren't these already ints?

No, it's float64:
https://golang.org/pkg/time/#Duration.Seconds

jtyr

@danielnelson All should be fixed in the last commit. Please review it.

jtyr · 2018-07-29T19:11:06Z

plugins/inputs/x509_cert/x509_cert.go

+
+func getFields(cert *x509.Certificate, now time.Time) map[string]interface{} {
+	age := int(now.Sub(cert.NotBefore).Seconds())
+	expiry := int(cert.NotAfter.Sub(now).Seconds())


No, it's float64:
https://golang.org/pkg/time/#Duration.Seconds

jtyr · 2018-07-29T19:11:31Z

plugins/inputs/x509_cert/x509_cert.go

+  # sources = ["/etc/ssl/certs/ssl-cert-snakeoil.pem", "tcp://example.org:443"]
+
+  ## Timeout for SSL connection
+  # timeout = 5


jtyr · 2018-07-29T19:11:34Z

plugins/inputs/x509_cert/x509_cert.go

+
+const sampleConfig = `
+  ## List certificate sources
+  # sources = ["/etc/ssl/certs/ssl-cert-snakeoil.pem", "tcp://example.org:443"]


jtyr · 2018-07-29T19:13:23Z

plugins/inputs/x509_cert/x509_cert.go

+	expiry := int(cert.NotAfter.Sub(now).Seconds())
+	startdate := cert.NotBefore.Unix()
+	enddate := cert.NotAfter.Unix()
+	valid := expiry > 0


jtyr · 2018-07-29T22:52:31Z

plugins/inputs/x509_cert/x509_cert.go

+		fallthrough
+	case "udp":
+		fallthrough
+	case "tcp":


jtyr · 2018-07-29T23:05:24Z

plugins/inputs/x509_cert/x509_cert.go

+		return certs, nil
+	case "file":
+		fallthrough
+	default:


jtyr · 2018-07-29T23:10:19Z

plugins/inputs/x509_cert/x509_cert.go

+		conn := tls.Client(ipConn, tlsCfg)
+		defer conn.Close()
+
+		hsErr := conn.Handshake()


I have removed the valid metric as per your request bellow (line 116).

danielnelson · 2018-07-30T18:05:03Z

plugins/inputs/x509_cert/README.md

+  sources = ["/etc/ssl/certs/ssl-cert-snakeoil.pem", "https://example.org"]
+
+  ## Timeout for SSL connection
+  # timeout = 5s


Add quotes to be valid toml

danielnelson · 2018-07-30T18:05:14Z

plugins/inputs/x509_cert/x509_cert.go

+  sources = ["/etc/ssl/certs/ssl-cert-snakeoil.pem", "tcp://example.org:443"]
+
+  ## Timeout for SSL connection
+  # timeout = 5s


Add quotes to be valid toml

jtyr force-pushed the jtyr-ssl_cert branch 2 times, most recently from 0691f34 to 44af23c Compare February 8, 2018 00:35

sepetrov suggested changes Feb 8, 2018

View reviewed changes

danielnelson added this to the 1.7.0 milestone Feb 9, 2018

jtyr force-pushed the jtyr-ssl_cert branch 3 times, most recently from 2f38465 to 8aae40a Compare February 12, 2018 20:41

danielnelson mentioned this pull request Mar 6, 2018

Feature request for SSL 2 Way [[inputs.ssl]] #3854

Closed

sepetrov reviewed Mar 18, 2018

View reviewed changes

jtyr force-pushed the jtyr-ssl_cert branch 2 times, most recently from 2f2c5ff to 9889fed Compare March 21, 2018 23:47

danielnelson added the new plugin label Mar 22, 2018

danielnelson mentioned this pull request Apr 10, 2018

Add new SSL input plugin #3829

Closed

3 tasks

danielnelson mentioned this pull request Apr 27, 2018

[feature request] SSL Certificate Input #1971

Closed

danielnelson reviewed May 9, 2018

View reviewed changes

danielnelson modified the milestones: 1.7.0, 1.8.0 Jun 3, 2018

jtyr added 4 commits June 9, 2018 00:02

Adding ssl_cert input plugin

54abdc2

Implementing review feedback

79c28d6

Exclude test flags from the public API

bd12da1

Implementing comments from @danielnelson

7001319

russorat mentioned this pull request Jun 22, 2018

Added a new input plugin to check SSL certs #1762

Closed

3 tasks

Fixing test (thanks @sepetrov)

b4a4908

jtyr force-pushed the jtyr-ssl_cert branch from 9889fed to b4a4908 Compare June 26, 2018 10:00

jtyr changed the title ~~Adding ssl_cert input plugin~~ Adding x509_cert input plugin Jun 26, 2018

Ordering imports

7d93a69

glinton added 2 commits July 20, 2018 12:45

Remove unnecessary vars for testing

146d616

Unify cert gathering

e53e1e7

glinton suggested changes Jul 20, 2018

View reviewed changes

Align README with the code

d0e540a

Improving test coverage

4d28028

jtyr force-pushed the jtyr-ssl_cert branch from 7d44ba3 to 4d28028 Compare July 22, 2018 21:34

glinton approved these changes Jul 23, 2018

View reviewed changes

Removing unnecessary check

8649314

danielnelson reviewed Jul 23, 2018

View reviewed changes

Implementing review comments from danielnelson

0e8cfe6

jtyr commented Jul 29, 2018

View reviewed changes

jtyr added 2 commits July 30, 2018 01:33

Changing tests to use subtasks

35b29c4

Fixing formatting

8a1f0e4

danielnelson approved these changes Jul 30, 2018

View reviewed changes

glinton merged commit a897b84 into influxdata:master Jul 30, 2018

rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018

Adding x509_cert input plugin (influxdata#3768)

5d80e32

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019

Adding x509_cert input plugin (influxdata#3768)

e2aecac

	## Optional TLS Config
	# tls_ca = "/etc/telegraf/ca.pem"
	# tls_cert = "/etc/telegraf/cert.pem"
	# tls_key = "/etc/telegraf/key.pem"
	## Use TLS but skip chain & host verification
	# insecure_skip_verify = false


		certs := conn.ConnectionState().PeerCertificates

		if unsetCerts {

Conversation

jtyr commented Feb 8, 2018

Required for all PRs:

Uh oh!

sepetrov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielnelson commented Feb 9, 2018

Uh oh!

jtyr commented Feb 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtyr commented Mar 22, 2018

Uh oh!

danielnelson commented Mar 22, 2018

Uh oh!

sepetrov commented Mar 22, 2018

Uh oh!

jtyr commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtyr commented Jun 26, 2018

Uh oh!

jtyr commented Jul 20, 2018

Uh oh!

glinton commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

jtyr commented Jul 22, 2018 •

edited

Loading