feat: TLS, HTTP basic auth and config file for AgentCTL #1534

rewenset · 2019-10-21T15:55:11Z

Config File

Default Path: ~/.agentctl/config.yml
Path to directory with config file may be changed with --config-dir flag

Setup TLS

Example of conf file:

➜ cat ~/.agentctl/config.yml
kvdb-tls:
  cert-file: /home/user/certs/etcd1.pem
  key-file: /home/user/certs/etcd1-key.pem
  ca-file: /home/user/certs/ca.pem
  skip-verify: true

grpc-tls:
  cert-file: /home/user/certs/etcd1.pem
  key-file: /home/user/certs/etcd1-key.pem
  skip-verify: true

http-tls:
  disabled: true
➜

insecure-tls: yes in config file is the same as passing --insecure-tls flag. This configuration enables TLS with skip-verify: true (so client does not validate server's certificate) for all connections, even for disabled one (yes, it enables them but without using neither cert-file, nor key-file, and obviously nor ca-file). If TLS config was not disabled and cert-file with key-file are set, then --insecure-tls option will keep that as it is and use them for secure connections.

Agent host configuration

Using defaults (I do that by set config-dir to some random path which not contains any configuration file for agentctl)

➜ agentctl --config-dir=/ status
HTTP request failed: Cannot connect to the agent at 127.0.0.1. Is the agent running?

Using config file only

➜ cat ~/.agentctl/config.yml
host: fromconfig
➜ agentctl status                    
HTTP request failed: error during connect: Get http://fromconfig:9191/readiness: dial tcp: lookup fromconfig: no such host

Using config file together with env variable.

➜ cat ~/.agentctl/config.yml
host: fromconfig
➜ AGENT_HOST=fromenv agentctl status
HTTP request failed: error during connect: Get http://fromenv:9191/readiness: dial tcp: lookup fromenv: no such host

Using config file together with env variable and with flag.

➜ cat ~/.agentctl/config.yml
host: fromconfig
➜ AGENT_HOST=fromenv agentctl --host fromflags status
HTTP request failed: error during connect: Get http://fromflags:9191/readiness: dial tcp: lookup fromflags: no such host

Service label configuration

Config file line: service-label: best-service-ever
Env variable: MICROSERVICE_LABEL= awesome-service
Flag: agentctl --service-label magic-service ...

gRPC and HTTP ports

Config file lines:

grpc-port: 1010
http-port: 2020

Env variables: Not supported
Flag: agentctl --grpc-port 1010 --http-port 2020 ...

ETCD endpoints

Config file lines:

etcd-endpoints:
  - 1.1.1.1:1241
  - 2.2.2.2:3333

Env variable: ETCD_ENDPOINTS= 1.1.1.1:1241,2.2.2.2:3333 (use , as separator)
Flag: agentctl --debug --etcd-endpoints 1.1.1.1:1241 --etcd-endpoints 2.2.2.2:3333 ...
or agentctl --debug --etcd-endpoints 1.1.1.1:1241,2.2.2.2:3333 ...

Basic Auth for HTTP connection:

Without credentials

➜ agentctl log ls agent                             
HTTP request failed: Error response from daemon: [401] Unauthorized.
➜

Credentials provided with --http-basic-auth flag

➜ agentctl --http-basic-auth user:pass log ls agent      
LOGGER LEVEL 
agent  info  
➜

Credentials provided with environment variable AGENTCTL_HTTP_BASIC_AUTH

➜ AGENTCTL_HTTP_BASIC_AUTH=user:pass agentctl log ls agent 
LOGGER LEVEL 
agent  info  
➜

Credentials from config file:

➜  echo "http-basic-auth: user:pass" >> ~/.agentctl/config.yml
➜ agentctl log ls agent 
LOGGER LEVEL 
agent  info  
➜

rastislavs · 2019-10-22T12:00:23Z

Maybe it would be easier to just reuse the exsiting ConfigToClient func from cn-infra?

Anyway, it would be useful if etcd endpoints could be passed via config file as well. Maybe even if any agentctl argument (e.g. service label) could be passed via config file?

cmd/agentctl/cli/config.go

ondrej-fabry · 2019-10-22T14:55:49Z

cmd/agentctl/cli/config.go

+)
+
+// ConfigFile represents info from ~/.agentctl/all.conf.
+type ConfigFile struct {


I agree with comment from @rastislavszabo #1534 (comment)

Anyway, it would be useful if etcd endpoints could be passed via config file as well. Maybe even if any agentctl argument (e.g. service label) could be passed via config file?

This definition should serve as a complete configuration for the AgentCTL.

Initial values for the config should be provided by DefaultConfig() function and used on line 56. Then config file contents should be loaded (possibly overridding some defaults) and after loading config file, the environment variables and program arguments should be applied to the config value. This order should be used to properly handle precedence order:

default config

config file (possibly system and then local)

env vars

program arguments

The Go library viper might be really helpful here and provide simpler way to make this all work automatically.

a complete configuration for the AgentCTL

Sure, complete configuration sounds good. Will work on that too.

The Go library viper might be really helpful

yeah, tab with Viper is open in my browser from the beginning of work on this task, but for now I'm fighting for better TLS (I don't like that code from cn-infra). Definitely will look into Viper, but a little bit later, when I'll be working on a complete configuration and that order of different ways of configurating AgentCTL, I think.

rewenset · 2019-10-22T15:52:42Z

@rastislavszabo thanks for checking out my work! Main problem with reusing is that the mentioned function not only creates connection to ETCD, but also tries to finish with configuration, and I don't want that. The second problem is with tlsutils from the etcd repo. I don't like neither NewCert nor NewCertPool even though it is three times copy-pasted:

NewCert can be replaced with LoadX509KeyPair from crypto/tls and NewCertPool does part of the work already defined for tls.Config struct.
I'll try to propose more convenient way (Yet Another Way To Handle TLS 😁)

ondrej-fabry · 2019-10-22T15:53:23Z

cmd/agentctl/cli/flags.go

-	// TODO: support TLS
-	TLS        bool
-	TLSVerify  bool
-	TLSOptions *tlsconfig.Options


These options will be needed for providing security related options for connection (gRPC) to the agent and should not be used for the KVDB connection.

So grpc is the only thing which can be configured via flags? Right now I'm using --tls flag to tell whether or not use TLS for different connections, for example if all files (cert, key and ca) are defined for kvdb in config file, but I want to connect to local etcd, so I don't need any TLS for now, but later I want to connect to remote etcd with auth enabled so I going to need those files.
I'll rethink this with clear head tomorrow. Now I'm only about to push today's work and go home 😌

Well, I guess we can keep this shared for now since the agentctl uses one connection per single run anyway.

In the future when we start using viper any config option defined for configuration can be overriden automatically from env vars and flags by using BindEnv and BindFlags. So it would be easy to override the TLS configuration from file with using some auto-generated flag --kvdb-tls=false, essentially same as cfg.Kvdb.TLS = false.

cmd/agentctl/client/client.go

rewenset · 2019-10-29T14:39:51Z

I'm done with TLS and it looks like Viper requires big changes for Agenctl, so I think it will be better to continue (although I did not push anything) working on that in a new PR.

EDIT:
I think, I'll push here code which uses Viper only for TLS configuration

cmd/agentctl/cli/flags.go

rastislavs · 2019-11-05T13:06:50Z

Opening some discussion... I think that the config structure should be defined as a struct and viper should only be used for merging all types of config input (config, flag, env..) and apply them to the Config struct using viper.Unmarshal.

yeah, having the config in a struct would be better IMO as well

rewenset · 2019-11-05T13:09:30Z

Opening some discussion... I think that the config structure should be defined as a struct and viper should only be used for merging all types of config input (config, flag, env..) and apply them to the Config struct using viper.Unmarshal.

In such case it would be easier to see what configuration can be applied and we may get rid of viper.GetString... and viper.GetInt.... Also for each field we can add a comment describing what flag and env variable it can be configured with, so this struct will be kind of documentation for configuring the AgentCtl. And there may be some methods for this Config struct such as whether or not use tls for kvdb and etc.

I agree and I'll start to work on that (shouldn't take much time). @ondrej-fabry may you approve this with 👍 on this comment?

rewenset · 2019-11-05T13:21:41Z

It feels like endless PR ✨
I hear tears dropping from face of everyone who advocates for merging into dev daily as CI practice 😁

ondrej-fabry · 2019-11-05T13:46:44Z

It feels like endless PR ✨
I hear tears dropping from face of everyone who advocates for merging into dev daily as CI practice 😁

I feel you.. but better have it done properly at first than just increasing technical debt by putting "refactor.." task into backlog.

rewenset · 2019-11-06T11:51:00Z

Configuration table:

| Name             | Flag             | Short Flag | Env                 | Conf           | Type      | Default            |
|------------------|------------------|------------|---------------------|----------------|-----------|--------------------|
| LigatoAPIVersion |                  |            | LIGATO_API_VERSION  |                | string    | "" (empty string)  |
| Host             | --host           | -H         | AGENT_HOST          | host           | string    | "127.0.0.1"        |
| ServiceLabel     | --service-label  |            | MICROSERVICE_LABEL  | service-label  | string    | "" (empty string)  |
| GRPCPort         | --grpc-port      |            |                     | grpc-port      | int       | 9111               |
| HTTPPort         | --http-port      |            |                     | http-port      | int       | 9191               |
| ETCDEndpoints    | --etcd-endpoints | -e         | ETCD_ENDPOINTS      | etcd-endpoints | []string  | ["127.0.0.1:2379"] |
| BasicAuth        | --basic-auth     |            | AGENTCTL_BASIC_AUTH | basic-auth     | string    | "" (empty string)  |
| UseTLS           | --tls            |            |                     | use-tls        | bool      | false              |
| GRPCSecure       |                  |            |                     | grpc-tls       | TLSConfig | nil                |
| HTTPSecure       |                  |            |                     | http-tls       | TLSConfig | nil                |
| KVDBSecure       |                  |            |                     | kvdb-tls       | TLSConfig | nil                |
|                                                                                                     |                    |
| Not in Config struct:                                                                               |                    |
|                  | --config-dir     |            |                     |                | string    | ""                 |
|                  | --debug          | -D         |                     |                | bool      |                    |
|                  | --version        | -v         |                     |                | bool      |                    |
|                  | --log-level      | -l         |                     |                | string    |                    |
|                  | --help           | -h         |                     |                | bool      |                    |

cmd/agentctl/cli/config.go

ondrej-fabry · 2019-11-06T11:58:10Z

cmd/agentctl/cli/viper.go

+func viperReadInConfig() {
+	if err := viper.ReadInConfig(); err != nil {
+		if _, ok := err.(viper.ConfigFileNotFoundError); ok {
+			logging.Debug("unable to find config file")


Better to print the actual error here too. It contains more error context about where and what file was being accessed:

https://github.com/spf13/viper/blob/72b022eb357a56469725dcd03918449e2278d02e/viper.go#L115

ondrej-fabry · 2019-11-06T12:02:11Z

That configuration table is slick!

It would be nice to have a way to print this with something like vpp-agent --print-config-table

..or perhaps even better with some agentctl command remotely?

rewenset · 2019-11-06T12:23:42Z

That configuration table is slick! Would be nice to have a way to print this with something like vpp-agent --print-config-table

..or perhaps even better with some agentctl command remotely?

I'm glad you like it! I agree, it would be nice to have something like this printed by agentctl. But

it was created manually and I'm not sure how to automate this
it was created to give high overview of current state of things

And also, I vote for separate command to do that, so with flags you may change columns printed and maybe level of verbosity. To existing columns, I would add "example" and "current value" and maybe "comment" too.

But it is another task and not suits in this PR! :)

Let's finish with this first, please. And, in a first place, by putting that "high overview" configuration table, I wanted to receive some feedback. Maybe we need rename something or add more ENV variables or change defaults.

rastislavs · 2019-11-07T11:50:28Z

Thanks for the table and all the work! I have some comments, but I would prefer finally merging this PR and addressing that (as well as the table you were discussing) in a separate PR.

I would have one more config option candidate: ETCD dial-timeout (currently facing an issue with the hard-coded value)
Is BasicAuth used only fot HTTP connections? If yes, what about naming it HTTPBasicAuth?
is the --tls flag really needed?

rewenset · 2019-11-07T12:44:52Z

@rastislavszabo

ETCD dial-timeout

sure

Is BasicAuth used only fot HTTP connections? If yes, what about naming it HTTPBasicAuth?

Yes, it is for HTTP only. Before now, I thought that basic auth is HTTP thing only. I'll rename it so it will be more clear

is the --tls flag really needed?

I was thinking that user may not want to verify server's cert (or use host's CA set) so no need in CA file and server may not be configured to require client's cert so no need in cert and key files. In such case user just want that secure connection to work without any files and to achieve that you just add --tls flag and boom you're making secure requests now.

Somehow I've forgot about that and it's not working with latest changes. Thanks for pointing that out. In a current state there are two options: either remove that --tls completely or get that feature back.

Example of what I wanted to achieve:

gRPC connection

➜ agentctl --config-dir=/ dump all  
rpc error: code = Unavailable desc = all SubConns are in TransientFailure, latest connection error: connection closed
➜ agentctl --config-dir=/ --tls dump all
KEY                             VALUE                               ORIGIN    METADATA                                                                           
config/vpp/v2/interfaces/tap1   [ligato.vpp.interfaces.Interface]   from-NB   map[IPAddresses:[10.10.10.10/24] SwIfIndex:2 TAPHostIfName:tap-3058328937 Vrf:0]   
                                name: "tap1"                                                                                                                     
                                type: TAP                                                                                                                        
                                enabled: true                                                                                                                    
                                ip_addresses: "10.10.10.10/24"                                                                                                   
                                tap: <                                                                                                                           
                                  version: 2                                                                                                                     
                                >                                                                                                                                
                                                                              
➜

HTTP connection

➜ agentctl --config-dir=/ log ls agent       
HTTP request failed: Error response from daemon: [400] Client sent an HTTP request to an HTTPS server.
➜ agentctl --config-dir=/ --tls log ls agent
LOGGER LEVEL 
agent  info  
➜

KVDB connection (indeed timeout will be nice thing to have)

➜ agentctl --config-dir=/ kvdb ls      
WARN[0003] Failed to connect to Etcd: context deadline exceeded  endpoints="[127.0.0.1:2379]" loc="etcd/bytes_broker_impl.go(60)" logger=etcd-client
connecting to KVDB failed: connecting to Etcd failed: context deadline exceeded
➜ agentctl --config-dir=/ --tls kvdb ls
/vnf-agent/vpp/foo
bar
➜

EDIT:
by setting --config-dir=/ I disable my own config which is in default (~/.agentctl) directory and don't use any config file for agentctl

rastislavs · 2019-11-07T12:54:57Z

I was thinking that user may not want to verify server's cert (or use host's CA set) so no need in CA file and server may not be configured to require client's cert so no need in cert and key files. In such case user just want that secure connection to work without any files and to achieve that you just add --tls flag and boom you're making secure requests now.

Good idea, but maybe I would use different naming. E.g. with curl, you can achieve something similar with --insecure flag. What about --insecure-tls ?

rewenset · 2019-11-07T13:09:53Z

What about --insecure-tls ?

And by providing that flag, user will know that no verification of server's certificate is done behind the scenes, so it is basically insecure security 😁. Looks like a good naming choice 👍

And what if there is some TLS configuration? Will this flag override all connections to not verify server's certificate (and therefore skip CA file if set) and just send our cert (if it is set) for "authentication with HTTPS client certificates"?

rastislavs · 2019-11-07T13:20:32Z

And what if there is some TLS configuration? Will this flag override all connections to not verify server's certificate (and therefore skip CA file if set) and just send our cert (if it is set) for "authentication with HTTPS client certificates"?

yeah, this seems to be the best option

test-e2e

rastislavs · 2019-11-08T12:31:45Z

tests/e2e/resources/agentctl.conf

@@ -0,0 +1,16 @@
+use-tls: true


this option is already outdated, isn't it?

it is, thanks

Securing connection to ETCD

174fe57

rewenset added ⚡️ feature 🚧 WIP do not merge! work in progress! labels Oct 21, 2019

rewenset added 2 commits October 22, 2019 13:47

Refactor creation of secure connection with ETCD

d9cf5ff

Update dep

880533f

ondrej-fabry reviewed Oct 22, 2019

View reviewed changes

cmd/agentctl/cli/config.go Outdated Show resolved Hide resolved

ondrej-fabry reviewed Oct 22, 2019

View reviewed changes

Yet another way to handle TLS configuration

17b134a

golangcibot reviewed Oct 22, 2019

View reviewed changes

cmd/agentctl/client/client.go Outdated Show resolved Hide resolved

Securing connection to gRPC

4d732b9

golangcibot reviewed Oct 23, 2019

View reviewed changes

cmd/agentctl/client/client.go Outdated Show resolved Hide resolved

rewenset added 2 commits October 23, 2019 13:30

Configuration file renamed

7d02620

Securing connection via HTTP

d946e19

rewenset commented Oct 23, 2019

View reviewed changes

cmd/agentctl/client/client.go Outdated Show resolved Hide resolved

rewenset added 2 commits October 24, 2019 10:37

Fix schema for TLS, fix retrieving http client

6b1dce1

Do not use new Clone() method on Transport. Support older versions of Go

2afe5b5

ondrej-fabry reviewed Oct 25, 2019

View reviewed changes

cmd/agentctl/client/client.go Outdated Show resolved Hide resolved

Support Go1.12 and higher

a0a8477

rewenset changed the title ~~feat: Support secure connections from AgentCTL [WIP]~~ feat: Support secure connections from AgentCTL Oct 29, 2019

rewenset removed the 🚧 WIP do not merge! work in progress! label Oct 29, 2019

rewenset requested a review from ondrej-fabry October 29, 2019 14:40

golangcibot reviewed Oct 29, 2019

View reviewed changes

cmd/agentctl/cli/flags.go Outdated Show resolved Hide resolved

cmd/agentctl/cli/flags.go Show resolved Hide resolved

Use Viper for TLS configuration

a3a84b2

rewenset force-pushed the agentctl-secure branch from fe979e3 to a3a84b2 Compare October 30, 2019 07:12

rewenset added 2 commits October 30, 2019 13:05

Use Viper for agent host configuration

07e55ad

Merge dev with go modules 🎉

b5eb702

rewenset added the 🚧 WIP do not merge! work in progress! label Nov 6, 2019

rewenset added 2 commits November 6, 2019 13:46

Store configuration in a struct

0027932

Shorten comment

ec106ae

golangcibot reviewed Nov 6, 2019

View reviewed changes

cmd/agentctl/cli/config.go Outdated Show resolved Hide resolved

cmd/agentctl/cli/config.go Outdated Show resolved Hide resolved

cmd/agentctl/cli/config.go Outdated Show resolved Hide resolved

Remove bool comparison with bool constant

12cc8cc

ondrej-fabry reviewed Nov 6, 2019

View reviewed changes

Output error in logs when config file not found

8c1bf98

rewenset removed the 🚧 WIP do not merge! work in progress! label Nov 6, 2019

rewenset requested a review from ondrej-fabry November 6, 2019 13:29

Remove --tls flag and add --insecure-tls. Add e2e test to test it

b0ebbb7

test-e2e

rewenset force-pushed the agentctl-secure branch from d66e132 to b0ebbb7 Compare November 8, 2019 08:54

rewenset added 2 commits November 8, 2019 11:42

Add dial timeout configuration for kvdb (etcd) connection

4709ec4

Rename basic auth to http basic auth

4f53298

rastislavs reviewed Nov 8, 2019

View reviewed changes

Fix config for agentctl in e2e tests

0c647e1

rastislavs self-requested a review November 8, 2019 13:08

rastislavs approved these changes Nov 8, 2019

View reviewed changes

rastislavs merged commit 9f7bf3b into ligato:dev Nov 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: TLS, HTTP basic auth and config file for AgentCTL #1534

feat: TLS, HTTP basic auth and config file for AgentCTL #1534

rewenset commented Oct 21, 2019 •

edited

Loading

rastislavs commented Oct 22, 2019

ondrej-fabry Oct 22, 2019 •

edited

Loading

rewenset Oct 22, 2019 •

edited

Loading

rewenset commented Oct 22, 2019

ondrej-fabry Oct 22, 2019

rewenset Oct 22, 2019

ondrej-fabry Oct 22, 2019 •

edited

Loading

rewenset commented Oct 29, 2019 •

edited

Loading

rastislavs commented Nov 5, 2019

rewenset commented Nov 5, 2019

rewenset commented Nov 5, 2019

ondrej-fabry commented Nov 5, 2019

rewenset commented Nov 6, 2019

ondrej-fabry Nov 6, 2019 •

edited

Loading

ondrej-fabry commented Nov 6, 2019 •

edited

Loading

rewenset commented Nov 6, 2019 •

edited

Loading

rastislavs commented Nov 7, 2019

rewenset commented Nov 7, 2019 •

edited

Loading

rastislavs commented Nov 7, 2019 •

edited

Loading

rewenset commented Nov 7, 2019

rastislavs commented Nov 7, 2019

rastislavs Nov 8, 2019

rewenset Nov 8, 2019

feat: TLS, HTTP basic auth and config file for AgentCTL #1534

feat: TLS, HTTP basic auth and config file for AgentCTL #1534

Conversation

rewenset commented Oct 21, 2019 • edited Loading

Config File

Setup TLS

Agent host configuration

Service label configuration

gRPC and HTTP ports

ETCD endpoints

Basic Auth for HTTP connection:

rastislavs commented Oct 22, 2019

ondrej-fabry Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

rewenset Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

rewenset commented Oct 22, 2019

ondrej-fabry Oct 22, 2019

Choose a reason for hiding this comment

rewenset Oct 22, 2019

Choose a reason for hiding this comment

ondrej-fabry Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

rewenset commented Oct 29, 2019 • edited Loading

rastislavs commented Nov 5, 2019

rewenset commented Nov 5, 2019

rewenset commented Nov 5, 2019

ondrej-fabry commented Nov 5, 2019

rewenset commented Nov 6, 2019

ondrej-fabry Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

ondrej-fabry commented Nov 6, 2019 • edited Loading

rewenset commented Nov 6, 2019 • edited Loading

rastislavs commented Nov 7, 2019

rewenset commented Nov 7, 2019 • edited Loading

rastislavs commented Nov 7, 2019 • edited Loading

rewenset commented Nov 7, 2019

rastislavs commented Nov 7, 2019

rastislavs Nov 8, 2019

Choose a reason for hiding this comment

rewenset Nov 8, 2019

Choose a reason for hiding this comment

rewenset commented Oct 21, 2019 •

edited

Loading

ondrej-fabry Oct 22, 2019 •

edited

Loading

rewenset Oct 22, 2019 •

edited

Loading

ondrej-fabry Oct 22, 2019 •

edited

Loading

rewenset commented Oct 29, 2019 •

edited

Loading

ondrej-fabry Nov 6, 2019 •

edited

Loading

ondrej-fabry commented Nov 6, 2019 •

edited

Loading

rewenset commented Nov 6, 2019 •

edited

Loading

rewenset commented Nov 7, 2019 •

edited

Loading

rastislavs commented Nov 7, 2019 •

edited

Loading