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

mrubli · 2024-05-31T16:22:55Z

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

mrubli · 2024-05-31T19:21:51Z

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 · 2024-05-31T21:48:57Z

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 · 2024-06-01T14:40:37Z

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 · 2024-06-01T15:44:54Z

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 · 2024-06-01T15:51:26Z

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 · 2024-06-01T19:06:51Z

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 · 2024-06-03T20:47:36Z

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.

CommanderStorm

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

server/monitor-types/tls.js

CommanderStorm · 2024-06-03T21:47:57Z

db/knex_migrations/2024-04-30-0000-tls.js

+ return knex.schema
+ .alterTable("monitor", function (table) {
+ table.text("tcp_request").defaultTo(null);
+ table.boolean("tcp_enable_tls").notNullable().defaultTo(false);


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

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.

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.

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

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?

(NOT ignore_tls[_errors] seems equivalent to grpc_enable_tls)

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:

enable_tls == false ⇒ unencrypted connection (= TCP monitor today)

enable_tls == true && tcp_start_tls == false ⇒ Native TLS connection

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.

Having the migration alone is sufficient.

…refix

…nd/response

…tion

Co-authored-by: Frank Elsinga <[email protected]>

mrubli · 2024-06-26T11:23:51Z

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.

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

This comment was marked as resolved.

Sign in to view

CommanderStorm marked this pull request as ready for review May 31, 2024 18:47

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 force-pushed the feature/tcp-tls branch 4 times, most recently from 490d868 to d717a78 Compare June 3, 2024 20:44

CommanderStorm reviewed Jun 3, 2024

View reviewed changes

This was referenced Jun 3, 2024

Removed now unnecessary version checking #4815

Merged

removed some monitor drift in terms of code comments #4820

Merged

mrubli force-pushed the feature/tcp-tls branch from 33cdf0a to f02b4f8 Compare June 9, 2024 13:23

CommanderStorm mentioned this pull request Jun 12, 2024

feat: Add SNMP Monitor #4717

Merged

7 tasks

mrubli added 6 commits June 26, 2024 11:43

tls: db: monitors: Add 'request' and 'start_tls' columns

d92f9de

tls: pages: Add 'TCP Port (TLS)' monitor type and configuration options

33a3775

tls: server: Add 'TCP Port (TLS)' checker

c9260aa

tls: db: monitors: Remove init statements for request/start_tls

addd241

Having the migration alone is sufficient.

tls: db: monitors: Rename request/start_tls fields to have a 'tcp_' p…

2d0e7b3

…refix

tls: pages: Rename start-tls/request to have a 'tcp-' prefix

91150c4

mrubli and others added 21 commits June 26, 2024 11:43

tls: server: Rename startTls/request to tcpStartTls/tcpRequest

9a8a14b

tls: server: Various JSDoc fixes

7dcbd21

tls: server: Minor code style changes

58f4513

tls: server: Remove unnecessary function

7ea6d9c

tls: server: Remove 'options' object and use monitor properties directly

e03134c

tls: server: Document readData() socket/callback handling

1ca0896

tls: server: Rewrite and document the success check

38a252d

tls: server: Inline the logging helper functions

bb9a3d9

tls: server: Raise log level of main check results

8b114a9

Add unescape-js 1.1.4

710ed02

Add unescape-js 1.1.4 (package-lock.json)

22740ab

tls: db: monitors: Add fields for parametrizing STARTTLS prompt/comma…

2ba7cf7

…nd/response

tls: pages: Add UI for parametrizing STARTTLS prompt/command/response

110fde8

tls: server: Parametrize STARTTLS prompt/command/response

6a58451

tls: pages: Add links to RFCs relevant to STARTTLS

84bbda2

tls: server: Improve heartbeat and log messages

00c0563

tls: pages: Make keyword optional for the TLS monitor; update descrip…

94d04f7

…tion

test: backend-test: Add initial test cases for TLS monitor

37083db

tls: test: backend-test: Remove support for node <18

be622ed

Co-authored-by: Frank Elsinga <[email protected]>

test: backend-test: Add more test cases for TLS monitor

ddd7abb

tls: server: Clean up when TLS connection fails to establish

89eff42

mrubli force-pushed the feature/tcp-tls branch from df60aaf to 89eff42 Compare June 26, 2024 10:54

CommanderStorm mentioned this pull request Jun 29, 2024

When monitoring SSH (tcp/22), the monitored server logs "key_exchange_identification" errors. #4895

Closed

1 task

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mrubli commented May 31, 2024 •

edited by CommanderStorm

Loading

This comment was marked as resolved.

mrubli commented May 31, 2024

CommanderStorm commented May 31, 2024

mrubli commented Jun 1, 2024

CommanderStorm commented Jun 1, 2024

CommanderStorm commented Jun 1, 2024

mrubli commented Jun 1, 2024

mrubli commented Jun 3, 2024

CommanderStorm left a comment •

edited

Loading

CommanderStorm Jun 3, 2024

mrubli Jun 4, 2024

CommanderStorm Jun 4, 2024

mrubli Jun 9, 2024

CommanderStorm Jun 9, 2024

CommanderStorm Jun 9, 2024 •

edited

Loading

mrubli Jun 9, 2024

mrubli commented Jun 26, 2024

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

Are you sure you want to change the base?

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

Conversation

mrubli commented May 31, 2024 • edited by CommanderStorm Loading

Description

Type of change

Checklist

Screenshots

This comment was marked as resolved.

mrubli commented May 31, 2024

CommanderStorm commented May 31, 2024

mrubli commented Jun 1, 2024

CommanderStorm commented Jun 1, 2024

CommanderStorm commented Jun 1, 2024

mrubli commented Jun 1, 2024

mrubli commented Jun 3, 2024

CommanderStorm left a comment • edited Loading

Choose a reason for hiding this comment

CommanderStorm Jun 3, 2024

Choose a reason for hiding this comment

mrubli Jun 4, 2024

Choose a reason for hiding this comment

CommanderStorm Jun 4, 2024

Choose a reason for hiding this comment

mrubli Jun 9, 2024

Choose a reason for hiding this comment

CommanderStorm Jun 9, 2024

Choose a reason for hiding this comment

CommanderStorm Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

mrubli Jun 9, 2024

Choose a reason for hiding this comment

mrubli commented Jun 26, 2024

mrubli commented May 31, 2024 •

edited by CommanderStorm

Loading

CommanderStorm left a comment •

edited

Loading

CommanderStorm Jun 9, 2024 •

edited

Loading