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

#181 support TLS Insecure connection - added flag --tlsinsecure #1098

Closed
wants to merge 3 commits into from

Conversation

mschneider82
Copy link

@mschneider82 mschneider82 commented Jul 25, 2024

This PR adds a new flag for --tlsinsecure or env NATS_TLSINSECURE
Since the server side supports it, the cli should also support it.

fixes #181

tested in a pod:

nats-box:/# ./nats obj ls 
nats: error: tls: failed to verify certificate: x509: certificate is valid for ::1, 127.0.0.1, not 10.1.1.1

nats-box:/# ./nats obj ls --tlsinsecure
╭────────────────────────────────────────────────────────────────────╮
│                        Object Store Buckets                        │
├─────────┬─────────────┬─────────────────────┬────────┬─────────────┤
│ Bucket  │ Description │ Created             │ Size   │ Last Update │
│ test    │             │ 2024-07-22 08:31:17 │ 12 KiB │ 57m3s       │
╰─────────┴─────────────┴─────────────────────┴────────┴─────────────╯

cli/util.go Outdated Show resolved Hide resolved
@mschneider82 mschneider82 requested a review from ripienaar July 25, 2024 08:50
@@ -307,6 +308,18 @@ func natsOpts() []nats.Option {
connectionName = "NATS CLI Version " + Version
}

if opts().TlsInsecure {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe custom tlsconfigs has to be set using nats.Secure().

Again, this works today only due to chance and due to flow of nats.go which can change at any time. This feature has to go into nats.go please engage there and add an option so it's done right and we know it will work and be maintained in the long term.

Copy link
Author

Choose a reason for hiding this comment

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

What makes me a little confusion is the Description of nats.Secure() :

// Secure is an Option to enable TLS secure connections that skip server verification by default.
// Pass a TLS Configuration for proper TLS.
// A TLS Configuration using InsecureSkipVerify should NOT be used in a production setting.

In my first PR i tried to use nats.Secure() to enable InsecureSkipVerify, this only works if i supply a tls.Config{} struct. So currently nats.Secure() is broken because it doesnt handle the mentioned Auth things?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Secure() without a tlsc sets the Secure option true

https://github.com/nats-io/nats.go/blob/c693ec3784c00cf5f9ad9c0f4010b352482ab86d/nats.go#L880

Later this is called that will create a tlsc and set all the things so just calling Secure() would be fine

https://github.com/nats-io/nats.go/blob/c693ec3784c00cf5f9ad9c0f4010b352482ab86d/nats.go#L2088

if you do want to set a tls at least also set MinVersion: tls.VersionTLS12 here

Copy link
Collaborator

Choose a reason for hiding this comment

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

But all that aside, I am still not happy supporting this option.

Why is your TLS setup broken that you need this?

Copy link
Author

Choose a reason for hiding this comment

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

This is for development env, I embedded nats js server and didnt want to use plain connection, plain is way more insecure than using a self signed tls.

I have tested just calling nats.Secure() without an tls.Config and the tls failure still exists.

Choose a reason for hiding this comment

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

@mschneider82: I cloned the repository, applied the changes and installed the adjusted CLI. Finally I can just connect to all our test environments and see what is going on with my JetStream subscriptions -- without having to set up each of them with the right certificates locally.

This is worth gold! 🥇

Thank you!

Copy link
Author

Choose a reason for hiding this comment

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

Yes the patched Version, especially with the env NATS_TLSINSECURE set is total fine. It could be so simple to implement...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes its not if you can but if you should.

The need here, as stated, is laziness and not technical need .. "connect to all our test environments...without having to set up each of them with the right certificates locally." security is often inconvenient but enabling it to be bypassed is often ill advised.

I guess you love your rsh with zero password prompts to root user with no encryption or security - because it's convenient and you just don't like the effort of setting up a correct system?

Yeah, we don't do that, we set up TLS correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We already support bypassing TLS - just don't set it up since disabling key aspects of it has the same outcome.

Copy link

@tehsphinx tehsphinx Oct 10, 2024

Choose a reason for hiding this comment

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

We already support bypassing TLS - just don't set it up since disabling key aspects of it has the same outcome.

Can you elaborate?

But note, that I'm not interested in bypassing TLS. I'm just interested in disabling the server certificate verification in natsCLI. The communication is still TLS encrypted.

Exactly what this PR does: it doesn't disable TLS. Just allow connection to a server with a certificate it can't verify against a CA.

@liaoya
Copy link

liaoya commented Aug 16, 2024

This feature is very helpful and even our production use insecure certificate.

I am looking forward it in the next release.

@ripienaar
Copy link
Collaborator

If setting up TLS correctly to get the benefits of TLS including man in tbe middle attack prevention is too much effort. Then don’t set up TLS and make it clear to everyone you don’t care for those features.

Selectively disabling things like peer verification means you may as well not set up TLS.

@tehsphinx
Copy link

tehsphinx commented Oct 14, 2024

Selectively disabling things like peer verification means you may as well not set up TLS.

@ripienaar How is setting setting InsecureSkipVerify on the natsCLI equal to disabling peer verification? The natsCLI is not a peer. It is a debugging tool. (At least for the use case I'd like to be able to use InsecureSkipVerify for.)

@ripienaar
Copy link
Collaborator

It’s much more than a debugging tool. It’s capable of creating jetstream assets, removing them, destroying data, creating accounts and users and severely interrupting even destroying jetstream clusters.

It’s a full admin tool and equivalent to root level access for the nats server.

@ripienaar ripienaar closed this Oct 14, 2024
@tehsphinx
Copy link

tehsphinx commented Oct 15, 2024

It’s much more than a debugging tool. It’s capable of creating jetstream assets, removing them, destroying data, creating accounts and users and severely interrupting even destroying jetstream clusters.

It’s a full admin tool and equivalent to root level access for the nats server.

This doesn't answer the question.

Note, that I never said the NATS cli is merely a debug tool, but it is in the use case I need InsecureSkipVerify for. Also note that this does not degrade the security of the cluster as I'm not changing anything on the cluster side.

Overall I see a lot of made up reasons to dismiss this MR. A lot of aggression and assumptions, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow TLS verification to be skipped
4 participants