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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
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
14 changes: 14 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,19 @@ 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
Collaborator

Choose a reason for hiding this comment

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

if nats.Secure() doesn't do it, then again this should be fixed in nats.go

I strongly believe delegating tls setup to nats.go is the right thing to do - as security design, minimal versions or other constraints evolve we will forget to do same here when done in nats.go. Relying on nats.go to do the right thing is the safest behaviour.

Copy link
Author

Choose a reason for hiding this comment

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

Ok i will prepare a PR for that

insecureOption := func(o *nats.Options) error {
o.Secure = true
if o.TLSConfig == nil {
o.TLSConfig = &tls.Config{InsecureSkipVerify: true}
} else {
o.TLSConfig.InsecureSkipVerify = true
}
return nil
}
copts = append(copts, insecureOption)
}

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