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

Add 'certs' option to TLS block for multi-certs support #4889

Merged
merged 2 commits into from Dec 15, 2023

Conversation

wallyqs
Copy link
Member

@wallyqs wallyqs commented Dec 15, 2023

This adds a certs option to the tls block to support loading multiple certs:

    tls {
      certs = [
        {
          cert_file: "./configs/certs/srva-cert.pem"
          key_file:  "./configs/certs/srva-key.pem"
        },
        {
          cert_file: "./configs/certs/srvb-cert.pem"
          key_file:  "./configs/certs/srvb-key.pem"
        }
      ]
      ca_file: "./configs/certs/ca.pem"
      verify:  true
      timeout: 2
    }

Follow up from #2029

This does not add tests or docs, this is to sound out how people feel about the
API.

Turn the TLS options cert_file and key_file into list-separated paths (so
colon-delimited if on Unix, like `$PATH`); the count of entries must match, and
the keys and certs should zip together.

With this change, TLS SNI works to pick the correct cert to return for a given
connection, allowing a NATS server to have multiple identities.
Copy link
Member

@philpennock philpennock left a comment

Choose a reason for hiding this comment

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

What happens if given a config block with a present-but-empty list of certificates?

tls {
  certs: []
}

server/opts.go Outdated
case "key_file":
certPair.KeyFile = file
default:
return nil, &configErr{tk, fmt.Sprintf("error parsing tls certs config, unknown field [%q]", k)}
Copy link
Member

Choose a reason for hiding this comment

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

tiny nit, %q will already quote, so the square brackets seem redundant

@wallyqs
Copy link
Member Author

wallyqs commented Dec 15, 2023

What happens if given a config block with a present-but-empty list of certificates?

tls {
  certs: []
}

that would have been the same as doing this:

tls {
}

which also makes the server start with TLS 'configured' but actually cannot be used. I think we should change to prevent this?
In releases so far doing tls {} would result in this:

[86149] 2023/12/15 11:27:44.329219 [INF] TLS required for client connections
[86149] 2023/12/15 11:27:44.329509 [INF] Server is ready
[86149] 2023/12/15 11:27:59.796999 [ERR] 127.0.0.1:62606 - cid:5 - TLS handshake error: tls: no certificates configured

I think this needs a separate bug fix, since need to consider the leafnode TLS remote case where no cert is configured but want to change the tls timeout:

		  remotes = [
		    {
		      url: "tls://nats:7422"
		      tls {
		        timeout: 5
		      }
		    }
		  ]

@wallyqs wallyqs marked this pull request as ready for review December 15, 2023 19:39
@wallyqs wallyqs requested a review from a team as a code owner December 15, 2023 19:39
server/opts.go Outdated Show resolved Hide resolved
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM - Minor nit

server/opts.go Outdated Show resolved Hide resolved
```
tls {
  certs = [
    {
      cert_file: "./configs/certs/srva-cert.pem"
      key_file:  "./configs/certs/srva-key.pem"
    },
    {
      cert_file: "./configs/certs/srvb-cert.pem"
      key_file:  "./configs/certs/srvb-key.pem"
    }
  ]
  ca_file: "./configs/certs/ca.pem"
  verify:  true
  timeout: 2
}
```

Signed-off-by: Waldemar Quevedo <wally@nats.io>
@derekcollison derekcollison merged commit 8e8e1d7 into main Dec 15, 2023
4 checks passed
@derekcollison derekcollison deleted the multi-cert-tls branch December 15, 2023 22:50
wallyqs added a commit that referenced this pull request Dec 15, 2023
Includes:

- Check all filterSubjects of consumers when purging a stream (#4873)
- Move tokenizing of subject after early returns (#4880)
- Move helper subjectInfo() to the file where it's used (#4881)
- JSAPI internal routing and reporting and Source and Mirror setup (#4884)
- Do not load all blocks for NumPending when delivery is LastPerSubject. (#4885)
- Detect corrupt psim subjects during recovery of index.db (#4890)
- Add 'certs' option to TLS block for multi-certs support (#4889)
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.

None yet

3 participants