-
Notifications
You must be signed in to change notification settings - Fork 11
Allow insecure connections (self signed cert) #463
Conversation
log.Verboseln("SessionId " + headers.Get("X-Qlik-Session")) | ||
|
||
var dialer = enigma.Dialer{} | ||
dialer.TLSClientConfig = tlsClientConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good simplification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, nice addition. Just wondering why we changed the default web-socket url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, some minor comments
cmd/flags.go
Outdated
globalFlags.Bool("bash", false, "Bash flag used to adapt output to bash completion format") | ||
globalFlags.MarkHidden("bash") | ||
globalFlags.String("context", "", "Name of the context used when connecting to Qlik Associative Engine") | ||
globalFlags.BoolP("allow-insecure", "i", false, "Enabling allow-insecure will make it possible to connect using self signed certs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion we should skip the shorthand flag, since it is not recommended to ignore certificate validation (even if we are using self signed certificates. Also change certs
-> certificates
docs/spec.json
Outdated
"clispec": "0.1.0", | ||
"x-qlik-stability": "stable", | ||
"flags": { | ||
"allow-insecure": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also do a patch bump on the api spec version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Rename certificates to tlsClientConfig since it will configure both:
The tlsClientConfig will also be created by default now (possible problem?)
The path for connecting to engine is set to "/app/engineData/ttl/" for allowing pass-thru for Qlik Sense Proxy.
An
User-Agent
headercorectl/<version> (<os>)
is added to be able to track connections in ingress etc