Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

config,cmds: prefix admin- to tls-{cert,key}-file settings #1013

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

jchappelow
Copy link
Member

Resolves #999 (except for docs repo, which I'll address too)

@jchappelow
Copy link
Member Author

I just recalled that I need to put some deprecation code for these settings for config.toml how was done for the RPC request limit setting recently.

@jchappelow
Copy link
Member Author

I just recalled that I need to put some deprecation code for these settings for config.toml how was done for the RPC request limit setting recently.

This is done now.

@@ -115,7 +115,9 @@ private_key_path = "{{ .AppConfig.PrivateKeyPath }}"
# TCP address for the KWILD App's JSON-RPC server to listen on
jsonrpc_listen_addr = "{{ .AppConfig.JSONRPCListenAddress }}"

# Unix socket or TCP address for the KWILD App's Admin GRPC server to listen on
# Unix socket or TCP address for the KWILD App's Admin GRPC server to listen on.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Admin service does not use GRPC anymore, so it might be worth updating this comment

@@ -166,20 +168,17 @@ pg_db_name = "{{ .AppConfig.DBName }}"
# any TCP listen address.
admin_notls = {{ .AppConfig.NoTLS }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this totally unnecessary now since tls only gets configured for admin?

At what point would anyone set this to true?

Copy link
Member Author

Choose a reason for hiding this comment

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

So this can change, but right now TLS automatically gets enabled if the listen address is not either unix or a loopback tcp. The cert/key being empty in the config does not disable it (as it does with the user service) as the defaults to use in the aforementioned case are just rpc.cert/rpc.key in the root directory.
Thus, @charithabandi uses admin_notls to bypass this so test containers' admin service can be accessed via non-loopback TCP.

I'll take another a look but I think we want to enable TLS if it's listening on an interface that looks like it's going to be accessed from a remote machine. Two types of authentication can be used: password with admin_pass, which dangerous if the transport is not encrypted, or mTLS.

Will look again.

@brennanjl brennanjl merged commit cb6b0a4 into kwilteam:main Sep 27, 2024
2 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

app.tls_cert_file and app.tls_key_file are incorrectly documented
2 participants