Envoy router verifies ate apiserver's serving cert#237
Conversation
Zoe Zhao (zoez7)
force-pushed
the
mtls
branch
3 times, most recently
from
df3d9a0 to
dfb1db1
Compare
|
Can you talk a little bit about the background of splitting out separate CAs / signers for the workerpools and ate-system components? If at all possible, I would prefer to restrict us to two signers:
The reason is that we can be reasonably confident that signers like these will land in upstream K8s in the near-ish future. The certificates here aren't special or unique to substrate, and they aren't part of the value proposition of the project. So I think we should plan to rebase on native K8s capabilities as they become available. That's easiest if we don't get very custom with the signers. |
|
I think the main motivations for splitting out separate CAs / signers was, if I mount the "podidentity" CA bundle as trusted client CAs in ate-system services, workerpool pods (therefore actors) would also be able to connect to ate-system servers like the Valkey cluster and atelet server. But after typing it out loud - it seems this needs to be solved anyway (probably in a later milestone) by authorization inside servers such as valkey, instead of using separate CAs. Does that make sense to you? |
Yeah --- we should always draw a bright line between authentication and authorization. Assuming that a given client is authorized just because they were able to get a certificate from a particular CA has caused a lot of security problems in the past. Unfortunately, for the specific case of Valkey we might be limited by its capabilities, in which case it might require a separate signer. Let me check the docs. |
|
It looks like valkey can do it: https://valkey.io/topics/tls/#:~:text=Client%20certificate%20authentication |
Zoe Zhao (zoez7)
force-pushed
the
mtls
branch
2 times, most recently
from
c80f243 to
683d085
Compare
| Namespace string | ||
| Kubeconfig string | ||
| AteapiAddr string | ||
| AteapiClientCertPath string |
There was a problem hiding this comment.
Can we group the cert and auth related config into its own struct? Makes it easier to read:
type AuthConfig struct {...}
| } | ||
|
|
||
| func fileExists(path string) bool { | ||
| if path == "" { |
There was a problem hiding this comment.
Do we need this case actually? Empty file will get a does not exist error anyway.
| return false | ||
| } | ||
| _, err := os.Stat(path) | ||
| return err == nil |
There was a problem hiding this comment.
not clear we should eat the error if it's not a ENOENT error
| @@ -62,22 +58,24 @@ func init() { | |||
|
|
|||
| // RouterConfig holds deployment setup and endpoint options for the router node instance. | |||
| type RouterConfig struct { | |||
There was a problem hiding this comment.
suggest:
- Move RouterConfig to its own file (config.go)
- Make it not exported (rename to routerConfig)
- make ateAPITransportCreds() a member function
- rename ateAPITransportCreds to apiTransportCredentials
| } | ||
|
|
||
| func ateapiTLSConfig(cfg RouterConfig) (*tls.Config, error) { | ||
| haveCA := fileExists(cfg.AteapiCACertsPath) |
There was a problem hiding this comment.
suggest: just call stat and return the error. If the file exists, it should stat() with no error anyway.
3 participants
Part of #170
Establishes mutual TLS between the atenet router and ate-apiserver, and updates the certificate plumbing it depends on.
Main changes:
InsecureSkipVerify.Minor bug fixes:
servicednssignerfrom signing a cert with no DNS SANs. Also updated valkey cluster's cert configuration, because it was relying on the cert with empty DNS.