MEN-3730 all server communication via openssl#581
Conversation
|
Hello 😸 I created a pipeline for you here: Pipeline-173902074 Build Configuration Matrix
|
merlin-northern
force-pushed
the
men_3730_all_server_communication_via_openssl
branch
from
0044d01 to
bd72a91
Compare
|
Hello 😸 I created a pipeline for you here: Pipeline-173906117 Build Configuration Matrix
|
merlin-northern
force-pushed
the
men_3730_all_server_communication_via_openssl
branch
from
bd72a91 to
e02f06e
Compare
|
Hello 😸 I created a pipeline for you here: Pipeline-174015540 Build Configuration Matrix
|
merlin-northern
force-pushed
the
men_3730_all_server_communication_via_openssl
branch
from
e02f06e to
a86277f
Compare
|
Hello 😸 I created a pipeline for you here: Pipeline-174018823 Build Configuration Matrix
|
|
@merlin-northern accidentally added |
wow. thanks! |
mchalski
left a comment
mchalski
left a comment
There was a problem hiding this comment.
it looks good to me at least.
I'd wait for a review from core client devs though (if it's possible in reasonable time)
thanks! |
lluiscampos
left a comment
lluiscampos
left a comment
There was a problem hiding this comment.
Here my round of comments and questions.
We should have the meta-mender PR in place to merge both at the same time. Let me know if you need help with yocto!
I am not an TLS expert... I hope @alfrunes can provide more technical feedback.
Other nitpicks:
- The two first commits (implementation and tests) should be combined together IMO. Specially for change sets that include new golang dependencies, there is a chance that these are not in Debian and we need to revert the change. In that case, a single patch is better than two :)
- You don't need the JIRA link in the commit message (changelog-generator creates it for you if you have a MEN-XXXX reference). It doesn't hurt, though, so it is just FYI.
| conn, err := openssl.Dial("tcp", addr, contextSSL, flags) | ||
| if conn == nil || err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Mmm. could we have conn == nil and err == nil? In that case the return would not make sense, we would need two different ones:
| conn, err := openssl.Dial("tcp", addr, contextSSL, flags) | |
| if conn == nil || err != nil { | |
| return nil, err | |
| } | |
| conn, err := openssl.Dial("tcp", addr, contextSSL, flags) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if con == nil { | |
| ... | |
| } |
There was a problem hiding this comment.
Mmm. could we have
conn == nilanderr == nil? In that case the return would not make sense, we would need two different ones:
no chance for that. I checked that every time DialSession returns only nil,err (where err!=nil) or conn,nil (where conn!=nil)
There was a problem hiding this comment.
Then I would only check for err. The if condition is somewhat dangerous as-is.
There was a problem hiding this comment.
will do! thanks.
client/tests_start_https.go
Outdated
| //if ts.EnableHTTP2 { // to be uncommented when go version is updated in build_servers | ||
| // nextProtos = []string{"h2"} | ||
| //} |
There was a problem hiding this comment.
I don't understand this comment; the golang version for the client is defined by yocto, no relation with the build_servers. Better to specify from which golang version on can we uncomment this.
There was a problem hiding this comment.
will reword that
There was a problem hiding this comment.
I think you can remove this comment. httptest.NewUnstartedServer returns a structure where this field is false anyway. From the docs:
EnableHTTP2 [...] It must be set between calling
NewUnstartedServer and calling Server.StartTLS.
There was a problem hiding this comment.
I chose to reword the comment.
Alf, if you insist on removing that, I have no problem with that. I can also add:
if runtime.Version() == "go1.12" { ... }I wonder if that would work? but I would like to avoid it, since it leads to good old #ifdef __PLATFORM__ ... #endif mess.
| }), | ||
| localhostCert, | ||
| localhostKey) |
There was a problem hiding this comment.
The indentation here is strange
There was a problem hiding this comment.
blame go fmt ;-) I can change it to your liking :) I will try.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
sure! thanks. looks really better.
| })) | ||
| }), | ||
| localhostCert, | ||
| localhostKey) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think I am missing something, as some tests do use different certs for startTestHTTPS and NewApiClient.
There was a problem hiding this comment.
yes, thats the main idea. different certs combinations trigger different errors handled in dialOpenSSL.
There was a problem hiding this comment.
Okey.
But still the strings at startTestHTTPS could be loaded from the files instead of duplicating the certs.
There was a problem hiding this comment.
I will do what I can!
There was a problem hiding this comment.
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 localhostKeyShortEEKeyhold 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?
There was a problem hiding this comment.
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.
I have no idea if that is the correct place |
| @@ -0,0 +1,565 @@ | |||
| // Copyright 2020 Northern.tech AS | |||
There was a problem hiding this comment.
Nitpick: please change the filename to something ending in _test.go (e.g. https_server_test.go) so that the symbols won't be defined when running go build (also, this will contribute to the test coverage).
client/tests_start_https.go
Outdated
| //if ts.EnableHTTP2 { // to be uncommented when go version is updated in build_servers | ||
| // nextProtos = []string{"h2"} | ||
| //} |
There was a problem hiding this comment.
I think you can remove this comment. httptest.NewUnstartedServer returns a structure where this field is false anyway. From the docs:
EnableHTTP2 [...] It must be set between calling
NewUnstartedServer and calling Server.StartTLS.
| } | ||
| rsp, err := client.Request(ac, ts.URL, msger) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "end entity key too short") |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
what s your security level? cat /etc/ssl/openssl.cnf please?
There was a problem hiding this comment.
IMHO, I think the implementation should be resilient to openssl configuration if possible.
There was a problem hiding this comment.
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 :)
| transport := http.Transport{ | ||
| TLSClientConfig: &tlsc, | ||
| Proxy: http.ProxyFromEnvironment, | ||
| DialTLS: func(network string, addr string) (net.Conn, error) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
On a second thought I agree as well. Besides, I just discovered that the openssl library has hardcoded net.Dial anyway...
merlin-northern
force-pushed
the
men_3730_all_server_communication_via_openssl
branch
from
a86277f to
3cf5289
Compare
|
Hello 😸 I created a pipeline for you here: Pipeline-174476956 Build Configuration Matrix
|
merged and skipped the .gitignore update.
removed. |
lluiscampos
left a comment
lluiscampos
left a comment
There was a problem hiding this comment.
With latest rebase we lost the nice Changelog. Please fix that.
The rest, from my side, looks good to go (after the meta-mender PR, of course)
| })) | ||
| }), | ||
| localhostCert, | ||
| localhostKey) |
There was a problem hiding this comment.
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.
ah, right!
right! |
Codecov Report
@@ Coverage Diff @@
## master #581 +/- ##
==========================================
+ Coverage 69.90% 70.49% +0.58%
==========================================
Files 57 57
Lines 6520 6551 +31
==========================================
+ Hits 4558 4618 +60
+ Misses 1494 1466 -28
+ Partials 468 467 -1
|
merlin-northern
force-pushed
the
men_3730_all_server_communication_via_openssl
branch
from
3cf5289 to
a228500
Compare
|
Hello 😸 I created a pipeline for you here: Pipeline-174588786 Build Configuration Matrix
|
|
Hello 😸 I created a pipeline for you here: Pipeline-174589994 Build Configuration Matrix
|
merlin-northern
force-pushed
the
men_3730_all_server_communication_via_openssl
branch
from
a228500 to
7245bdb
Compare
|
Hello 😸 I created a pipeline for you here: Pipeline-174917657 Build Configuration Matrix
|
merlin-northern
force-pushed
the
men_3730_all_server_communication_via_openssl
branch
from
7245bdb to
9747a9f
Compare
|
Hello 😸 I created a pipeline for you here: Pipeline-174919466 Build Configuration Matrix
|
|
Hello 😸 I created a pipeline for you here: Pipeline-174965196 Build Configuration Matrix
|
All server communication goes through openssl:
* custom dial function is provided
* new verification errors are handled
Unit tests fixes:
* new certs
* new call startTestHTTPS, to avoid build-in localhostCert from httptest
* new tests:
* TestClientAuthDepthZeroSelfSignedCert
X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT
* TestClientAuthEndEntityKeyTooSmall
X509_V_ERR_EE_KEY_TOO_SMALL
* test host added to /etc/hosts in .gitlab-ci.yml
* added host verification error test
* added no verify test
ChangeLog:Switch to OpenSSL for all server communication.
Signed-off-by: Peter Grzybowski <peter@northern.tech>
merlin-northern
force-pushed
the
men_3730_all_server_communication_via_openssl
branch
from
9747a9f to
63d50ed
Compare
|
Hello 😸 I created a pipeline for you here: Pipeline-174999555 Build Configuration Matrix
|
| */ | ||
| import "C" | ||
|
|
||
| var OpenSSLSecurityLevel = C.X_SSL_get_security_level() |
5 participants
MEN-3730 Switch all client crypto operations over to OpenSSL.
https://tracker.mender.io/browse/MEN-3730
Note: goes with meta-mender/pull/1052
spin offs:
ChangeLog: Switch to OpenSSL for all server communication.
Signed-off-by: Peter Grzybowski peter@northern.tech