Checking certificate by sburmanoctopus · Pull Request #1295 · OctopusDeploy/OctopusTentacle

sburmanoctopus · 2026-09-09T04:36:22Z

Background

When registering or configuring a polling Tentacle against a WebSocket address, OctopusServerChecker probes the Octopus Server over TLS to learn the thumbprint to trust. The probe accepts any certificate — it has to, since the thumbprint can't be read from a handshake we refuse to complete — but the harvested thumbprint was then pinned with no further checks and nothing shown to the operator.

Establishing trust this way is intentional and documented, and exploiting it needs an attacker already positioned between the Tentacle and the Server, so this isn't treated as a vulnerability.

Results

Fixes LEV-1806

Fixes #1296

The probe now records the SslPolicyErrors alongside the thumbprint, and registration acts on it:

Certificate validates → trusted as before, no prompt.
Doesn't validate → operator is shown the thumbprint and the failure reason, and must confirm.
No operator to ask (Console.IsInputRedirected) → fails with a message naming the thumbprint and
how to proceed.
New --dangerously-skip-certificate-validation → trusts without prompting, for unattended runs.

Only the WebSocket path changes. Comms-port registration takes its thumbprint from the authenticated
Octopus API, so it never relied on the probe.

Breaking change: unattended WebSocket registration against a server whose certificate doesn't
chain to a local trust anchor now fails instead of silently trusting it. Intended for the next set
of breaking changes.

Also fixes a real defect: ServicePointManager.ServerCertificateValidationCallback is process-wide
and was left installed after the probe. It's now restored in a finally.

Testing

We wrote tests for this functionality. This did mean having to pass down classes through the layers so we could mock these objects.

We manually ran the "register-with" command with a couple of these scenarios to ensure registration behaved as expected.

Pre-requisites

I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
I have considered informing or consulting the right people, according to the ownership map.
I have considered appropriate testing for my change.

xwipeoutx · 2026-09-09T22:24:11Z

This needs a commit with the comment +semver: major - or it needs to be in the PR title. See the readme.

xwipeoutx

I think we need to reshape this a fair bit - further scrutiny and AI interrogation is showing me the original suggested fix had some gaps.

Happy to jump on a call.

xwipeoutx · 2026-09-09T22:30:35Z

+
+namespace Octopus.Tentacle.Communications
+{
+    public class ConsoleCertificateTrustPrompt : ICertificateTrustPrompt


[nit] given the question comes in via the method, I think this is a general prompt, not a certificate trust prompt

Agreed. Changed

xwipeoutx · 2026-09-09T22:33:02Z

+                policyErrors = errors;
+
+                // We have to accept the certificate to complete the handshake, otherwise we never get to read its thumbprint. 
                return true;


I would rather this part not return true if the cert failed and they haven't put in the --skip... CLI. Fail as early as we can.

I looked into this, and making it return false means we end up throwing an exception rather than returning a nice message.

I'm also not a massive fan of putting the user prompt in here either.

We had a discussion, and decided to leave it as is.

xwipeoutx · 2026-09-09T22:34:31Z

+                return serverCheckResult.Thumbprint;
            }

+            // Every other address takes its thumbprint from the authenticated Octopus API rather than from the probe,


I don't really understand the why here - why is web sockets different? Feels like bugs and vulnerabilities waiting to happen.

There's probably a reason, and I'd like us to nab it now while we're in major-version-and-security-land

I threw Claude at it - the websocket endpoint cert could very well be terminated before it even gets to server - like, IIS or caddy or nginx would be doing that. The thumbprint reported in Server is specifically the comms thumbprint. They're just different.

Asking the user to just "confirm it's the same as octopus server" is going to mislead them. I think we need to be clearer about it.

Rewording was done on the prompts and error messages.

xwipeoutx · 2026-09-09T22:42:34Z

            Options.Add("server-comms-port=", "The comms port on the Octopus Server; the default is " + DefaultServerCommsPort + ". If specified, this will take precedence over any port number in server-comms-address.", s => serverCommsPort = int.Parse(s));
            Options.Add("server-web-socket=", "When using active communication over websockets, the address of the Octopus Server, eg 'wss://example.com/OctopusComms'. Refer to http://g.octopushq.com/WebSocketComms", s => serverWebSocketAddress = s);
            Options.Add("reuse-server-thumbprint", "Reuse the Server Thumbprint from the first trusted server instance currently configured", _ => reuseThumbprint = true);
+            Options.Add(ServerCertificateTrustConfirmation.SkipValidationArgumentName, ServerCertificateTrustConfirmation.SkipValidationArgumentDescription, _ => skipCertificateValidation = true);


I don't like this constant, made it way harder to track down the name

I was on the fence on this one. If we remove the constants, it means some repeats, but it fits everything else.

I'm happy to remove them 🙂

xwipeoutx · 2026-09-09T22:46:43Z

+            this.prompt = prompt;
+        }
+
+        public void EnsureCertificateIsTrusted(Uri serverAddress, OctopusServerCommunicationsCheckResult checkResult, bool skipValidation)


Hmm, I know the linear ticket said skip validation, but I think we can be a little stronger here and say --server-certificate-thumbprint=<X> instead. My concern is automated scripts (say a k8s cluster of elastic workers) all having --dangerously-skip-certificate-validation by default, leaving any of them open for this MITM.

Of course we can still show them the thumbprint we received, so they can still copy/paste it in for the unattended case. And we can still y/N prompt them in the attended case (with a warning to check the thumbprint of course)

Hmm it's also becoming clear to me that this flag is only for websockets - so it should be named as such.

We have --server-web-socket=wss:// I reckon we also need --server-web-socket-thumbprint=<X> to cover all this, and we're good, yeah?

There's the extra fun case of when they use ws:// instead of wss:// - do we even allow that?

Sounds good. I'll go with --server-web-socket-thumbprint=<X>

xwipeoutx

A few suggested wording changes, and comment.

I won't approve until I see a major version rev.

xwipeoutx · 2026-09-10T01:55:33Z

            Options.Add("server-web-socket=", "When using active communication over websockets, the address of the Octopus Server, eg 'wss://example.com/OctopusComms'. Refer to http://g.octopushq.com/WebSocketComms", s => serverWebSocketAddress = s);
            Options.Add("reuse-server-thumbprint", "Reuse the Server Thumbprint from the first trusted server instance currently configured", _ => reuseThumbprint = true);
-            Options.Add(ServerCertificateTrustConfirmation.SkipValidationArgumentName, ServerCertificateTrustConfirmation.SkipValidationArgumentDescription, _ => skipCertificateValidation = true);
+            Options.Add("server-web-socket-thumbprint=", "When using active communication over websockets, the thumbprint of your server's websockets certificate, eg 'AB1C2D...'. This is the SSL certificate configured wherever TLS is terminated for the websockets endpoint, not the Octopus Server's own certificate. When supplied the certificate is trusted only if its thumbprint matches, which allows registering against an endpoint whose certificate cannot otherwise be validated (for example a self-signed certificate) without prompting.", s => serverWebSocketThumbprint = s);


"not the Octopus Server's own certificate"

In all likelihood, it is the same SSL cert for HTTPS as WSS - this is kinda misleading.

perhaps "not the Octopus Server's Tentacle Communications certificate"?

See this docs page - which is clearly about the TCP transport not websockets

Changed to not the Octopus Server's Tentacle Communications certificate in all locations that had not the Octopus Server's own certificate

xwipeoutx · 2026-09-10T01:58:39Z

-                    $"{problem} {CannotConfirmExplanation} " +
-                    $"Verify the thumbprint against the one shown by your Octopus Server, then re-run this command with --{SkipValidationArgumentName} to trust it.");
+                    $"{problem} Because this certificate could not be validated, it is not possible to confirm that it belongs to your Octopus Server rather than to an attacker positioned between this machine and the server. " +
+                    "Check that this thumbprint matches your server's websockets certificate - the one configured wherever TLS is terminated for the websockets endpoint, such as IIS, HTTP.SYS or a reverse proxy, and not the Octopus Server's own certificate. " +


I'd just leave out the "and not..." bit.

xwipeoutx

Approved - as discussed, let's not squash-merge this one, I'm not sure the major version bump survives it

. . Renaming Prompt Renaming prompt Review feedback +semver: major PR Wording .

sburmanoctopus requested a review from a team as a code owner September 9, 2026 04:36

sburmanoctopus marked this pull request as draft September 9, 2026 04:36

sburmanoctopus marked this pull request as ready for review September 9, 2026 05:48

xwipeoutx reviewed Sep 9, 2026

View reviewed changes

xwipeoutx reviewed Sep 10, 2026

View reviewed changes

xwipeoutx approved these changes Sep 10, 2026

View reviewed changes

sburmanoctopus added 2 commits September 10, 2026 12:35

Checking certificate

ee57157

. . Renaming Prompt Renaming prompt Review feedback +semver: major PR Wording .

+semver: major

43e8d44

sburmanoctopus force-pushed the levi/certificate-prompting branch from 06387d6 to 43e8d44 Compare September 10, 2026 02:35

sburmanoctopus merged commit 8d114ae into main Sep 10, 2026
48 checks passed

sburmanoctopus deleted the levi/certificate-prompting branch September 10, 2026 04:09

Uh oh!

Conversation

sburmanoctopus commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Results

Testing

Pre-requisites

Uh oh!

xwipeoutx commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xwipeoutx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwipeoutx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xwipeoutx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sburmanoctopus commented Sep 9, 2026 •

edited

Loading

xwipeoutx commented Sep 9, 2026 •

edited

Loading