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

tls: support multiple key/cert pairs, for SNI #2029

Closed
wants to merge 1 commit into from

Conversation

philpennock
Copy link
Member

This does not add tests or docs, this is to sound out how people feel about the
API. If we like it, we can dress it up formally.

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.

/cc @nats-io/core

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

add a unit test so this functionality does not disappear?

@wallyqs
Copy link
Member

wallyqs commented Mar 23, 2021

Thanks @philpennock ok sounds like we should add a field to support multiple certs optionally then:

tls {
  certs = [{cert_file = "first.pem", key_file = "first.key"}]
}

@ripienaar
Copy link
Contributor

Agree with @wallyqs that's a lot clearer

@philpennock
Copy link
Member Author

It's still my intent to find time to work on the new config layout and write the tests (but I'm happy for someone else to take it if they have time). The push just now is not me doubling-down on the quick hack proof-of-concept, it's just that I have this live at home and I wanted to install 2.2.3 so I rebased and pushed after confirming it works.

@derekcollison
Copy link
Member

What is the status here?

@philpennock
Copy link
Member Author

This is still useful functionality to have, but needs to be done more cleanly, as wally requested. I'm still running with this at home (to dual-name with a .lan hostname from my personal cert authority, and with a Let's Encrypt cert for the current external IP) and we today used this patch for a fiddly migration, to be able to cut DNS over to a box at our leisure.

So the use-case is definitely there and it needs a better integration than the proof-of-concept with the path-separator strings, and tests to protect against regression.

@philpennock philpennock marked this pull request as draft November 12, 2022 02:51
@bruth
Copy link
Member

bruth commented Apr 23, 2023

Is this worth revisiting and prioritizing?

@derekcollison
Copy link
Member

You should sync with @philpennock

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.
@philpennock
Copy link
Member Author

Closing in favor of Wally's #4889 which does the configuration the Right Way for nats-server, instead of the old-fashioned Unix way. 😆

derekcollison added a commit that referenced this pull request Dec 15, 2023
This adds a `certs` option to the `tls` block to support loading
multiple certs:
    
```hcl
    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
@bruth bruth deleted the pdp/multi-cert-tls branch December 29, 2023 16:35
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

6 participants