Add TLS connection info to response object #322

robingustafsson · 2017-09-25T08:30:28Z

This PR adds TLS version, cipher suite and OCSP stapled response information to each HTTP response object, implementing #298.

Example usage:

import http from "k6/http";
import { check } from "k6";

export let options = {
    tlsCipherSuites: [
        "TLS_RSA_WITH_RC4_128_SHA",
        "TLS_RSA_WITH_AES_128_GCM_SHA256",
    ],
    tlsVersion: {
        min: "ssl3.0",
        max: "tls1.2"
    }
};

export default function() {
    let res = http.get("https://sha256.badssl.com");
    check(res, {
        "is TLSv1.2": (r) => r.tls_version === "tls1.2",
        "is sha256 cipher suite": (r) => r.tls_cipher_suite === "TLS_RSA_WITH_AES_128_GCM_SHA256"
    });
};

and

import http from "k6/http";
import { check } from "k6";

export default function() {
    let res = http.get("https://stackoverflow.com");
    check(res, {
        "is OCSP response good": (r) => r.ocsp.stapled_response.status === "good"
    });
};

The OCSP stapled_response object also contains the following properties:

ocsp.stapled_response.revocation_reason: a string
ocsp.stapled_response.produced_at: a date string
ocsp.stapled_response.this_update: a date string
ocsp.stapled_response.next_update: a date string
ocsp.stapled_response.revoked_at: a date string

robingustafsson · 2017-09-25T16:15:59Z

Draft (hidden) documentation is now also available at:

liclac · 2017-10-04T13:03:53Z

I want to make a structural change here, because for the first time in k6' history, we have a use for constants.

Making this easy-to-read string comparisons (ocsp.status === "good") sounds good in theory, until someone typo's it as "god" and suddenly we're refusing to trust any authority short of a holy scripture. And while that's a fairly easy one to catch, what about "tls1.2" vs "TLS1.2"?

The change I'm proposing is:

Change the module definition from simply type HTTP struct{}, to:

type HTTP struct{
    SSL_3_0, TLS_1_0, TLS_1_1, TLS_1_2 string
}

func New() *HTTP {
    return &HTTP{
        SSL_3_0: "ssl3.0",
        // ...
    }
}

Change js/modules/index.go from &http.HTTP{} to http.New(), and for consistency, also do this for all modules + their unit tests.

The actual naming of the constants is up for debate, and I think OCSP status may be better implemented as a tristate bool (*bool, true/false/null).

liclac · 2017-10-03T22:35:13Z

js/modules/k6/http/http.go

@@ -34,9 +35,12 @@ import (
 	"sync"
 	"time"

+	"golang.org/x/crypto/ocsp"


Please merge this into the import block below.

liclac · 2017-10-04T13:09:58Z

js/modules/k6/http/http.go

+}
+
+type OCSP struct {
+	StapledResponse OCSPStapledResponse


Is this indirection really needed...?

Probably not, maybe it should just be res.ocsp_response on the JS side?

Or maybe even just res.ocsp?

liclac · 2017-10-04T13:20:46Z

js/modules/k6/http/http.go

@@ -240,6 +257,60 @@ func (*HTTP) request(ctx context.Context, rt *goja.Runtime, state *common.State,
 		tags["status"] = strconv.Itoa(resp.Status)
 		tags["proto"] = resp.Proto

+		if res.TLS != nil {
+			tlsState := res.TLS


You can shorten this down to if tlsState := res.TLS; tlsState != nil {.

👍 typical Go noob mistake 😄

liclac · 2017-10-04T13:24:34Z

js/modules/k6/http/http.go

+				resp.TLSVersion = "tls1.2"
+			}
+			resp.TLSCipherSuite = lib.SupportedTLSCipherSuitesToString[tlsState.CipherSuite]
+			resp.OCSP.StapledResponse = OCSPStapledResponse{}


This is redundant.

liclac · 2017-10-04T13:26:40Z

js/modules/k6/http/http.go

@@ -240,6 +257,60 @@ func (*HTTP) request(ctx context.Context, rt *goja.Runtime, state *common.State,
 		tags["status"] = strconv.Itoa(resp.Status)
 		tags["proto"] = resp.Proto

+		if res.TLS != nil {
+			tlsState := res.TLS
+			switch tlsState.Version {


Let's make this a helper function, then assign resp.OCSP.StapledResponse to a fully initialized object. These switches are real lengthy.

liclac · 2017-10-04T13:39:06Z

js/modules/k6/http/http.go

+				resp.OCSP.StapledResponse.ProducedAt = ocspRes.ProducedAt.String()
+				resp.OCSP.StapledResponse.ThisUpdate = ocspRes.ThisUpdate.String()
+				resp.OCSP.StapledResponse.NextUpdate = ocspRes.NextUpdate.String()
+				resp.OCSP.StapledResponse.RevokedAt = ocspRes.RevokedAt.String()


This should be in a format we can compare on the JS side... UNIX timestamps?

(I'm also kind of questioning what purpose it will serve for a load test script; I suppose it could be useful for logging, but not much more.)

Yeah, should we use JS Date objects or Epoch milliseconds (to compare with Date.now() and Date.getTime()) when exposing to JS? Had the same question in the cookie PR.

Just in the interest of performance, I think maybe giving a UNIX timestamp would be better than instancing whole date objects every time. The JS-native way seems to be to deal with milliseconds for them, so then you can just do new Date(revokedAt).

liclac · 2017-10-04T13:42:16Z

js/modules/k6/http/http_test.go

+			_, err := common.RunString(rt, `http.get("https://expired.badssl.com/");`)
+			assert.EqualError(t, err, "GoError: Get https://expired.badssl.com/: x509: certificate has expired or is not yet valid")
+		})
+		t.Run("tls10", func(t *testing.T) {


These versions are nigh identical and would do better templated than copypasted.

Ok, will look into that.

liclac · 2017-10-04T13:51:48Z

I'd also like TLS version and OCSP status to be added as tags to responses.

robingustafsson · 2017-10-05T12:37:15Z

@liclac Haha, ocsp.status === "god" is of course the status when you've successfully infiltrated a CA with a HTTP request 😄

Agree on the use of constants for TLS version and cipher suites. I'll change the OCSP status to use constants as well. IMHO that's more clear when reading than a tristate.

robingustafsson · 2017-10-20T13:09:04Z

@liclac Requested changes have now been done, please have another look.

liclac · 2017-10-20T17:20:08Z

js/modules/k6/http/http.go

+	case tls.VersionTLS12:
+		res.TLSVersion = h.TLS_1_2
+	}
+	res.TLSCipherSuite = lib.SupportedTLSCipherSuitesToString[tlsState.CipherSuite]


Newline, please.

liclac · 2017-10-20T17:20:23Z

js/modules/k6/http/http.go

+		res.TLSVersion = h.TLS_1_2
+	}
+	res.TLSCipherSuite = lib.SupportedTLSCipherSuitesToString[tlsState.CipherSuite]
+	ocspStapledRes := OCSP{Status: h.OCSP_STATUS_UNKNOWN}


Newline, please.

liclac · 2017-10-20T17:21:03Z

js/modules/k6/http/http.go

+func (res *HTTPResponse) setTLSInfo(h *HTTP, tlsState *tls.ConnectionState) {
+	switch tlsState.Version {
+	case tls.VersionSSL30:
+		res.TLSVersion = h.SSL_3_0


These constants should honestly be global consts, passing in the HTTP object just to read them feels silly.

liclac · 2017-10-20T17:21:48Z

js/modules/k6/http/http.go

+	RevocationReason                              string
+	Status                                        string
+}
+
 type HTTPResponseTimings struct {
 	Duration, Blocked, LookingUp, Connecting, Sending, Waiting, Receiving float64
 }

 type HTTPResponse struct {


This struct is honestly getting big enough that I'd split it off into a response.go file.

liclac · 2017-10-20T17:23:44Z

js/modules/k6/http/http.go

@@ -240,6 +348,12 @@ func (*HTTP) request(ctx context.Context, rt *goja.Runtime, state *common.State,
 		tags["status"] = strconv.Itoa(resp.Status)
 		tags["proto"] = resp.Proto

+		if tlsState := res.TLS; res.TLS != nil {


tlsState is longer than res.TLS. As far as shorthands go, that's pretty lousy.

robingustafsson · 2017-10-23T22:45:01Z

@liclac Requested changes have been made.

robingustafsson requested a review from liclac September 25, 2017 08:30

liclac suggested changes Oct 4, 2017

View reviewed changes

Add TLS connection info to response object

78b70c5

robingustafsson force-pushed the feature/tls-resp-info branch from ad9135d to 2bca667 Compare October 20, 2017 12:21

Changes based on PR review feedback

bfa56d8

robingustafsson force-pushed the feature/tls-resp-info branch from 2bca667 to bfa56d8 Compare October 20, 2017 12:45

Fix examples

c0ba57d

liclac suggested changes Oct 20, 2017

View reviewed changes

Move response struct and related code to separate file

15bcfa0

liclac approved these changes Oct 24, 2017

View reviewed changes

liclac merged commit c849005 into master Oct 24, 2017

liclac deleted the feature/tls-resp-info branch October 24, 2017 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS connection info to response object #322

Add TLS connection info to response object #322

robingustafsson commented Sep 25, 2017 •

edited

robingustafsson commented Sep 25, 2017

liclac commented Oct 4, 2017

liclac Oct 3, 2017

liclac Oct 4, 2017

robingustafsson Oct 5, 2017

liclac Oct 5, 2017

liclac Oct 4, 2017

robingustafsson Oct 5, 2017

liclac Oct 4, 2017

liclac Oct 4, 2017

liclac Oct 4, 2017

robingustafsson Oct 5, 2017

liclac Oct 5, 2017

liclac Oct 4, 2017

robingustafsson Oct 5, 2017

liclac commented Oct 4, 2017

robingustafsson commented Oct 5, 2017

robingustafsson commented Oct 20, 2017

liclac Oct 20, 2017

liclac Oct 20, 2017

liclac Oct 20, 2017

liclac Oct 20, 2017

liclac Oct 20, 2017

robingustafsson commented Oct 23, 2017

Add TLS connection info to response object #322

Add TLS connection info to response object #322

Conversation

robingustafsson commented Sep 25, 2017 • edited

robingustafsson commented Sep 25, 2017

liclac commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liclac commented Oct 4, 2017

robingustafsson commented Oct 5, 2017

robingustafsson commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robingustafsson commented Oct 23, 2017

robingustafsson commented Sep 25, 2017 •

edited