Use libbeat LoadCertificate function on server side #235

sterchelen · 2017-10-13T12:38:46Z

To add the encrypted private key loading feature, I've :

update the vendor/github.com/elastic/beats/libbeat/outputs package which put in public mode the LoadCertificate function.
use the LoadCertificate function and inject the certificate in the http.Server.
Modify tests in consequence.
Modify reference yaml files.
Add more log on certificate / private key loading.

elasticmachine · 2017-10-13T12:38:48Z

Can one of the admins verify this patch?

sterchelen · 2017-10-13T12:43:28Z

@ruflin @jalvz Here is the change to use the libbeat LoadCertificate function.

There is no test for a private key with a pass_phrase. To allow that we should think to reuse the following function with little modification to decrypt the pem block: https://github.com/elastic/beats/blob/d15cab2b3ec265a7f8bf3514c0e4fbc01c498a40/libbeat/outputs/transport/transptest/testing.go#L180

Do I make the modification on beats repository or I copy the function inside our repository and apply the modification ?

ruflin · 2017-10-16T06:32:48Z

As the functionality is inherited from beats now, it should be up to beats to make sure it works as expected. Or is the usage / implementation different here?

sterchelen · 2017-10-17T17:27:22Z

@ruflin ,
The implementation is exactly the same as beats ! I will made a pull request on beats to implement this test :).

sterchelen · 2017-10-24T20:10:02Z

PR open on beats repository --> elastic/beats#5431
It modify the function that generate a certificate for testing purpose by adding a password parameter.

When the PR will be merged, I'll can add more tests on the beater/server_test.go file with a focus on private key with password handling.

sterchelen · 2017-11-12T15:36:11Z

Gentlemen,

I've use the beats modification that I've made to create testing certificate. I've also added a new unit test to test the server in https mode with a encrypted certificate key.

jalvz · 2017-11-13T09:36:20Z

Great @sterchelen ! Ill have a look soon. In the meanwhile, can you rebase master and fix conflicts?

sterchelen · 2017-11-18T22:12:01Z

@jalvz I've already merged master on it :)

jalvz · 2017-11-22T17:34:11Z

_meta/beat.reference.yml

@@ -20,7 +20,8 @@ apm-server:
  #ssl.enabled: false
  #ssl.certificate : "path/to/cert"
  #ssl.key : "path/to/private_key"
-
+  #ssl.pass_phrase: "foobar"


correct attribute here is ssl.key_passphrase

jalvz · 2017-11-22T17:42:35Z

beater/server_test.go

@@ -13,7 +13,10 @@ import (
 	"testing"
 	"time"

-	"github.com/kabukky/httpscerts"


this dependency is not used anymore, should be removed then (also remember to run make notice after removing it to remove its entry from the NOTICE file as well)

@jalvz This dependency is already removed. You are looking in the wrong side of the diff :)

Sorry, I meant the actual content from the dependency, not the import.
https://github.com/elastic/apm-server/blob/master/vendor/vendor.json#L1163-L1168 , https://github.com/elastic/apm-server/tree/master/vendor/github.com/kabukky/httpscerts and https://github.com/elastic/apm-server/blob/master/NOTICE.txt#L817-L848

Doesn't look like this has been addressed yet.

jalvz · 2017-11-22T17:56:33Z

beater/server_test.go

@@ -160,16 +163,29 @@ func TestServerNoContentType(t *testing.T) {
 	assert.Equal(t, http.StatusBadRequest, rr.Code, rr.Body.String())
 }

+func TestServerSecureOkWithPasswordKey(t *testing.T) {


This is good!
I miss 1 more test that "breaks" if the passphrase generated and the one passed in the server config do not match.
Note: in that scenario the server won't be able to start, so you need to assert that a timeout happens instead, the one in waitForServer. That is hardcoded to 10 seconds, so you will also need to modify that function to accept a timeout param and pass a lower value for that test because 10 secs every time it runs would be very slow.
Does this makes sense to you?

jalvz · 2017-11-22T17:57:31Z

beater/server.go

+		logp.Info("Loading private key: %s", config.SSL.Certificate.Key)
+		cert, err := outputs.LoadCertificate(&config.SSL.Certificate)
+		if err != nil {
+			return err


This code path should also be tested, I think

@jalvz While I'm writing a test for this path, I saw that we are completely hiding error from the setupServer function (server_test file). Particularly on the go run statement.

If we check the error while running the server, we could remove all the checkErrMsgtests.

For instance, the TestServerSecureBadDomain test, would only have to check if an error has occured with the following msg : http: TLS handshake error from 127.0.0.1:60368: remote error: tls: bad certificate.
Again, for the TestServerBadProtocol test, we would only have to check for an error like http: TLS handshake error from 127.0.0.1:60386: tls: oversized record received with length 21536 which tells us that we are trying to reach the https server with the http protocol.

Do you agree ?

@sterchelen that sounds good to me, but how do check the error while running the server?

jalvz · 2017-11-22T17:58:40Z

Great work @sterchelen ! I think this is very very close :)

Rev: 5c51448f5fc0c9f458273c452e0ce855fc749a1f

- Inject certificate in the http.Server - Change tests to use the output.CertificateConfig

- Vendoring transptest package from beats repository - Use GenCertForTestingPurpose function to generate testing certificates

sterchelen · 2017-12-23T15:29:21Z

@jalvz sorry for the delay ... So much professional work :)

graphaelli · 2018-01-04T16:24:48Z

_meta/beat.reference.yml

@@ -20,7 +20,7 @@ apm-server:
  #ssl.enabled: false
  #ssl.certificate : "path/to/cert"
  #ssl.key : "path/to/private_key"
-
+  #ssl.key_passphrase: "foobar"


let's make this an empty string to match other elastic pkgs

graphaelli · 2018-01-04T16:24:52Z

apm-server.reference.yml

@@ -20,7 +20,7 @@ apm-server:
  #ssl.enabled: false
  #ssl.certificate : "path/to/cert"
  #ssl.key : "path/to/private_key"
-
+  #ssl.key_passphrase: "foobar"


let's make this an empty string to match other elastic pkgs

graphaelli · 2018-01-04T16:27:03Z

beater/client.go

+			secureInfo := ""
+			if secure {
+				secureInfo = " (secure mode activated)"
+			}


building secureInfo can be done outside the for loop. Also good with removing it altogether.

Agree with building it outside the for loop. Not agree with removing it. IMO, logged that the server is in secure mode is relatively important to know ?

graphaelli · 2018-01-04T16:28:01Z

beater/server.go

 	"net/http"
 	"time"

 	"github.com/elastic/apm-server/version"
+	"github.com/elastic/beats/libbeat/outputs"
+


extra newline here

graphaelli · 2018-01-04T16:32:50Z

beater/server_test.go

@@ -274,7 +301,7 @@ func randomAddr() string {
 	return l.Addr().String()
 }

-func waitForServer(secure bool, host string) {
+func waitForServer(secure bool, host string, timeout int) {


would prefer a context to timeout here but we can add that later

Do you mean the name of the variable is not intelligible ?

graphaelli · 2018-01-04T16:34:33Z

beater/server_test.go

 		if check() == http.StatusOK {
 			return
 		}
+		time.Sleep(time.Second * 1)


How long does this normally take? I expect we can bring this way down.

graphaelli · 2018-01-04T16:34:57Z

beater/server_test.go

+	// 	if check() == http.StatusOK {
+	// 		return
+	// 	}
+	// }


remove commented out code

graphaelli · 2018-01-04T16:37:13Z

beater/server_test.go


-	return &SSLConfig{Cert: cert, PrivateKey: key}
+	return &SSLConfig{Enabled: newTrue(), Certificate: outputs.CertificateConfig{Certificate: name + ".pem", Key: name + ".key", Passphrase: passwordKey}}


since this is the only usage of newTrue(), a local truthy var like elsewhere would be preferred.

graphaelli · 2018-01-04T16:37:56Z

beater/server_test.go

@@ -13,7 +13,10 @@ import (
 	"testing"
 	"time"

-	"github.com/kabukky/httpscerts"


Doesn't look like this has been addressed yet.

sterchelen · 2018-01-20T22:14:43Z

@graphaelli Does the PR seems good to you ?

sterchelen · 2018-02-07T15:41:38Z

@graphaelli @ruflin Any news ?

elasticmachine · 2018-02-07T15:41:40Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jalvz · 2018-05-08T15:23:40Z

Hi @sterchelen we would like to have this changes in, but this got a bit old now and needs to rebase master.
Do you think you will have time to have a look or should I cherry-pick your work and take it from there?

sterchelen · 2018-05-08T16:26:36Z

Hi ! I will update the PR by the end of the week ! Sounds correct to you ?

…

On 8 May 2018, at 17:23, Juan Álvarez ***@***.***> wrote: Hi @sterchelen we would like to have this changes in, but this got a bit old now and needs to rebase master. Do you think you will have time to have a look or should I cherry-pick your work and take it from there? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jalvz · 2018-05-09T09:55:01Z

Sounds fantastic! 👍

sterchelen · 2018-05-18T11:20:44Z

@jalvz I will start fresh from master ! It tried to rebase master on the branch but it's too old... I don't want to add wrong code.
I will get back to you soon !

jalvz · 2018-06-07T07:23:36Z

Hi @sterchelen, I want to make sure you still have time for this. If not it is ok, I appreciate a lot the work you have done so far!

sterchelen · 2018-06-08T06:22:29Z

Hi Juan ! Sorry for the delay ! Unfortunately I can't update the PR right now ! May be this week end but not sure ! Cordialement, Sterchele Nicolas. 2018-06-07 9:23 GMT+02:00 Juan Álvarez <notifications@github.com>:

…

Hi @sterchelen <https://github.com/sterchelen>, I want to make sure you still have time for this. If not it is ok, I appreciate a lot the work you have done so far! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#235 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQfcaYoVy7SVPz2UQHelcYZvrSOKkRc9ks5t6NT-gaJpZM4P4bN3> .

jalvz · 2018-06-15T09:36:45Z

Took this over to #1010
Thanks @sterchelen !

jalvz reviewed Nov 22, 2017

View reviewed changes

Sterchele Nicolas added 7 commits December 23, 2017 16:06

Update outputs libbeat package.

0d1f952

Rev: 5c51448f5fc0c9f458273c452e0ce855fc749a1f

Log https usage on checking server up

1f06ea5

Use libbeat LoadCertificate function for ssl mode

7ec22ef

- Inject certificate in the http.Server - Change tests to use the output.CertificateConfig

Use beats function's to generate testing certificate

0ca9886

- Vendoring transptest package from beats repository - Use GenCertForTestingPurpose function to generate testing certificates

Add unit test with certificate key encrypted.

a919370

Fix wrong ssl passphrase property on yaml files

4390baf

Add new unit test for wrong certificate passphrase

a2f289e

graphaelli self-assigned this Jan 4, 2018

graphaelli mentioned this pull request Jan 4, 2018

Follow up issue for TLS implementation #44

Closed

6 tasks

graphaelli added the enhancement label Jan 4, 2018

graphaelli requested changes Jan 4, 2018

View reviewed changes

Sterchele Nicolas added 2 commits January 5, 2018 13:32

Remove httpscert dependencies + minor fix

76360e6

Merge branch 'master' into feature/use_libbeat_tls_cert_loading

4335384

alvarolobato added the [zube]: Inbox label Mar 28, 2018

alvarolobato added [zube]: In Progress and removed [zube]: Inbox labels Apr 16, 2018

graphaelli added [zube]: Backlog and removed [zube]: In Progress labels Apr 26, 2018

alvarolobato assigned alvarolobato, jalvz and acesir and unassigned graphaelli and alvarolobato May 7, 2018

alvarolobato added [zube]: Ready and removed [zube]: Backlog labels May 7, 2018

alvarolobato added [zube]: In Review and removed [zube]: Ready labels May 23, 2018

jalvz added [zube]: Blocked and removed [zube]: In Review labels May 28, 2018

jalvz closed this Jun 15, 2018

zube bot added [zube]: Done and removed [zube]: Blocked labels Jun 15, 2018

alvarolobato removed the [zube]: Done label Jun 27, 2018


		return &SSLConfig{Cert: cert, PrivateKey: key}
		return &SSLConfig{Enabled: newTrue(), Certificate: outputs.CertificateConfig{Certificate: name + ".pem", Key: name + ".key", Passphrase: passwordKey}}

Use libbeat LoadCertificate function on server side #235

Use libbeat LoadCertificate function on server side #235

Conversation

sterchelen commented Oct 13, 2017 • edited by alvarolobato Loading

elasticmachine commented Oct 13, 2017

sterchelen commented Oct 13, 2017

ruflin commented Oct 16, 2017

sterchelen commented Oct 17, 2017

sterchelen commented Oct 24, 2017

sterchelen commented Nov 12, 2017

jalvz commented Nov 13, 2017

sterchelen commented Nov 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sterchelen Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

jalvz Nov 27, 2017 • edited Loading

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

jalvz commented Nov 22, 2017

sterchelen commented Dec 23, 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

sterchelen commented Jan 20, 2018

sterchelen commented Feb 7, 2018

elasticmachine commented Feb 7, 2018

jalvz commented May 8, 2018

sterchelen commented May 8, 2018 via email

jalvz commented May 9, 2018

sterchelen commented May 18, 2018

jalvz commented Jun 7, 2018

sterchelen commented Jun 8, 2018 via email

jalvz commented Jun 15, 2018

sterchelen commented Oct 13, 2017 •

edited by alvarolobato

Loading

sterchelen commented Nov 18, 2017 •

edited

Loading

sterchelen Nov 24, 2017 •

edited

Loading

jalvz Nov 27, 2017 •

edited

Loading