Skip to content
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.

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

Monitor support for TCP ports that use TLS/STARTTLS #4806

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

mrubli
Copy link

@mrubli mrubli commented May 31, 2024

  • I have read and understand the pull request rules.

Description

This adds support for TCP ports that use either native TLS or STARTTLS-initiated TLS.

I had previously started a PR (#1626) that seemed to trigger people's interest but ultimately went nowhere. This is a complete rewrite for the latest development branch and now comes with STARTTLS support since that was requested in the original PR discussion. I've also renamed the monitor to "TCP Port (TLS)" as suggested by @louislam at the time.

Fixes #1079

Type of change

Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots

image

@mrubli mrubli force-pushed the feature/tcp-tls branch 3 times, most recently from ff7e21a to 2d1adce Compare May 31, 2024 16:57
CommanderStorm

This comment was marked as resolved.

@CommanderStorm CommanderStorm marked this pull request as ready for review May 31, 2024 18:47
@CommanderStorm CommanderStorm added area:monitor Everything related to monitors type:new proposing to add a new monitor pr:please address review comments this PR needs a bit more work to be mergable labels May 31, 2024
@mrubli
Copy link
Author

mrubli commented May 31, 2024

Thanks for the PR. (I am assuming this is ready for review despite being a draft)

Thanks for the fast turnaround time! I was going to mark it as ready once all the checks have passed but you beat me to it. :-)

Overall, this seems fine. I have left a few inline comments below for you to look at ^^

Great, I'll work on those.

About the STARTTLS part I am a bit concerned that we might break this part of the monitor accidentally without noticing it. (i.e. the same as for #4736) Do you think adding a Testcase if this still works is something you could add?

I have some local test cases for development that I should be able to integrate. Currently they are against my personal mail server but it should be possible to just use the SMTP or IMAP servers of a big email provider for those. I'll think about what the best option is.

@CommanderStorm
Copy link
Collaborator

One thing which I forgot to ask: this seems like a superset of the existing TCP port monitor.
Can said monitor be implemented via this monitor?

@mrubli
Copy link
Author

mrubli commented Jun 1, 2024

One thing which I forgot to ask: this seems like a superset of the existing TCP port monitor. Can said monitor be implemented via this monitor?

For the looks of it that would be possible. The current "TCP Port" monitor uses the tcp-ping library which also just uses a socket. Instead of reading/sending anything it just disconnects immediately. Seems like a superset indeed.

Btw, for addressing review comments, do you prefer it if I create separate commits, so that it's easy to see what changed, and in the end we squash/fixup the original commits? Or do you want me to use fixup as I go?

@CommanderStorm
Copy link
Collaborator

Btw, for addressing review comments, do you prefer it if I create separate commits, so that it's easy to see what changed, and in the end we squash/fixup the original commits? Or do you want me to use fixup as I go?

Do this as you prefer. No need to squash/rebase/.. anything, I will do that at the end in any case

@CommanderStorm
Copy link
Collaborator

Seems like a superset indeed

In this case, lets not keep two monitors with so similar functionality around.
That would likely be very confusing for our users
=> please consider switching to the old monitor types ID and remove the old monitor.

@mrubli
Copy link
Author

mrubli commented Jun 1, 2024

In this case, lets not keep two monitors with so similar functionality around. That would likely be very confusing for our users => please consider switching to the old monitor types ID and remove the old monitor.

Sounds like a plan. I'll work on the other stuff first and will merge the two monitors at the end. In the meantime, if you have any suggestions on how to best arrange the UI, please let me know. Otherwise I'll just go with my intuition for now.

@mrubli mrubli force-pushed the feature/tcp-tls branch 4 times, most recently from 490d868 to d717a78 Compare June 3, 2024 20:44
@mrubli
Copy link
Author

mrubli commented Jun 3, 2024

About the STARTTLS part I am a bit concerned that we might break this part of the monitor accidentally without noticing it. (i.e. the same as for #4736) Do you think adding a Testcase if this still works is something you could add?

@CommanderStorm I've added a couple of initial test cases: d717a78
Is this what you had in mind? If so, I'll add a bunch more to cover different SMTP, POP3, and IMAP4 scenarios. I just want to be sure it's okay to access external hosts like this before I start working on the rest of the test suite.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I know that you asked only about the testcases, but these jumped out at me. Hope you don't mind ^^

The many comments are so that you can accept them with less work required on your side ^^

test/backend-test/test-tls.js Outdated Show resolved Hide resolved
test/backend-test/test-tls.js Outdated Show resolved Hide resolved
server/monitor-types/tls.js Show resolved Hide resolved
server/monitor-types/tls.js Show resolved Hide resolved
server/monitor-types/tls.js Show resolved Hide resolved
server/monitor-types/tls.js Show resolved Hide resolved
server/monitor-types/tls.js Show resolved Hide resolved
server/monitor-types/tls.js Show resolved Hide resolved
server/monitor-types/tls.js Show resolved Hide resolved
return knex.schema
.alterTable("monitor", function (table) {
table.text("tcp_request").defaultTo(null);
table.boolean("tcp_enable_tls").notNullable().defaultTo(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets use the regular tls boolean field instead (no need to have a second field for this)

Copy link
Author

Choose a reason for hiding this comment

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

Probably I'm missing something but I can't find a "regular tls boolean field". The only fields containing tls in the name are:

grpc_enable_tls (boolean)
ignore_tls (boolean)
tls_ca (text)
tls_cert (text)
tls_key (text)

ignore_tls could conceivably be abused for this but it stands for "Ignore TLS/SSL errors for HTTPS websites", so that would seem a bit of a stretch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see where you are coming from. The grpc field should not exist.

In an ideal world, both (tcp+grpc) of these monitoes would use the same mechanism as http:

  • expiryNotifcation controls if tls expiry is a notification
  • ignoreTls controls if tls errors are notifications

Being transparent:
The crosstalk (expiry is currently an tls error 😅) between the two options is a bit unfortunate and not clear to our users.

Copy link
Author

Choose a reason for hiding this comment

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

I plan to eventually add support for expiry_notification and ignore_tls (which should really be called ignore_tls_errors) because that has been requested in the original PR as well. But I might do that as a separate PR because there's always "just one more feature" that could be added to any PR. :-)

The two options do have a very different function, though, so if it's not clear to users, that seems like a documentation issue. Sometimes a bit of explanatory text in the UI can go a long way.

As for the "enable TLS" setting, what do you suggest as the way forward? Is renaming grpc_enable_tls to enable_tls a possibility as part of the DB migration? If so, at least GRPC and TCP could use the same setting. Otherwise that would leave us with the generic name enable_tls (which other monitors could use down the line) or the monitor-specific tcp_enable_tls

Copy link
Collaborator

Choose a reason for hiding this comment

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

I might do that as a separate PR

Good choice 👍🏻

that seems like a documentation issue

Definitvely. Also the expiry buttons could be disabled if tls is ignored (different issue)

Is renaming grpc_enable_tls to enable_tls a possibility as part of the DB migration?

I think this should be merged into ignore_tls(_errors) instead.
(I agree on the renaming, but lets keep this PR focused on one thing..)

I don't think a separate enable_tls needs to exist, right?

Copy link
Collaborator

@CommanderStorm CommanderStorm Jun 9, 2024

Choose a reason for hiding this comment

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

(NOT ignore_tls[_errors] seems equivalent to grpc_enable_tls)

Copy link
Author

Choose a reason for hiding this comment

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

With the TLS monitor as it is right now, enable_tls == true is implied. But once I merge it with the TCP monitor, we'll definitely need to have a setting that determines how to connect:

  1. enable_tls == false ⇒ unencrypted connection (= TCP monitor today)
  2. enable_tls == true && tcp_start_tls == false ⇒ Native TLS connection
  3. enable_tls == true && tcp_start_tls == true ⇒ Start off unencrypted, TLS after STARTTLS handshake

Once implemented (separate PR), the two existing settings would become relevant for the merged TCP/TLS monitor as well:

ignore_tls[_errors]: Don't fail on TLS errors like "wrong host", "self-signed certificate", etc.
expiry_notification: Warn if the certificate is going to expire soon.

@mrubli
Copy link
Author

mrubli commented Jun 26, 2024

I've rebased the PR onto the latest master branch before starting work on merging the TCP Port and TCP Port (TLS) monitors.

For future reference, the check failure on "ARM64, 18" might be related to nodejs/node#46959.

@github-actions github-actions bot added the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:monitor Everything related to monitors needs:resolve-merge-conflict pr:please address review comments this PR needs a bit more work to be mergable pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again type:new proposing to add a new monitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SSL Certificate checks for General TCP ports
2 participants