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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c6e160
support TLS Insecure connection - added flag --tlsinsecure
mschneider82 Jul 25, 2024
efb23c4
improved tlsinsecure
mschneider82 Jul 25, 2024
e15ea14
also set secure true
mschneider82 Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ var (

// These are persisted by contexts, as properties thereof.
// So don't include NATS_CONTEXT in this list.
overrideEnvVars = []string{"NATS_URL", "NATS_USER", "NATS_PASSWORD", "NATS_CREDS", "NATS_NKEY", "NATS_CERT", "NATS_KEY", "NATS_CA", "NATS_TIMEOUT", "NATS_SOCKS_PROXY", "NATS_COLOR"}
overrideEnvVars = []string{"NATS_URL", "NATS_USER", "NATS_PASSWORD", "NATS_CREDS", "NATS_NKEY", "NATS_CERT", "NATS_KEY", "NATS_CA", "NATS_TIMEOUT", "NATS_SOCKS_PROXY", "NATS_COLOR", "NATS_TLSINSECURE"}
)

func registerCommand(name string, order int, c func(app commandHost)) {
Expand Down
5 changes: 5 additions & 0 deletions cli/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"bufio"
"bytes"
"context"
"crypto/tls"
"encoding/base64"
"encoding/json"
"errors"
Expand Down Expand Up @@ -307,6 +308,10 @@ 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.

Copy link

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 •
edited
Loading

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.

copts = append(copts, nats.Secure(&tls.Config{InsecureSkipVerify: true}))
mschneider82 marked this conversation as resolved.
Show resolved Hide resolved
}

return append(copts, []nats.Option{
nats.Name(connectionName),
nats.MaxReconnects(-1),
Expand Down
1 change: 1 addition & 0 deletions nats/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ See 'nats cheat' for a quick cheatsheet of commands`
ncli.Flag("tlskey", "TLS private key").Envar("NATS_KEY").PlaceHolder("FILE").ExistingFileVar(&opts.TlsKey)
ncli.Flag("tlsca", "TLS certificate authority chain").Envar("NATS_CA").PlaceHolder("FILE").ExistingFileVar(&opts.TlsCA)
ncli.Flag("tlsfirst", "Perform TLS handshake before expecting the server greeting").BoolVar(&opts.TlsFirst)
ncli.Flag("tlsinsecure", "Disable TLS Certificate Verification").Envar("NATS_TLSINSECURE").BoolVar(&opts.TlsInsecure)
if runtime.GOOS == "windows" {
ncli.Flag("certstore", "Uses a Windows Certificate Store for TLS (user, machine)").PlaceHolder("TYPE").EnumVar(&opts.WinCertStoreType, "user", "windowscurrentuser", "machine", "windowslocalmachine")
ncli.Flag("certstore-match", "Which certificate to use in the store").PlaceHolder("QUERY").StringVar(&opts.WinCertStoreMatch)
Expand Down
2 changes: 2 additions & 0 deletions options/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ type Options struct {
TlsKey string
// TlsCA is the certificate authority to verify the connection with
TlsCA string
// TlsInsecure Disable TLS Certificate Verification
TlsInsecure bool
// Timeout is how long to wait for operations
Timeout time.Duration
// ConnectionName is the name to use for the underlying NATS connection
Expand Down