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.

Sign up for GitHub

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

add client cert support #70

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add client cert support #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f118623
add client cert support
britcey Sep 22, 2020
9154509
updates based on PR feedback
britcey Sep 23, 2020
543004f
Merge branch 'master' into feature/client_certs
krasi-georgiev Sep 30, 2020
9cf450d
return jira.BasicAuthTransport.Client if client certs/CAcert are not …
britcey Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 100 additions & 4 deletions cmd/jiralert/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@
package main

import (
"crypto/tls"
"crypto/x509"
"encoding/json"
"flag"
"fmt"
"io/ioutil"
"net/http"
"os"
"runtime"
Expand Down Expand Up @@ -94,11 +97,13 @@ func main() {
level.Debug(logger).Log("msg", " matched receiver", "receiver", conf.Name)

// TODO: Consider reusing notifiers or just jira clients to reuse connections.
tp := jira.BasicAuthTransport{
Username: conf.User,
Password: string(conf.Password),
hc, err := createHTTPClient(conf)
if err != nil {
errorHandler(w, http.StatusInternalServerError, err, conf.Name, &data, logger)
return
}
client, err := jira.NewClient(tp.Client(), conf.APIURL)

client, err := jira.NewClient(hc, conf.APIURL)
if err != nil {
errorHandler(w, http.StatusInternalServerError, err, conf.Name, &data, logger)
return
Expand Down Expand Up @@ -179,3 +184,94 @@ func setupLogger(lvl string, fmt string) (logger log.Logger) {
logger = log.With(logger, "ts", log.DefaultTimestampUTC, "caller", log.DefaultCaller)
return
}

// createHTTPClient returns a jira.BasicAuthTransport or http Client, depending
// on CAFile/client certificate options
func createHTTPClient(conf *config.ReceiverConfig) (*http.Client, error) {

// if CAFile, CertFile or KeyFile aren't specified, return BasicAuthTransport client
if len(conf.CAFile) == 0 && len(conf.CertFile) == 0 && len(conf.KeyFile) == 0 {
tp := jira.BasicAuthTransport{
Username: conf.User,
Password: string(conf.Password),
}
return tp.Client(), nil
}

tlsConfig, err := newTLSConfig(conf)
if err != nil {
return nil, err
}

hc := &http.Client{
Transport: &http.Transport{
TLSClientConfig: tlsConfig,
},
}

return hc, nil
}

func newTLSConfig(conf *config.ReceiverConfig) (*tls.Config, error) {
tlsConfig := &tls.Config{
InsecureSkipVerify: conf.InsecureSkipVerify,
Renegotiation: tls.RenegotiateOnceAsClient,
}

// Read in a CA certificate, if one is specified.
if len(conf.CAFile) > 0 {
b, err := readCAFile(conf.CAFile)
if err != nil {
return nil, err
}
if !updateRootCA(tlsConfig, b) {
return nil, fmt.Errorf("unable to use specified CA certificate %s", conf.CAFile)
}
}

// Configure TLS with a client certificate, if certificate and key files are specified.
if len(conf.CertFile) > 0 && len(conf.KeyFile) == 0 {
return nil, fmt.Errorf("client certificate file %q specified without client key file", conf.CertFile)
}

if len(conf.KeyFile) > 0 && len(conf.CertFile) == 0 {
return nil, fmt.Errorf("client key file %q specified without client certificate file", conf.KeyFile)
}

if len(conf.CertFile) > 0 && len(conf.KeyFile) > 0 {
cert, err := getClientCertificate(conf)
if err != nil {
return nil, err
}
tlsConfig.Certificates = []tls.Certificate{*cert}
}

return tlsConfig, nil
}

// readCAFile reads the CA certificate file from disk.
func readCAFile(f string) ([]byte, error) {
data, err := ioutil.ReadFile(f)
if err != nil {
return nil, fmt.Errorf("unable to load specified CA certificate %s: %s", f, err)
}
return data, nil
}

func updateRootCA(cfg *tls.Config, b []byte) bool {
caCertPool := x509.NewCertPool()
if !caCertPool.AppendCertsFromPEM(b) {
return false
}
cfg.RootCAs = caCertPool
return true
}

// getClientCertificate reads the pair of client certificate and key from disk and returns a tls.Certificate.
func getClientCertificate(c *config.ReceiverConfig) (*tls.Certificate, error) {
cert, err := tls.LoadX509KeyPair(c.CertFile, c.KeyFile)
if err != nil {
return nil, fmt.Errorf("unable to use specified client certificate (%s) and key (%s): %s", c.CertFile, c.KeyFile, err)
}
return &cert, nil
}
3 changes: 3 additions & 0 deletions examples/jiralert.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ defaults:
api_url: https://jiralert.atlassian.net
user: jiralert
password: 'JIRAlert'
# Optional client certificate.
# cert_file: /path/to/cert.crt
# key_file: /path/to/cert.key

# The type of JIRA issue to create. Required.
issue_type: Bug
Expand Down
33 changes: 30 additions & 3 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,13 @@ type ReceiverConfig struct {
Name string `yaml:"name" json:"name"`

// API access fields
APIURL string `yaml:"api_url" json:"api_url"`
User string `yaml:"user" json:"user"`
Password Secret `yaml:"password" json:"password"`
APIURL string `yaml:"api_url" json:"api_url"`
User string `yaml:"user" json:"user"`
Password Secret `yaml:"password" json:"password"`
CAFile string `yaml:"ca_file" json:"ca_file"`
CertFile string `yaml:"cert_file" json:"cert_file"`
KeyFile string `yaml:"key_file" json:"key_file"`
InsecureSkipVerify bool `yaml:"insecure_skip_verify" json:"insecure_skip_verify"`

// Required issue fields
Project string `yaml:"project" json:"project"`
Expand Down Expand Up @@ -178,6 +182,9 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
if _, err := url.Parse(rc.APIURL); err != nil {
return fmt.Errorf("invalid api_url %q in receiver %q: %s", rc.APIURL, rc.Name, err)
}
// Username and password aren't necessarily required if using a client
// certificate for authentication, but they will (usually) be ignored in
// that situation, so leaving them as required.
if rc.User == "" {
if c.Defaults.User == "" {
return fmt.Errorf("missing user in receiver %q", rc.Name)
Expand All @@ -191,6 +198,26 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
rc.Password = c.Defaults.Password
}

// We want to be able to override CAFile/KeyFile/CertFile in each receiver
// even if there is a default value set. Setting it to a blank string will
// cause the existing logic to just fall back to the default, so we use
// "none" to indicate no value set.
if rc.CAFile == "none" {
Copy link
Member

Choose a reason for hiding this comment

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

none? hmmm, don't get this, why we need this?

Copy link
Author

Choose a reason for hiding this comment

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

This is what I was referring to in the first bullet point of the PR comment -

if you set a CAFile (or KeyFile/CertFile) in the defaults but didn't want it set in one of the specific receivers, you can't just set it to blank in the receiver config - the current config logic is that "if it's blank in the receiver config, use the default".

So I went with a "magic" value of "none" - I don't love that sort of magic, but I'm not sure what else to do short of totally rewriting the config logic. AFAICT, you can't set 'nil' in YAML - setting it to 'null' or '~' just results in an empty string.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect the following to work:

Marshal the default config
Then marshal the receiver config into the initialized default struct - all fields that are missing in the receiver's YAML will not override the defaults. All fields that are present in the receiver's config will override the default config.

Example receiver uses the defaults:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = file
		APIURL = url2

Example receiver overrides the defaults:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  cert_file: file2
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = file2
		APIURL = url2

Example receiver overrides the defaults and doesn't use cert:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  cert_file: ""
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = ""
		APIURL = url2

In other words, when the field doesn't exist in the receiver config use the default and in all other cases use the field value from the receiver config.

Also, let's add a test for this to make it clear and make sure we don't break this logic in the feature

Copy link
Author

Choose a reason for hiding this comment

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

(apologies for the big gap in responding - other work got in the way)

This may be my Go inexperience showing, but I believe that won't work as-is, since unmarshaling YAML with missing fields just initializes those fields to the empty string - it doesn't differentiate between "not specified" and "blank".

https://play.golang.org/p/_4ih2Xomnit

However, if I make them string pointers, the pointers will get initialized to nil if not set, which I can use to detect "not specified" vs. "blank". This appears to be one of the cases when using string pointers vs. strings makes sense: https://dhdersch.github.io/golang/2016/01/23/golang-when-to-use-string-pointers.html

Let me know if that sounds like a reasonable approach.

rc.CAFile = ""
} else if rc.CAFile == "" && c.Defaults.CAFile != "" {
rc.CAFile = c.Defaults.CAFile
}
if rc.CertFile == "none" {
rc.CertFile = ""
} else if rc.CertFile == "" && c.Defaults.CertFile != "" {
rc.CertFile = c.Defaults.CertFile
}
if rc.KeyFile == "none" {
rc.KeyFile = ""
} else if rc.KeyFile == "" && c.Defaults.KeyFile != "" {
rc.KeyFile = c.Defaults.KeyFile
}

// Check required issue fields
if rc.Project == "" {
if c.Defaults.Project == "" {
Expand Down