Follow up issue for TLS implementation about apm-server HOT 20 CLOSED

@sterchelen we also have our Discuss forums: https://discuss.elastic.co/c/apm
@ruflin Inspired by your comment, I took the liberty to create the channel #elastic-apm on Freenode. I'm a rebel, sorry 😅

from apm-server.

Hi @ruflin , I'm looking to contribute to this project...
For the following point:

Use ucfg for Enabled() flag

What did you meant ? Replacing the isEnabled() function by using the ucfg package ?

from apm-server.

@sterchelen Happy to hear you want to contribute.

For the Enabled() part: It is exactly as you described to replace isEnabled by the internal Enabled() function from go-ucfg. But I think I tried to this quickly in the past and it actually made the code more complex instead of simpler and is the reason we went with the implementation we have now.

In general it would be great to share code between libbeat and apm-server for TLS (if possible) but never checked in detail how much we can share.

from apm-server.

I can see if it's possible to share the TLS usage of beats with the apm-server.

@ruflin , sorry to ask a question of this type here but do you have a slack channel or anything else to talk more freely with the rest of the team ? I didn't see a link to a chat room on the contributing page...

Thank you.

from apm-server.

@sterchelen Our main communication channel is Github. Most teams also have an IRC channel: https://www.elastic.co/community But APM does not have one yet.

from apm-server.

@ruflin I made an investigation to reuse the loadCertificate function in libbeat. Here is my point of view:

• We could use the CertificateConfig type https://github.com/elastic/beats/blob/c821b84cf55f88778c9702a60aea52c52d5643d7/libbeat/outputs/tls.go#L41. Only this type, for the moment, because we didn't have yet the others TLS features implemented (based on this comment --> #32 (comment))
• Create our own loadCertificate function. We shall, also, extract the readPEMFile function which is the central point to decrypt a pem file based on a passphrase.
• Finally, on the run function https://github.com/elastic/apm-server/blob/master/beater/server.go#L70, based on the isEnabled() test we load the tls.Config and affect it to the http.Server. The tls.Config contains the certificate.

With this feature we can handle a certificate that is protected by a passphrase. It could, also, allows to start the "refactoring" of the TLS usage and reuse more or less the libbeat version.

I can start working on it...

from apm-server.

@sterchelen I like the idea. How would the config look like after this change? Could you post a yaml example?

I would like to get @jalvz opinion here before we start working on it has he did the implementation and knows best if this fits in.

from apm-server.

@ruflin , here is the yaml example:

apm-server:

  host: "localhost:8200"
  read_timeout: 2s
  write_timeout: 2s
  shutdown_timeout: 5s
  concurrent_requests: 20

  tls.enabled: true
  tls.certificate: "/path/to/cert"
  tls.key: "path/to/private_key"
  tls.key_passphrase: "blabla"

ssl is the predecessor of tls so if we want to be etymologically correct we should use tls instead of ssl.
The other modification is the addition of the tls.key_passphrase.

from apm-server.

The config looks good to. Even though tls is more correct, we use ssl to be in sync with the rest of the elastic stack so I would keep ssl for now.

I want @jalvz to chime in here to see if we are on the right track.

from apm-server.

All right ! Waiting @jalvz thoughts.

from apm-server.

sorry, this slipped through the cracks, but overall SGTM

if we are going to duplicate too much code from loadCertifcate / readPEMFile i think it would be better to expose those functions in libbeat, or at least extract and expose the common bits we need

looking forward to it

from apm-server.

@jalvz Ok, when you say:

expose those functions in libbeat

You are meaning modifying libbeat functions inside the vendor folder ? Or creating a pull request to the beats repository ?

from apm-server.

The right way would be a PR in beats, yes.

from apm-server.

As you want... I can create a pull request about this subject.

from apm-server.

PR created --> elastic/beats#5388

from apm-server.

Hi, the PR has been merged !

Start working on loading certificate from the libbeat outputs package.

from apm-server.

great! 👍

from apm-server.

• Evaluate to use libbeat cert generation --> Done
• Evaluate to reuse load certificate from libbeat --> Done

from apm-server.

reviewing this for possible inclusion in 6.2 - #235

from apm-server.

i'm closing this until a requirement for more functionality comes up. PR235 is still open and can be merged when ready.

from apm-server.