Skip to content

MEN-3730 all server communication via openssl #581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merlin-northern merged 1 commit into from
Aug 7, 2020
Merged

MEN-3730 all server communication via openssl #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"strings"
"time"

openssl "github.com/Linutronix/golang-openssl"
"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"golang.org/x/net/http2"
Expand Down Expand Up @@ -280,6 +281,49 @@ func newHttpClient() *http.Client {
return &http.Client{}
}

func dialOpenSSL(conf Config, network string, addr string) (net.Conn, error) {
contextSSL, err := openssl.NewCtx()
// probably should consider reusing the context, but then we
// have to propagate it with every request.
err = contextSSL.LoadVerifyLocations(conf.ServerCert, "")
if err != nil {
return nil, err
}
flags := openssl.DialFlags(0)

if conf.NoVerify {
flags = openssl.InsecureSkipHostVerification
}

conn, err := openssl.Dial("tcp", addr, contextSSL, flags)
if err != nil {
return nil, err
}

v := conn.VerifyResult()
if v != openssl.Ok {
if v == openssl.CertHasExpired {
return nil, errors.Errorf("certificate has expired, "+
"openssl verify rc: %d server cert file: %s", v, conf.ServerCert)
}
if v == openssl.DepthZeroSelfSignedCert {
return nil, errors.Errorf("depth zero self-signed certificate, "+
"openssl verify rc: %d server cert file: %s", v, conf.ServerCert)
}
if v == 0x42 { //X509_V_ERR_EE_KEY_TOO_SMALL
return nil, errors.Errorf("end entity key too short, "+
"openssl verify rc: %d server cert file: %s", v, conf.ServerCert)
}
if v == 0x14 { //X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY
return nil, errors.Errorf("certificate signed by unknown authority, "+
"openssl verify rc: %d server cert file: %s", v, conf.ServerCert)
}
return nil, errors.Errorf("not a valid certificate, "+
"openssl verify rc: %d server cert file: %s", v, conf.ServerCert)
}
return conn, err
}

func newHttpsClient(conf Config) (*http.Client, error) {
client := newHttpClient()

Expand All @@ -295,6 +339,9 @@ func newHttpsClient(conf Config) (*http.Client, error) {
transport := http.Transport{
TLSClientConfig: &tlsc,
Proxy: http.ProxyFromEnvironment,
DialTLS: func(network string, addr string) (net.Conn, error) {
Copy link
Contributor

@alfrunes alfrunes Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs recommend using DialTLSContext:

// DialTLS [...]
// Deprecated: Use DialTLSContext instead, which allows the transport
// to cancel dials as soon as they are no longer needed.
// If both are set, DialTLSContext takes priority.

However, I don't think we use request contexts within the client so I'm not sure if this is relevant.

Copy link
Contributor Author

@merlin-northern merlin-northern Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. Alf, we have to support older go, thats why I use the deprecated method, and also that is why I added the comment about ts.EnableHTTP2 above.

Copy link
Contributor

@alfrunes alfrunes Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second thought I agree as well. Besides, I just discovered that the openssl library has hardcoded net.Dial anyway...

return dialOpenSSL(conf, network, addr)
},
}

client.Transport = &transport
Expand Down
167 changes: 158 additions & 9 deletions client/client_auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ import (
"fmt"
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"

"github.com/mendersoftware/mender/client/tests_helpers"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -88,13 +88,15 @@ func TestClientAuth(t *testing.T) {
http.Header{},
}

ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ts := startTestHTTPS(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
responder.headers = r.Header
w.WriteHeader(responder.httpStatus)
w.Header().Set("Content-Type", "application/json")

fmt.Fprint(w, responder.data)
}))
}),
localhostCert,
localhostKey)
Comment on lines +97 to +99
Copy link
Member

@lluiscampos lluiscampos Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation here is strange

Copy link
Contributor Author

@merlin-northern merlin-northern Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blame go fmt ;-) I can change it to your liking :) I will try.

Copy link
Contributor

@alfrunes alfrunes Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer having a line with the same number of closing brackets on a single line as an opposing line introduces. IOW, if you add a newline after startTestHTTPS( the indentation should look OK.

Copy link
Contributor Author

@merlin-northern merlin-northern Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! thanks. looks really better.

defer ts.Close()

ac, err := NewApiClient(
Expand Down Expand Up @@ -122,8 +124,10 @@ func TestClientAuth(t *testing.T) {
}

func TestClientAuthExpiredCert(t *testing.T) {
ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
}))
ts := startTestHTTPS(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
localhostCertExpired,
localhostKeyExpired)
defer ts.Close()

ac, err := NewApiClient(
Expand All @@ -145,8 +149,10 @@ func TestClientAuthExpiredCert(t *testing.T) {
}

func TestClientAuthUnknownAuthorityCert(t *testing.T) {
ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
}))
ts := startTestHTTPS(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
localhostCertUnknown,
localhostKeyUnknown)
defer ts.Close()

ac, err := NewApiClient(
Expand All @@ -164,12 +170,76 @@ func TestClientAuthUnknownAuthorityCert(t *testing.T) {
rsp, err := client.Request(ac, ts.URL, msger)
assert.Error(t, err)
assert.Contains(t, err.Error(), "certificate signed by unknown authority")
// see https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/x509/x509_vfy.c#L3268
// for self-signed openssl always returns self-signed error; either
// X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT or X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN
// and never X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT or X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY
assert.Nil(t, rsp)
}

//X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT
func TestClientAuthDepthZeroSelfSignedCert(t *testing.T) {
if tests_helpers.OpenSSLSecurityLevel < 2 {
t.Skip("skipping TestClientAuthEndEntityKeyTooSmall - security level < 2")
}
ts := startTestHTTPS(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
localhostCert,
localhostKey)
defer ts.Close()

ac, err := NewApiClient(
Config{"server.zero.depth.self.signed.crt", true, false},
)
assert.NotNil(t, ac)
assert.NoError(t, err)

client := NewAuth()
assert.NotNil(t, client)

msger := &testAuthDataMessenger{
reqData: []byte("foobar"),
}
rsp, err := client.Request(ac, ts.URL, msger)
assert.Error(t, err)
assert.Contains(t, err.Error(), "depth zero self-signed certificate")
assert.Nil(t, rsp)
}

//X509_V_ERR_EE_KEY_TOO_SMALL
func TestClientAuthEndEntityKeyTooSmall(t *testing.T) {
if tests_helpers.OpenSSLSecurityLevel < 2 {
t.Skip("skipping TestClientAuthEndEntityKeyTooSmall - security level < 2")
}
ts := startTestHTTPS(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
localhostCertShortEEKey,
localhostKeyShortEEKey)
defer ts.Close()

ac, err := NewApiClient(
Config{"server.crt", true, false},
)
assert.NotNil(t, ac)
assert.NoError(t, err)

client := NewAuth()
assert.NotNil(t, client)

msger := &testAuthDataMessenger{
reqData: []byte("foobar"),
}
rsp, err := client.Request(ac, ts.URL, msger)
assert.Error(t, err)
assert.Contains(t, err.Error(), "end entity key too short")
Copy link
Contributor

@alfrunes alfrunes Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious and tried to run these tests locally. However, this test failed with the following error:

    client_auth_test.go:223: 
        	Error Trace:	client_auth_test.go:223
        	Error:      	"generic error occurred while executing authorization request: Post \"https://127.0.0.1:41371/api/devices/v1/authentication/auth_requests\": depth zero self-signed certificate, openssl verify rc: 18 server cert file: server.crt" does not contain "end entity key too short"
        	Test:       	TestClientAuthEndEntityKeyTooSmall
--- FAIL: TestClientAuthEndEntityKeyTooSmall (0.00s)

Copy link
Contributor Author

@merlin-northern merlin-northern Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what s your security level? cat /etc/ssl/openssl.cnf please?

Copy link
Contributor

@alfrunes alfrunes Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, I think the implementation should be resilient to openssl configuration if possible.

Copy link
Contributor Author

@merlin-northern merlin-northern Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would merge this, and then create a follow up, without this one we will have to fork openssl wrapper. We might do it anyways, but for the sake of continuing, I would beg to try to make it work at level2 and then first thing next free moment do as you say. since I kind of agree :)

assert.Nil(t, rsp)
}

func TestClientAuthNoCert(t *testing.T) {
ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
}))
ts := startTestHTTPS(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
localhostCert,
localhostKey)
defer ts.Close()

ac, err := NewApiClient(
Expand All @@ -178,3 +248,82 @@ func TestClientAuthNoCert(t *testing.T) {
assert.NotNil(t, ac)
assert.NoError(t, err)
}

func TestClientAuthHostValidationNocheck(t *testing.T) {
ts := startTestHTTPS(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
localhostCert,
localhostKey)
defer ts.Close()

ac, err := NewApiClient(
Config{"server.crt", true, true},
)
assert.NotNil(t, ac)
assert.NoError(t, err)

client := NewAuth()
assert.NotNil(t, client)

msger := &testAuthDataMessenger{
reqData: []byte("foobar"),
}
rsp, err := client.Request(ac, ts.URL, msger)

assert.NoError(t, err)
assert.NotNil(t, rsp)
}

func TestClientAuthHostValidationError(t *testing.T) {
ts := startTestHTTPS(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
localhostCertWrongHost,
localhostKeyWrongHost)
defer ts.Close()

ac, err := NewApiClient(
Config{"server.unknown-authority.crt", true, false},
)
assert.NotNil(t, ac)
assert.NoError(t, err)

client := NewAuth()
assert.NotNil(t, client)

msger := &testAuthDataMessenger{
reqData: []byte("foobar"),
}
rsp, err := client.Request(ac, ts.URL, msger)

assert.Error(t, err)
assert.Contains(t, err.Error(), "Host validation error")
assert.Nil(t, rsp)
}

// this tests for the error that is handled by default 'not a valid certificate'
// via X509_V_ERR_CA_KEY_TOO_SMALL
func TestClientAuthNotValidCertificate(t *testing.T) {
if tests_helpers.OpenSSLSecurityLevel < 2 {
t.Skip("skipping TestClientAuthEndEntityKeyTooSmall - security level < 2")
}
ts := startTestHTTPS(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
localhostCertCAKeyTooShort,
localhostKeyCAKeyTooShort)
defer ts.Close()

ac, err := NewApiClient(
Config{"server.ca.key.too.small.crt", true, false},
)
assert.NotNil(t, ac)
assert.NoError(t, err)

client := NewAuth()
assert.NotNil(t, client)

msger := &testAuthDataMessenger{
reqData: []byte("foobar"),
}
rsp, err := client.Request(ac, ts.URL, msger)

assert.Error(t, err)
assert.Contains(t, err.Error(), "not a valid certificate")
assert.Nil(t, rsp)
}
16 changes: 9 additions & 7 deletions client/client_inventory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2017 Northern.tech AS
// Copyright 2020 Northern.tech AS
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,6 @@ package client
import (
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"

"github.com/pkg/errors"
Expand All @@ -35,12 +34,15 @@ func TestInventoryClient(t *testing.T) {
}

// Test server that always responds with 200 code, and specific payload
ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(responder.httpStatus)
ts := startTestHTTPS(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(responder.httpStatus)

responder.recdata, _ = ioutil.ReadAll(r.Body)
responder.path = r.URL.Path
}))
responder.recdata, _ = ioutil.ReadAll(r.Body)
responder.path = r.URL.Path
}),
localhostCert,
localhostKey)
Copy link
Member

@lluiscampos lluiscampos Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all these tests, we use a cert loaded from string (from tests_start_https.go) and also as a filename parameter for NewApiClient. I wonder if we can eliminate the duplication by making startTestHTTPS read the certs from the disk.

Copy link
Member

@lluiscampos lluiscampos Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am missing something, as some tests do use different certs for startTestHTTPS and NewApiClient.

Copy link
Contributor Author

@merlin-northern merlin-northern Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, thats the main idea. different certs combinations trigger different errors handled in dialOpenSSL.

Copy link
Member

@lluiscampos lluiscampos Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey.

But still the strings at startTestHTTPS could be loaded from the files instead of duplicating the certs.

Copy link
Contributor Author

@merlin-northern merlin-northern Aug 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do what I can!

Copy link
Contributor Author

@merlin-northern merlin-northern Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I took another look, Lluis maybe we can keep it like that? every certificate in https_server_test.go (was: tests_start_https.go) is different, I mean all the variables:

var localhostCertUnknown
var localhostKeyUnknown
var localhostCertExpired
var localhostKeyExpired
var localhostCert
var localhostKey
var localhostCertShortEEKey
var localhostKeyShortEEKey

hold different data, to trigger different errors in verification. if you insist I will load them form files, but you will have to convince me :) what are the benefits here?

Copy link
Member

@lluiscampos lluiscampos Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that, for example, var localhostCertExpired is the same content as file client/server.expired.crt. So for these (variables that are duplicated in files) we could just read the files.

It is not a big deal anyway, leave it as-is if you like it more.

defer ts.Close()

ac, err := NewApiClient(
Expand Down
16 changes: 9 additions & 7 deletions client/client_log_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2017 Northern.tech AS
// Copyright 2020 Northern.tech AS
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,6 @@ package client
import (
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"

"github.com/pkg/errors"
Expand All @@ -35,12 +34,15 @@ func TestLogUploadClient(t *testing.T) {
}

// Test server that always responds with 200 code, and specific payload
ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(responder.httpStatus)
ts := startTestHTTPS(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(responder.httpStatus)

responder.recdata, _ = ioutil.ReadAll(r.Body)
responder.path = r.URL.Path
}))
responder.recdata, _ = ioutil.ReadAll(r.Body)
responder.path = r.URL.Path
}),
localhostCert,
localhostKey)
defer ts.Close()

ac, err := NewApiClient(
Expand Down
16 changes: 9 additions & 7 deletions client/client_status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2018 Northern.tech AS
// Copyright 2020 Northern.tech AS
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,6 @@ package client
import (
"io/ioutil"
"net/http"
"net/http/httptest"
"testing"

"github.com/pkg/errors"
Expand All @@ -35,12 +34,15 @@ func TestStatusClient(t *testing.T) {
}

// Test server that always responds with 200 code, and specific payload
ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(responder.httpStatus)
ts := startTestHTTPS(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(responder.httpStatus)

responder.recdata, _ = ioutil.ReadAll(r.Body)
responder.path = r.URL.Path
}))
responder.recdata, _ = ioutil.ReadAll(r.Body)
responder.path = r.URL.Path
}),
localhostCert,
localhostKey)
defer ts.Close()

ac, err := NewApiClient(
Expand Down
Loading